-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[$500] IOU - "Unsubscribe from thread" button is visible on the hover menu #34428
Comments
Triggered auto assignment to @puneetlath ( |
Job added to Upwork: https://www.upwork.com/jobs/~0117b089c8644a20b4 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @s77rt ( |
ProposalPlease re-state the problem that we are trying to solve in this issue."Unsubscribe from thread" button is visible for a short time on the hover menu. What is the root cause of that problem?
In here because we don't set childReportNotificationPreference in reportAction of request money message, childReportNotificationPreference is empty string then we check isActionCreator App/src/pages/home/report/ContextMenu/ContextMenuActions.js Lines 201 to 202 in b6a18b1
so subscribed is true and we display unsubcribe button on the context menu
What changes do you think we should make in order to solve the problem?Note that: This bug only be happened at a moment because the API return We should set childReportNotificationPreferenc === 'hidden' in optimistic data of request money message Line 2899 in b6a18b1
We should add
as the value is returned from BE What alternative solutions did you explore? (Optional)NA |
ProposalPlease re-state the problem that we are trying to solve in this issue.The unsubscribe option menu shows for a brief moment on a scanning request. What is the root cause of that problem?One of the conditions the unsubs option will show is that the App/src/pages/home/report/ContextMenu/ContextMenuActions.js Lines 199 to 213 in b6a18b1
Because it's empty, based on the logic above, the value will default to ALWAYS because we are the creator. Then, later we will get a HIDDEN value from the server, so the button hides, but the subscribe option never shows because we hide it if it's a whisper. App/src/pages/home/report/ContextMenu/ContextMenuActions.js Lines 170 to 171 in b6a18b1
What changes do you think we should make in order to solve the problem?We can hide the unsubscribe option too if it's a whisper. we can optionally set the notification preference to hidden optimistically ONLY when the request is going to be scanned by checking the length of the |
Contributor details Problem Statement: Root Cause Analysis: Solution Approach: Explicitly Set childReportNotificationPreference: javascript javascript javascript Conclusion: I look forward to your feedback and the opportunity to contribute to enhancing the stability and usability of the Expensify system. Best regards, |
📣 @AliKhokhar123! 📣
|
@DylanDylann Thanks for the proposal. Your RCA is about right. However when the server response is received I expected the subscribe button to appear. The fact that it didn't means we have some other inconsistency to deal with (not a blocker). Setting the |
@bernhardoj Thanks for the proposal. Your RCA is correct. Hiding the unsubscribe option for whisper actions seems to be the most straightforward solution here. Let's go with that. 🎀 👀 🎀 C+ reviewed cc @srikarparsi |
Current assignee @puneetlath is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new. |
@AliKhokhar123 Thanks for the proposal. Unfortunately it's a dupe. |
@puneetlath While at it, can you check those two BE bugs (also related to this issue): Bug 1:
Bug 2
Technically the BE fix should also fix this issue. But for consistency I think we should still apply the FE fix (disable unsubscribe option for whisper actions) Screen.Recording.2024-01-13.at.2.44.54.PM.mov |
Not overdue. @puneetlath ^ |
Hey, coming from this issue where we removed the ability to |
Not overdue. @puneetlath ^ |
@srikarparsi are you saying that the current behavior is the right behavior and we should close this issue? |
Sorry, I think that this issue is a bug. I just think that the solution to this is to remove the I previously wanted to keep the |
@puneetlath Did you get a chance to check #34428 (comment)? |
Issue not reproducible during KI retests. (First week) |
@puneetlath @s77rt this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks! |
@puneetlath When you get a chance please check #34428 (comment) Also cc @rlinoz as I think you worked on notification preferences |
I think we started returning |
Not overdue. ^ |
Hey, we actually removed the leave thread/unsubscribe from thread option everywhere so I don't think this issue will be there anymore? |
@puneetlath @s77rt this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:
Thanks! |
Current assignee @s77rt is eligible for the Internal assigner, not assigning anyone new. |
@srikarparsi Correct! This is no longer reproducible |
Ah cool, I'll go ahead and close this out then. Thanks y'all. |
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.7
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:
The button shouldn't disappear.
Actual Result:
"Unsubscribe from thread" button is visible for a short time on the hover menu.
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6339779_1705039568974.bandicam_2024-01-12_06-59-14-888.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: