Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$125] Workspace - System message changes in the #admins room after changing WS description #53534

Closed
2 of 8 tasks
IuliiaHerets opened this issue Dec 4, 2024 · 44 comments
Closed
2 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Dec 4, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.71-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5296387&group_by=cases:section_id&group_id=229065&group_order=asc
Email or phone of affected tester (no customers): N/A
Issue reported by: Applause Internal Team

Action Performed:

  1. Navigate to https://staging.new.expensify.com/
  2. Log in with a Gmail account
  3. Create a new workspace
  4. Navigate to Workspace settings - Description
  5. Replace the default description with a few letters
  6. Navigate to the #admins room of the workspace

Expected Result:

System message should be "updated the description of this workspace to".

Actual Result:

System message changes in the #admins room after changing WS description. "updated the description of this workspace from "" to" message is briefly visible before it changes to "updated the description of this workspace to".

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6683907_1733291774960.bandicam_2024-12-04_06-47-43-549.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021864276008252275164
  • Upwork Job ID: 1864276008252275164
  • Last Price Increase: 2025-01-07
Issue OwnerCurrent Issue Owner: @mountiny
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 4, 2024
Copy link

melvin-bot bot commented Dec 4, 2024

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@nkdengineer
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

System message changes in the #admins room after changing WS description. "updated the description of this workspace from "" to" message is briefly visible before it changes to "updated the description of this workspace to".

What is the root cause of that problem?

We don't have a translation for POLICYCHANGELOG_UPDATE_DESCRIPTION action and it displays the message[0]?.html by default. After we change the workspace's description, the backend returns the message updated the description of this workspace from "" to. But it's changed to updated the description of this workspace to ... after OpenReport API is called

Screenshot 2024-12-04 at 17 23 58

What changes do you think we should make in order to solve the problem?

  1. We need to define a new translation for POLICYCHANGELOG_UPDATE_DESCRIPTION action. It is updated the description of this workspace from "${oldDescription}" to "${newDescription}" if oldDescription is not empty. If not, it is updated the description of this workspace to "${newDescription}" if oldDescription is empty
updateDescription: ({oldDescription, newDescription}: {oldDescription: string; newDescription: string}) =>
                `updated the description of this workspace ${oldDescription ? `"from ${oldDescription} "` : ''}to "${newDescription}"`,
  1. Add CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.UPDATE_DESCRIPTION as an old dot action

  1. Add a case for this action in getMessageOfOldDotReportAction function so it can work in all places like LHN, ReportActionItem, thread name header, copy to clipboard
case CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.UPDATE_DESCRIPTION: {
    const {oldDescription, newDescription}
    return Localize.translateLocal('workspace.common.updateDescription', {oldDescription, newDescription})
}

function getMessageOfOldDotReportAction(oldDotAction: PartialReportAction | OldDotReportAction, withMarkdown = true): string {

OPTIONAL: When we update workspace description we can generate this action in optimistic data and send the reportActionID to backend to generate this action

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

None

What alternative solutions did you explore? (Optional)

@trjExpensify trjExpensify added the External Added to denote the issue can be worked on by a contributor label Dec 4, 2024
@melvin-bot melvin-bot bot changed the title Workspace - System message changes in the #admins room after changing WS description [$250] Workspace - System message changes in the #admins room after changing WS description Dec 4, 2024
Copy link

melvin-bot bot commented Dec 4, 2024

Job added to Upwork: https://www.upwork.com/jobs/~021864276008252275164

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 4, 2024
Copy link

melvin-bot bot commented Dec 4, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @jjcoffee (External)

@trjExpensify trjExpensify moved this to Bugs and Follow Up Issues in [#whatsnext] #expense Dec 4, 2024
@jjcoffee
Copy link
Contributor

jjcoffee commented Dec 4, 2024

@nkdengineer's proposal LGTM. I think a unit test might be appropriate here though?

@jjcoffee
Copy link
Contributor

jjcoffee commented Dec 4, 2024

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Dec 4, 2024

Triggered auto assignment to @mountiny, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@nkdengineer
Copy link
Contributor

I think a unit test might be appropriate here though?

If we want I think we can add a unit test for getMessageOfOldDotReportAction with CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.UPDATE_DESCRIPTION and verify that the translation is returned correctly for each case of oldDescription and newDescription.

@twilight2294
Copy link
Contributor

this needs to be fixed on the BE i guess, if the admin/owner is editing description for the first time, this bug occurs because BE doesn't check if we are editing for the first time. The OpenReport call sends the correct data which means we only need to fix the changelog when the user updates the description, what do you think @jjcoffee ?

@FitseTLT
Copy link
Contributor

FitseTLT commented Dec 4, 2024

This is a BE bug

@jjcoffee
Copy link
Contributor

jjcoffee commented Dec 4, 2024

Ah my bad I thought it was being set optimistically! @mountiny I guess it doesn't make sense to add a new translation for POLICYCHANGELOG_UPDATE_DESCRIPTION? If that's the case this can be internal.

@nkdengineer
Copy link
Contributor

I guess it doesn't make sense to add a new translation for POLICYCHANGELOG_UPDATE_DESCRIPTION

We already do this for update room description so I think we can do the same for update policy description.

updateRoomDescription: 'set the room description to:',

cc @trjExpensify What do you think?

@mountiny mountiny added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Dec 4, 2024
@mountiny
Copy link
Contributor

mountiny commented Dec 5, 2024

Still need to make this change in the be

@trjExpensify
Copy link
Contributor

Gotcha, okay.

@mountiny
Copy link
Contributor

mountiny commented Dec 8, 2024

Started a draft

@melvin-bot melvin-bot bot added the Overdue label Dec 11, 2024
Copy link

melvin-bot bot commented Dec 12, 2024

@trjExpensify, @jjcoffee, @mountiny Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@mountiny mountiny added the Reviewing Has a PR in review label Dec 12, 2024
Copy link

melvin-bot bot commented Dec 20, 2024

@trjExpensify, @jjcoffee, @mountiny Whoops! This issue is 2 days overdue. Let's get this updated quick!

@trjExpensify
Copy link
Contributor

Alrighty, that Auth PR has been deployed. Are we good here?

@nkdengineer
Copy link
Contributor

@trjExpensify What do you think about my comment here #53534 (comment)?

@trjExpensify
Copy link
Contributor

Do other actualy workspace change logs to settings on the workspace get translated now, or only this room description example?

@nkdengineer
Copy link
Contributor

Do other actualy workspace change logs to settings on the workspace get translated now, 

@trjExpensify Yes some other change logs like remove/invite member, update workspace name,... have already translated.

@trjExpensify
Copy link
Contributor

"some other" ... but not all workspace change logs? 🤔

@nkdengineer
Copy link
Contributor

Yes, not all workspace change logs.

@trjExpensify
Copy link
Contributor

That's a sorta' weird inconsistency. Any idea why, @JmillsExpensify?

@nkdengineer
Copy link
Contributor

@trjExpensify I think this because we have many workspace change logs action, we're only focusing on the translation for the popular system logs mentioned above.

@trjExpensify
Copy link
Contributor

Hm, I don't think "workspace description" is that popular. But equally, whatever they change it to will be in the language they write. So I think it's probably fine.

I do think we should consider being consistent with translating workspace change logs and not partial ones though at some point.

Copy link

melvin-bot bot commented Dec 24, 2024

@trjExpensify, @jjcoffee, @mountiny 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

Copy link

melvin-bot bot commented Dec 26, 2024

@trjExpensify, @jjcoffee, @mountiny Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

@mountiny
Copy link
Contributor

I have also updated the copy in Web https://github.com/Expensify/Web-Expensify/pull/44911

@mountiny
Copy link
Contributor

What is the conclusion about the translations? I agree we should be consistent @trjExpensify

@trjExpensify
Copy link
Contributor

Yeah, seems like we should do the following:

  1. Add a translation for this one here to round out this issue.
  2. Create a new issue for translating workspace change logs overall such that they'e consistently translated. Though I would still like the input of @sakluger & @JmillsExpensify on why that isn't the case.

@JmillsExpensify
Copy link

Workspace change logs are super old, so I wouldn't be surprised if the entirety of them are not translated. In fact, workspace description would only added more recently right, as part of simplified collect? So maybe we didn't add the right translation at that point.

@trjExpensify
Copy link
Contributor

Hm yeah, it's strange that apparently only the workspace change logs for invite/remove workspace members and updating the workspace name are translated.

Copy link

melvin-bot bot commented Jan 7, 2025

@trjExpensify, @jjcoffee, @mountiny Eep! 4 days overdue now. Issues have feelings too...

@mountiny mountiny changed the title [$250] Workspace - System message changes in the #admins room after changing WS description [$125] Workspace - System message changes in the #admins room after changing WS description Jan 7, 2025
Copy link

melvin-bot bot commented Jan 7, 2025

Upwork job price has been updated to $125

@mountiny
Copy link
Contributor

mountiny commented Jan 7, 2025

@nkdengineer do you want to add the translations here for 125?

@nkdengineer
Copy link
Contributor

@mountiny Yeah I can do this.

@Krishna2323
Copy link
Contributor

Krishna2323 commented Jan 7, 2025

@mountiny, I'm updating all the translations in this PR.

@sakluger
Copy link
Contributor

sakluger commented Jan 8, 2025

Sorry for the late response - it doesn't look like we discussed translations at all in the original Workspace Change Logs design doc, so I'm not surprised that the functionality isn't really there for most changes.

I agree that we should create a separate issue to translating workspace change logs overall.

@trjExpensify
Copy link
Contributor

@mountiny, I'm updating all the translations in this PR.

Oh nice, so this is already in progress.

@mountiny
Copy link
Contributor

mountiny commented Jan 8, 2025

Nice so i think we can close this one

@mountiny mountiny closed this as completed Jan 8, 2025
@github-project-automation github-project-automation bot moved this from Bugs and Follow Up Issues to Done in [#whatsnext] #expense Jan 8, 2025
@trjExpensify
Copy link
Contributor

Yup, thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
Status: Done
Development

No branches or pull requests

10 participants