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

[HOLD for payment 2024-02-20] [$500] IOU - Inconsistency seen for regular messages &IOU while interacting with thread options #35092

Closed
2 of 6 tasks
lanitochka17 opened this issue Jan 24, 2024 · 24 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@lanitochka17
Copy link

lanitochka17 commented Jan 24, 2024

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.31-1
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:

  1. Go to https://staging.new.expensify.com/
  2. Send a message
  3. Long press the message and note context menu displays "leave thread" option
  4. Send a IOU
  5. Long press the message and note context menu displays "join thread" option
  6. Now long press text message, and tap " Leave thread"( note LHN behavior) and then "join thread" ( note LHN behavior)
  7. Note, tapping on join thread, a separate conversation is not displayed in LHN
  8. Now long press IOU created, and tap " join thread"( note LHN behavior) and then "Leave thread" ( note LHN behavior)
  9. Note, tapping on join thread, a separate conversation is displayed in LHN

Expected Result:

There must be consistent behavior in showing thread option in context menu for regular text messages and IOU. Similarly, consistent behavior must be shown on displaying conversation in LHN for regular text messages and IOU on tapping join thread

Actual Result:

When we send message, context menu shows leave thread option, but while request money, join thread option is displayed

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6353562_1706121043249.thread.mp4

View all open jobs on GitHub

@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 24, 2024
@melvin-bot melvin-bot bot changed the title IOU - Inconsistency seen for regular messages &IOU while interacting with thread options [$500] IOU - Inconsistency seen for regular messages &IOU while interacting with thread options Jan 24, 2024
Copy link

melvin-bot bot commented Jan 24, 2024

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

Copy link

melvin-bot bot commented Jan 24, 2024

Triggered auto assignment to @mallenexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

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

melvin-bot bot commented Jan 24, 2024

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

@paultsimura
Copy link
Contributor

This is intentional. The Expense reports have a "Hidden" notification preference by default, while the messages have "Always". The "join/leave thread" buttons are displayed based on the notification preference.

@fedirjh
Copy link
Contributor

fedirjh commented Jan 25, 2024

Long press the message and note context menu displays "leave thread" option

This seems like a bug to me. It makes sense to display this option only if the message is linked to a thread.

Long press the message and note context menu displays "join thread" option

This looks like a bug as well. Inside the IOU report, there is no option to leave the thread. Also, there is no option to change the notification preference

Screenshot 2024-01-25 at 10 37 59 PM Screenshot 2024-01-25 at 10 38 26 PM

@paultsimura
Copy link
Contributor

It makes sense to display this option only if the message is linked to a thread.

This was done intentionally to mimic the Slack's "Turn on/off notification for the responses".
Even if your message has no thread created, you automatically will be notified about the replies in the thread once it's created – this is only if you are the author of the message.

image

Same about messages posted by others – there might be a message that has no thread yet, but you want to subscribe to the thread once it's created (let's say, someone posted a question – it has no answers yet, but you want to be notified once they appear).

So by default, if you are the message's author, you will be notified about new replies, but you can unsubscribe (leave thread) manually. And if you are not the author – you can subscribe (join the thread) even if no thread has been created yet.

@paultsimura
Copy link
Contributor

I think @srikarparsi can tell more about the expected behavior here as he was the author of the leave/join functionality

@paultsimura
Copy link
Contributor

paultsimura commented Jan 26, 2024

Proposal

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

The "Join/Leave Thread" options appear for Report Preview (Expense & IOU).

What is the root cause of that problem?

We explicitly allow these options to appear for the REPORTPREVIEW actions:

shouldShow: (type, reportAction, isArchivedRoom, betas, menuTarget, isChronosReport, reportID) => {
const childReportNotificationPreference = ReportUtils.getChildReportNotificationPreference(reportAction);
const isDeletedAction = ReportActionsUtils.isDeletedAction(reportAction);
const shouldDisplayThreadReplies = ReportUtils.shouldDisplayThreadReplies(reportAction, reportID);
const subscribed = childReportNotificationPreference !== 'hidden';
const isCommentAction = reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT && !ReportUtils.isThreadFirstChat(reportAction, reportID);
const isReportPreviewAction = reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW;
const isIOUAction = reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU && !ReportActionsUtils.isSplitBillAction(reportAction);
const isWhisperAction = ReportActionsUtils.isWhisperAction(reportAction);
return !subscribed && !isWhisperAction && (isCommentAction || isReportPreviewAction || isIOUAction) && (!isDeletedAction || shouldDisplayThreadReplies);
},

shouldShow: (type, reportAction, isArchivedRoom, betas, menuTarget, isChronosReport, reportID) => {
const childReportNotificationPreference = ReportUtils.getChildReportNotificationPreference(reportAction);
const isDeletedAction = ReportActionsUtils.isDeletedAction(reportAction);
const shouldDisplayThreadReplies = ReportUtils.shouldDisplayThreadReplies(reportAction, reportID);
const subscribed = childReportNotificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;
if (type !== CONST.CONTEXT_MENU_TYPES.REPORT_ACTION) {
return false;
}
const isCommentAction = reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT && !ReportUtils.isThreadFirstChat(reportAction, reportID);
const isReportPreviewAction = reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW;
const isIOUAction = reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU && !ReportActionsUtils.isSplitBillAction(reportAction);
return subscribed && (isCommentAction || isReportPreviewAction || isIOUAction) && (!isDeletedAction || shouldDisplayThreadReplies);
},

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

