-
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
[HOLD for payment 2024-01-17] [$500] Thread - You can create a thread for a deleted message in offline mode #32236
Comments
Triggered auto assignment to @bfitzexpensify ( |
Job added to Upwork: https://www.upwork.com/jobs/~010998768658278f28 |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Thread - You can create a thread for a deleted message in offline mode What is the root cause of that problem?We do not have the conditions for this requirement What changes do you think we should make in order to solve the problem?As for me We can add new condition
App/src/pages/home/report/ContextMenu/ContextMenuActions.js Lines 124 to 134 in f7124d5
Also we can make the same for subscribeToThread What alternative solutions did you explore? (Optional)NA |
ProposalPlease re-state the problem that we are trying to solve in this issue.User can a thread for a deleted message in offline mode What is the root cause of that problem?we dont disable App/src/pages/home/report/ContextMenu/ContextMenuActions.js Lines 118 to 134 in 32338b2
What changes do you think we should make in order to solve the problem?we need to add const isCommentDeleted = ReportActionsUtils.isDeletedAction(reportAction);
return (
(isCommentAction || isReportPreviewAction || isIOUAction || isModifiedExpenseAction || isTaskAction) &&
!ReportUtils.isThreadFirstChat(reportAction, reportID) &&
!isCommentDeleted
); |
ProposalPlease re-state the problem that we are trying to solve in this issue.You can create a thread for a deleted message in offline mode What is the root cause of that problem?In here, we don't hide "Reply in thread" for deleted message that doesn't have an existing thread. What changes do you think we should make in order to solve the problem?In here, hide "Reply in thread" for deleted message (check using
We can do the same for "Subscribe/unsubscribe to thread" What alternative solutions did you explore? (Optional)NA |
ProposalPlease re-state the problem that we are trying to solve in this issue.You can create a thread for a deleted message in offline mode What is the root cause of that problem?We didn't consider pendingAction
What changes do you think we should make in order to solve the problem?we can update like this.
App/src/pages/home/report/ContextMenu/ContextMenuActions.js Lines 125 to 127 in 61ed900
What alternative solutions did you explore? (Optional) |
@eVoloshchak, @bfitzexpensify Eep! 4 days overdue now. Issues have feelings too... |
Proposals ready for review @eVoloshchak |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
Bump on review @eVoloshchak - thank you! |
Another bump here, thanks @eVoloshchak |
@eVoloshchak, @bfitzexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
@eVoloshchak @bfitzexpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks! |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@ZhenjaHorbach's proposal: this wouldn't work, since the button would be disabled for all the messages while offline, regardless of whether it was deleted or not @abzokhattab's proposal uses the correct approach, but as @dukenv0307 has pointed out, we shouldn't hide "Reply in thread" option if there is already an existing thread. There isn't much value in replying to a thread you've just deleted, but there is value in viewing the thread @dukenv0307's proposal looks good to me, addresses the issue above, I think we should proceed with this implementation 🎀👀🎀 C+ reviewed! |
@eVoloshchak The PR is ready for review. |
Bumping @eVoloshchak for review |
This issue has not been updated in over 15 days. @eVoloshchak, @Julesssss, @bfitzexpensify, @dukenv0307 eroding to Monthly issue. P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do! |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.23-4 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-01-17. 🎊 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:
|
I'm going to be mostly ooo until Jan 29, assigning a second BZ team member to keep an eye on this.
|
Triggered auto assignment to @trjExpensify ( |
@eVoloshchak, @Julesssss, @trjExpensify, @bfitzexpensify, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Kewl! Still awaiting the checklist, @eVoloshchak! |
Regression Test Proposal
Do we agree 👍 or 👎 |
Online today for a bit so I can take care of this. Payment summary:
Opened https://github.com/Expensify/Expensify/issues/361939 to add the regression test steps. We're all done here - thanks everyone! |
$500 approved for @eVoloshchak based on comment above. |
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: 1.4.5-3
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 function of creating a Thread should be disabled for messages deleted offline
Actual Result:
You can create a thread for a deleted message in offline mode
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6295222_1701296353332.Recording__786.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @eVoloshchakThe text was updated successfully, but these errors were encountered: