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-07-22] [HOLD for payment 2024-07-17] [$250] Expensify personal - Users are able to create threads in the "System" DM chat #43562

Closed
6 tasks done
lanitochka17 opened this issue Jun 12, 2024 · 37 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 Jun 12, 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.82-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Issue found when executing PR #41343

Action Performed:

  1. Go to staging.new.expensify.com
  2. Create a new Gmail account
  3. Go through onboarding flow
  4. Go to Expensify chat
  5. Right click on any message from Expensify > Reply in thread
  6. Wait for the composer in thread to disappear
  7. Click on chat header > Notification preferences
  8. Change to other preference

Expected Result:

In Step 5, app should disable reply in thread to Expensify message

Actual Result:

In Step 5, user can reply in thread to Expensify message. Then the composer disappears after replying in thread (Step 6)
In Step 8, after changing notification preference in thread message with Expensify, the preference reverts to "Immediately"

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

Bug6510129_1718154038828.20240612_085358.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01aaf20e189bef2fa2
  • Upwork Job ID: 1805918670583601550
  • Last Price Increase: 2024-06-26
  • Automatic offers:
    • hoangzinh | Reviewer | 102966604
    • c3024 | Contributor | 102966605
Issue OwnerCurrent Issue Owner: @muttmuure
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 12, 2024
Copy link

melvin-bot bot commented Jun 12, 2024

Triggered auto assignment to @muttmuure (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.

@lanitochka17
Copy link
Author

@muttmuure FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@melvin-bot melvin-bot bot added the Overdue label Jun 14, 2024
Copy link

melvin-bot bot commented Jun 17, 2024

@muttmuure Huh... This is 4 days overdue. Who can take care of this?

@muttmuure
Copy link
Contributor

Not overdue

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jun 18, 2024
@c3024
Copy link
Contributor

c3024 commented Jun 23, 2024

Proposal

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

A user can reply to a thread for messages in chat with Expensify by right clicking on a report action in the chat.

What is the root cause of that problem?

There should be no Reply in thread and some other options in the right click context menu action popover.

Incidentally, the MiniReportActionContextMenu that appears on hovering the report action, however, does not show these options.

miniActions.mov

That is because we pass disabledActions here to the MiniReportActionContextMenu

disabledActions={!ReportUtils.canWriteInReport(report) ? RestrictedReadOnlyContextMenuActions : []}

but we do not pass them here to the right click report action context menu

The disabledActions include these here

const restrictedReadOnlyActions: TranslationPaths[] = [
'common.download',
'reportActionContextMenu.replyInThread',
'reportActionContextMenu.editAction',
'reportActionContextMenu.joinThread',
'reportActionContextMenu.deleteAction',
];
const RestrictedReadOnlyContextMenuActions: ContextMenuAction[] = ContextMenuActions.filter(
(action) => 'textTranslateKey' in action && restrictedReadOnlyActions.includes(action.textTranslateKey),
);

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

Pass the disabledActions here


similar to this
disabledActions={!ReportUtils.canWriteInReport(report) ? RestrictedReadOnlyContextMenuActions : []}

Once we fix this a thread cannot be started and the other issue of changing notification preference does not exist anymore.
Additionally in the other issue, the notification preference changes optimistically when we select another option but backend returns a 404 error later saying report not found so the notification preference gets reset.

What alternative solutions did you explore? (Optional)

Copy link

melvin-bot bot commented Jun 24, 2024

@muttmuure Huh... This is 4 days overdue. Who can take care of this?

@muttmuure muttmuure added the External Added to denote the issue can be worked on by a contributor label Jun 26, 2024
Copy link

melvin-bot bot commented Jun 26, 2024

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

@melvin-bot melvin-bot bot changed the title Expensify personal - Cannot select notification settings after replying in thread to Expensify [$250] Expensify personal - Cannot select notification settings after replying in thread to Expensify Jun 26, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 26, 2024
Copy link

melvin-bot bot commented Jun 26, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Jun 26, 2024
Copy link

melvin-bot bot commented Jun 26, 2024

@hoangzinh @muttmuure 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!

@hoangzinh
Copy link
Contributor

@c3024's proposal #43562 (comment) looks good to me

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Jun 27, 2024

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

@marcaaron
Copy link
Contributor

That is because we pass disabledActions here to the MiniReportActionContextMenu
but we do not pass them here to the right click report action context menu

Are we sure this is not intentional behavior? Did some PR introduce the regression?

@c3024
Copy link
Contributor

c3024 commented Jun 27, 2024

It is not a recent PR but I think it was missed to be included in the right click Context Menu in this PR #40010. Here it was included only in the mini menu.

disabledActions={ReportUtils.isReadOnly(report) ? RestrictedReadOnlyContextMenuActions : []}

I think both Context Menus should have the same options. Can they be different? 🤔

@marcaaron
Copy link
Contributor

I think both Context Menus should have the same options. Can they be different?

I'm not sure. It might be intentional. That's why I'm asking about it.

Can you maybe post what it looks like with all the options so that Design can review the change?

In Step 5, app should disable reply in thread to Expensify message

Also this is not really clear to me. Why should it be expected that we disable this? @muttmuure what's your interpretation of this problem? I feel like I'm probably missing what problem we're trying to solve with this one.

I watched the video and it looks like the bug is related to not being able to set a notification preference so not really sure what that has to do with context menu actions really. 🤷‍♂️

@marcaaron
Copy link
Contributor

app should disable reply in thread to Expensify message

I think this is the part we should be focusing on. Confirming whether this is true or not? If there's no reason why someone wouldn't be able to reply in a thread then this looks like more of a backend problem. But tbh, not sure we should do anything about this right now.

@c3024
Copy link
Contributor

c3024 commented Jun 28, 2024

Can you maybe post what it looks like with all the options so that Design can review the change?

Here are the options in the MiniReportActionContextMenu (menu on hover): QuickReactionEmojis, Emoji Picker, Mark as unread, Copy to clipboard, Copy link.

Here are the options in the ReportActionContextMenu (menu on right-click): QuickReactionEmojis, Emoji Picker, Mark as unread, Copy to clipboard, Copy link, Reply in thread, Join thread.

contextMenuOptions.mov

Also this is not really clear to me. Why should it be expected that we disable this?

We do not display the composer for chats with Expensify, and the footer prompts users to chat with Concierge for setup.

Screenshot 2024-06-28 at 8 44 01 AM

This is due to canWriteInReport being false for this report, among other reasons.

In my view, since messaging is not permitted in this report, creating threads should also be restricted. This is the behaviour in all cases as far as I remember. So, in this case, it seems the option was intentionally removed from the mini menu, but its removal from the right-click menu was missed.

I watched the video and it looks like the bug is related to not being able to set a notification preference so not really sure what that has to do with context menu actions really.

I think the thread should not have been created in the first place and backend error is also due to this forbidden creation 😃. That is why I suggested that fix.

Copy link

melvin-bot bot commented Jul 2, 2024

@hoangzinh, @marcaaron, @muttmuure Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot melvin-bot bot added the Overdue label Jul 2, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jul 4, 2024
@c3024
Copy link
Contributor

c3024 commented Jul 4, 2024

@hoangzinh PR is ready!

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jul 10, 2024
@melvin-bot melvin-bot bot changed the title [$250] Expensify personal - Users are able to create threads in the "System" DM chat [HOLD for payment 2024-07-17] [$250] Expensify personal - Users are able to create threads in the "System" DM chat Jul 10, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jul 10, 2024
Copy link

melvin-bot bot commented Jul 10, 2024

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

Copy link

melvin-bot bot commented Jul 10, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.5-13 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-07-17. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Jul 10, 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:

  • [@hoangzinh / @c3024] The PR that introduced the bug has been identified. Link to the PR:
  • [@hoangzinh / @c3024] 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:
  • [@hoangzinh / @c3024] 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:
  • [@hoangzinh / @c3024] Determine if we should create a regression test for this bug.
  • [@hoangzinh / @c3024] 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.
  • [@muttmuure] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jul 15, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-07-17] [$250] Expensify personal - Users are able to create threads in the "System" DM chat [HOLD for payment 2024-07-22] [HOLD for payment 2024-07-17] [$250] Expensify personal - Users are able to create threads in the "System" DM chat Jul 15, 2024
Copy link

melvin-bot bot commented Jul 15, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.6-8 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-07-22. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Jul 15, 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:

  • [@hoangzinh / @c3024] The PR that introduced the bug has been identified. Link to the PR:
  • [@hoangzinh / @c3024] 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:
  • [@hoangzinh / @c3024] 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:
  • [@hoangzinh / @c3024] Determine if we should create a regression test for this bug.
  • [@hoangzinh / @c3024] 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.
  • [@muttmuure] 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 Overdue and removed Weekly KSv2 labels Jul 16, 2024
Copy link

melvin-bot bot commented Jul 19, 2024

@hoangzinh, @marcaaron, @muttmuure, @c3024 Whoops! This issue is 2 days overdue. Let's get this updated quick!

@marcaaron
Copy link
Contributor

Are we blocked on anything here?

@c3024
Copy link
Contributor

c3024 commented Jul 23, 2024

Nope. Awaiting payment from @muttmuure since 17-Jul. Automation incorrectly added another date.

Copy link

melvin-bot bot commented Jul 23, 2024

@hoangzinh, @marcaaron, @muttmuure, @c3024 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@hoangzinh
Copy link
Contributor

hoangzinh commented Jul 24, 2024

BugZero Checklist:

  • The PR that introduced the bug has been identified. Link to the PR: Support read only messages #40010
  • 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: https://github.com/Expensify/App/pull/40010/files#r1689630389
  • 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: N/A
  • Determine if we should create a regression test for this bug: ✅ Yes we should

@hoangzinh
Copy link
Contributor

Regression Test Proposal

Web/Desktop

  1. Sign in to an account that has Expensify (DM) chat
  2. Right-click on any message to open the context action menu.
  3. Verify that the Reply in thread and Join thread options are not displayed

mWeb/Native apps

  1. Sign in to an account that has Expensify (DM) chat
  2. Tap and hold on any message to open the context action menu.
  3. Verify that the Reply in thread and Join thread options are not displayed

Do we agree 👍 or 👎

@marcaaron
Copy link
Contributor

LGTM

@muttmuure
Copy link
Contributor

All paid up

@melvin-bot melvin-bot bot removed the Overdue label Jul 25, 2024
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
No open projects
Archived in project
Development

No branches or pull requests

6 participants