If we want to hide the "Join/Leave Thread" options for Report Previews, we should change these conditions to check for !isReportPreviewAction:

return !subscribed && !isWhisperAction && !isReportPreviewAction && (isCommentAction || isIOUAction) && ...

Depending on the expected behavior, we can hide these options for the individual Money Request Previews as well – use !isIOUAction

Also, if we decide that we want to hide the "Join/Leave" options for the messages without actual threads created – we should early return false in the corresponding shouldShow functions if !lodashGet(reportAction, 'childReportID', '0').

What alternative solutions did you explore? (Optional)

@mallenexpensify
Copy link
Contributor

mallenexpensify commented Jan 27, 2024

@paultsimura thanks for chiming in here and providing context cuz I was on Team Fedi when thinking about

Long press the message and note context menu displays "leave thread" option

It seems weird to me that, in a DM, on a chat without a thread, that there'd be an option to 'leave thread' . The default for receiving notifications in Slack for room for thread comments:
No notification

  • User C is Member of room, ie. #expensify-open-source
  • User A leaves a comment
  • User B replies in thread
  • User C will NOT get a notification about User B's comment in thread

If the above is true, what's the purpose of 'leave thread' if you're not going to receive notifications? Maybe the behaviour is different in Slack DMs for threads? Also.. if User C was to click 'leave thread' then someone tagged them in the thread, 'we' would still want them to receive that notification, right? (ie. big convo in a thread, you leave thread cuz then later someone tags you asking a question. You'd want to be notified of that).

My intention is to open/reopen a can o' worms if this has already been discussed and consensus was reached, just trying to understand the potential bug here more (and typing out my thoughts on a Saturday morn when I need another cup of ☕ , bad)

@paultsimura
Copy link
Contributor

paultsimura commented Jan 27, 2024

@mallenexpensify when talking about "leave thread", I clarified that it's true for the messages you're the author of. Only for these messages, the notifications are enabled by default.
That's true for both Slack and ND.

@paultsimura
Copy link
Contributor

paultsimura commented Jan 27, 2024

User C is Member of room, ie. #expensify-open-source
User A leaves a comment
User B replies in thread
User C will NOT get a notification about User B's comment in thread

For this scenario, the User C will have an option to "join thread", not "leave".

I'm not sure how the mentions work in ND when you've left the thread though🤔

@mallenexpensify
Copy link
Contributor

clarified that it's true for the messages you're the author of.

ah ha, that makes sense then. Thanks for the clarification. Based on all the details we now have, @fedirjh , can you review @paultsimura 's proposal above?

@melvin-bot melvin-bot bot removed the Overdue label Jan 31, 2024
@fedirjh
Copy link
Contributor

fedirjh commented Feb 1, 2024

@paultsimura's proposal looks good to me.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Feb 1, 2024

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

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 2, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review and removed Daily KSv2 labels Feb 3, 2024
@melvin-bot melvin-bot bot added the Weekly KSv2 label Feb 3, 2024
@paultsimura
Copy link
Contributor

Thanks. The PR is ready for review: #35769

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Feb 13, 2024
@melvin-bot melvin-bot bot changed the title [$500] IOU - Inconsistency seen for regular messages &IOU while interacting with thread options [HOLD for payment 2024-02-20] [$500] IOU - Inconsistency seen for regular messages &IOU while interacting with thread options Feb 13, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Feb 13, 2024
Copy link

melvin-bot bot commented Feb 13, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Feb 13, 2024

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:

  • @paultsimura requires payment (Needs manual offer from BZ)
  • @fedirjh requires payment (Needs manual offer from BZ)

Copy link

melvin-bot bot commented Feb 13, 2024

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:

  • [@fedirjh] The PR that introduced the bug has been identified. Link to the PR:
  • [@fedirjh] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@fedirjh] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@fedirjh] Determine if we should create a regression test for this bug.
  • [@fedirjh] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Feb 20, 2024
Copy link

melvin-bot bot commented Feb 20, 2024

Payment Summary

Upwork Job

  • ROLE: @paultsimura paid $(AMOUNT) via Upwork (LINK)
  • ROLE: @fedirjh paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@mallenexpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@melvin-bot melvin-bot bot added the Overdue label Feb 22, 2024
@mallenexpensify
Copy link
Contributor

@fedirjh @paultsimura can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~01457ddb9bb6fce778

@melvin-bot melvin-bot bot removed the Overdue label Feb 22, 2024
@paultsimura
Copy link
Contributor

Accepted, thanks @mallenexpensify

@fedirjh
Copy link
Contributor

fedirjh commented Feb 22, 2024

@mallenexpensify Thank you. Accepted

@mallenexpensify
Copy link
Contributor

Contributor: @paultsimura paid $500 via Upwork
Contributor+: @fedirjh paid $500 via Upwork

@fedirjh , will you complete the BZ Checklist above? Thx

@fedirjh
Copy link
Contributor

fedirjh commented Feb 24, 2024

BugZero Checklist:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

5 participants