-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[HOLD for payment 2024-02-20] [$500] Room - In offline, changing room name, in room chat changed message is not displayed #34364
Comments
Job added to Upwork: https://www.upwork.com/jobs/~01cc0589030a9cc719 |
Triggered auto assignment to @anmurali ( |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @jjcoffee ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.In offline, changing room name, in room chat changed message is not displayed What is the root cause of that problem?We are only setting the updated room name in optimistic data and have not dealt with creating the renamed report action in App/src/libs/actions/Report.ts Lines 1719 to 1733 in 55746c7
What changes do you think we should make in order to solve the problem?We should also add the changed name message reportAction to the room in optimistic data same as what we get from the backend like
We should create renamed optimistic report action creator utility function in
And then add the
and
Additionally, ofcourse there is a BE job to align with this change to update the api to take the What alternative solutions did you explore? (Optional) |
ProposalPlease re-state the problem that we are trying to solve in this issue.In offline, changing room name, in room chat changed message is not displayed What is the root cause of that problem?We did not have the logic to update optimistic data for rename action in both room and workspace What changes do you think we should make in order to solve the problem?We should create the new function named
Then pass the renamed reportActionID to API's parameters, so BE can return the same report action to client (as what we already did with created action) We need to create the translation for text and update the report respectively as well. Improvement: Because What alternative solutions did you explore? (Optional)The room and WS share the same functionality so we need to update the WS as well. |
ProposalPlease re-state the problem that we are trying to solve in this issue.What is the root cause of that problem?Warning Please pay attention to the proposals' edit time. When I posted this proposal, the proposals above did not have the "send When renaming a room, we do not add an optimistic App/src/libs/actions/Report.ts Lines 1706 to 1764 in c0c4d64
What changes do you think we should make in order to solve the problem?First, we should build an optimistic report action with type="RENAME" and add it to the {
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
value: {
[optimisticRenamedAction.reportActionID]: optimisticRenamedAction,
},
}, Second, equally important: we should modify the We do similar with the task updates: Lines 434 to 443 in 382a096
This is needed to avoid duplicates when the BE response comes and adds another What alternative solutions did you explore? (Optional)To avoid BE changes, we could also remove this optimistic action on API call success or failure: {
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
value: {
[optimisticRenamedAction.reportActionID]: null,
},
}, |
Updated to add detailed implementation |
Sorry for the delay here, I'll review tomorrow! |
After an in-depth analysis of timestamps ( 😅 ), I think we can go with @tienifr's proposal. All three proposals are similar, but @tienifr got the most fully fleshed out proposal first, including an important detail of needing to translate the message text. I do appreciate @paultsimura's proposal also included an extra detail to avoid duplicate report actions that @tienifr later included, but IMO it would've been handled in the PR anyway. I'm not sure how long it would take to add BE support for an extra 🎀👀🎀 C+ reviewed |
Triggered auto assignment to @flodnv, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@neil-marcellini looks like you implemented this 5 months ago, can you check please? The proposal sounds reasonable to me, and I do think we should update the backend, as well as do this:
|
The proposal sounds good to me. I'm not sure what part I implemented 5 months ago. Let me know via DM if you need anything else from me. |
Not overdue. @flodnv and @neil-marcellini are working through what needs to change. |
Thanks. The PR is ready for review: #35826 |
Thanks, will review tomorrow! |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.40-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-02-20. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
Due in 4 days to be paid but I am OOO so I am reassigning. @jjcoffee meanwhile, if applicable, can you add regression tests and complete the BZ checklist? |
Triggered auto assignment to @strepanier03 ( |
Regression Test Proposal
Do we agree 👍 or 👎 |
Reg test created, everyone paid, closing out. Thanks again @jjcoffee and @paultsimura for your help getting this one finished up 🙌 |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 1.4.24
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
Action Performed:
Expected Result:
In offline, changing room name, in room chat changed message must be displayed
Actual Result:
In offline, changing room name, in room chat changed message is not displayed
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6338935_1704982987654.az_recorder_20240111_044733.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: