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 SEARCH] IOU Actions - Notifications don't come if subscribed from combined report #39706

Closed
3 of 6 tasks
izarutskaya opened this issue Apr 5, 2024 · 30 comments
Closed
3 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Monthly KSv2

Comments

@izarutskaya
Copy link

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: v1.4.60-5
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/cases/view/2730113
#39490
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

Preconditions: two users A and B.

  1. User A made Money Request to user B.
  2. User B open IOU report and send a message (this will subscribe the user to notifications).
  3. User B is navigated away from the conversation.
  4. User A edit Money request: Amount, Merchant, Description or Date.

Expected Result:

User B receives a notification for the edit action. The notification shows a preview of the action that was made by user A.

Actual Result:

User B not receives a notification for the edit action.

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

Bug6438370_1712258503824.bandicam_2024-04-04_22-05-52-118.mp4

View all open jobs on GitHub

@izarutskaya izarutskaya added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 5, 2024
Copy link

melvin-bot bot commented Apr 5, 2024

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

Copy link

melvin-bot bot commented Apr 5, 2024

Triggered auto assignment to @Julesssss (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@izarutskaya
Copy link
Author

@jliexpensify 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.

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Apr 5, 2024
Copy link
Contributor

github-actions bot commented Apr 5, 2024

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@izarutskaya
Copy link
Author

We think this issue might be related to the #vip-vsb.

@izarutskaya
Copy link
Author

Production

bandicam.2024-04-05.16-46-04-844.mp4

@mountiny mountiny added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Apr 5, 2024
@Julesssss
Copy link
Contributor

The checklist is small and I see no change related to editing expenses or notifications. I wonder if it might be this expected change.

@mountiny
Copy link
Contributor

mountiny commented Apr 5, 2024

I do not think this has to be a blocker.

The reason for this is that the single expense view is rendering the expense report id and the transaction thread report actions are then composed on it.

when we queue the notification for the transaction thread message/ change its not going to show up on the expense report view basically.

@NikkiWines not sure what would be the best way to handle this

@mountiny
Copy link
Contributor

mountiny commented Apr 5, 2024

Its coming from this PR #36934

@NikkiWines
Copy link
Contributor

Yeah agreed, it's not a blocker. Definitely something to investigate further though

@trjExpensify
Copy link
Contributor

Ah yeah, this is nice catch! With the actionable system messages project underway, we're going to "silence" all system messages by default, so these "edit" expense sys messages aren't going to notify or mark the chat as unread -- so do we need to solve this? 🤔

CC: @dylanexpensify @deetergp

@melvin-bot melvin-bot bot added the Overdue label Apr 8, 2024
@mountiny
Copy link
Contributor

mountiny commented Apr 8, 2024

we're going to "silence" all system messages by default, so these "edit" expense sys messages aren't going to notify or mark the chat as unread -- so do we need to solve this? 🤔

With the Search page project though, users/ admins can see the individual expenses in the All page of Search and then comment on the expense level/ transaction thread. So there will be a case of non-system message as well

Those comments should show up dynamically same as if you are chatting with someone in Home tab/ expense report

@Julesssss
Copy link
Contributor

Not overdue, discussion in progress

@melvin-bot melvin-bot bot removed the Overdue label Apr 8, 2024
@dylanexpensify
Copy link
Contributor

From the standpoint of system messages, this won't factor in (since they'll be silent), though I do think Vit brings up a good point about Search page

@trjExpensify
Copy link
Contributor

With the Search page project though, users/ admins can see the individual expenses in the All page of Search and then comment on the expense level/ transaction thread. So there will be a case of non-system message as well

I don't get the concern. If it's a one expense report they would see the combined view in the RHP all the same wouldn't they? :/

@mountiny
Copy link
Contributor

mountiny commented Apr 8, 2024

If it's a one expense report they would see the combined view in the RHP all the same wouldn't they?

Why would they? If its the All expense view and we list only transactions, we should show the transaction thread. Otherwise, if the expense is on multi-expense report we would change to transaction thread. This would be changing based on where the expense is/ context of the report and I think that would be confusing to the user

@Julesssss
Copy link
Contributor

Does someone with more context want to take this issue? Based on the above comments I think you'll be able to make a better decision regarding the solution here.

@trjExpensify
Copy link
Contributor

Why would they? If its the All expense view and we list only transactions, we should show the transaction thread.

Because in a "one expense report" case to the user there isn't an additional transaction thread. Even in the search v1 doc, you can see action buttons on one expense report rows in All.

Otherwise, if the expense is on multi-expense report we would change to transaction thread. This would be changing based on where the expense is/ context of the report and I think that would be confusing to the user

If that's a real concern, then it would be in the same vein as clicking a preview component in a chat that leads to a one expense report view or a multi-expense report view. The view you see when clicking the preview changes depending on how many expenses there are.

@mountiny
Copy link
Contributor

mountiny commented Apr 9, 2024

Because in a "one expense report" case to the user there isn't an additional transaction thread. Even in the search v1 doc, you can see action buttons on one expense report rows in All.

So in search v1, clicking expense that is on single expense report will open the expense report and if we open expense which is on multiple expense report, we will open transaction thread in RHP? I feel like this will be root cause of lots of bugs and confusion not only for users but also for developers, especially when we add held requests / deleting requests to the game and we would have to switch between what report to show dynamically

@JmillsExpensify
Copy link

I can see where you're coming from, though this is our design with one-expense reports vs grouped reports. I think we should lean into it for now and we can re-assess based on feedback.

@trjExpensify
Copy link
Contributor

Yeah, I don't see how that concern really is any different to a chat view where you click the report preview and either get the combined view or not.

@mountiny
Copy link
Contributor

mountiny commented Apr 9, 2024

I would love to be proven wrong, but I am happy to leave this up for later once we can get the feel of this in App and get some customer feedback 👍

@melvin-bot melvin-bot bot added the Overdue label Apr 11, 2024
Copy link

melvin-bot bot commented Apr 12, 2024

@Julesssss, @jliexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@jliexpensify
Copy link
Contributor

Hello @trjExpensify - apologies if I have misunderstood, but to summarise the discussion: we'll do nothing and close this issue (based off this and this)?

@melvin-bot melvin-bot bot removed the Overdue label Apr 16, 2024
@trjExpensify
Copy link
Contributor

IMO, we don't need to solve for it. If a user adds a comment and someone replies to that comment, they will be notified. The system messages are in the process of being silenced anyway. 👍

@mountiny
Copy link
Contributor

I think we can put this on hold for the search project and see if there are any problems with that

@melvin-bot melvin-bot bot added the Overdue label Apr 18, 2024
Copy link

melvin-bot bot commented Apr 19, 2024

@Julesssss @jliexpensify this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

Copy link

melvin-bot bot commented Apr 19, 2024

@Julesssss, @jliexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@jliexpensify
Copy link
Contributor

Moved to #Collect (as that's where the Search project is under) and popped on HOLD. Please feel free to close if you disagree!

@melvin-bot melvin-bot bot removed the Overdue label Apr 22, 2024
@jliexpensify jliexpensify changed the title IOU Actions - Notifications don't come if subscribed from combined report [HOLD FOR SEARCH] IOU Actions - Notifications don't come if subscribed from combined report Apr 22, 2024
@jliexpensify jliexpensify added Monthly KSv2 and removed Daily KSv2 labels Apr 22, 2024
@Julesssss
Copy link
Contributor

If a user adds a comment and someone replies to that comment

I'm with Tom here that we can close this issue. We're about to rework notifications. If anyone disagree we can re-open and discuss, but I think this is would end up being held for a long time just to become irrelevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Monthly KSv2
Projects
No open projects
Archived in project
Development

No branches or pull requests

8 participants