Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.48k stars 2.84k forks source link

[$250] Expense - Mark as unread green line for one transaction is not shown initially #50568

Open lanitochka17 opened 1 week ago

lanitochka17 commented 1 week ago

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: 9.0.47-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: N/A Issue reported by: Applause - Internal Team

Action Performed:

  1. Launch app
  2. Tap on a workspace chat
  3. Submit an expense
  4. Open the expense
  5. Select a category
  6. For category selected system message is displayed below, mark it as unread
  7. Note new green line not displayed
  8. Long press the message and open reply in thread
  9. Note you can check new green line in thread page
  10. Tap from link and now note for category selected system message, green line displayed now

Expected Result:

Mark as unread green line for one transaction must be shown initially before opening reply in thread page

Actual Result:

Mark as unread green line for one transaction is not shown initially before opening reply in thread page

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/0b756d09-17e1-43eb-a505-2579e0b9f6be

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021846309887749659812
  • Upwork Job ID: 1846309887749659812
  • Last Price Increase: 2024-10-15
Issue OwnerCurrent Issue Owner: @dukenv0307
melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @johncschuster (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 commented 1 week ago

@johncschuster 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[bot] commented 6 days ago

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

melvin-bot[bot] commented 6 days ago

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

melvin-bot[bot] commented 6 days ago

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

truph01 commented 6 days ago

Edited by proposal-police: This proposal was edited at 2024-10-16 09:27:54 UTC.

Proposal

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

Mark as unread green line for one transaction is not shown initially before opening reply in thread page

What is the root cause of that problem?

When users go to one transaction report, we will open iou report and combine the actions from iou report and transaction thread report

https://github.com/Expensify/App/blob/96ffbfe27c5c1b85b062e6ab50c0adc46b0e94c3/src/pages/home/report/ReportActionsView.tsx#L213

Here is the logic to get unread report action id

https://github.com/Expensify/App/blob/96ffbfe27c5c1b85b062e6ab50c0adc46b0e94c3/src/pages/home/report/ReportActionsList.tsx#L219-L222

we use unreadMarkerTime from report.lastReadTime

https://github.com/Expensify/App/blob/96ffbfe27c5c1b85b062e6ab50c0adc46b0e94c3/src/pages/home/report/ReportActionsList.tsx#L209

The report here is iou report

When users press mark as unread the category selected system message, we call markAsUnread API, this API will update report.lastReadTime, but the updated report is transaction thread report since this message comes from transaction report, not iou report

That why the logic to get unreadMarkerReportActionID is not triggered

There's one more issue when we open iouReport for one transaction:

If users add new messages, they would expect to add them in thread report, but they're actually added in iou report -> That causes the confusion

https://github.com/user-attachments/assets/0a58f75b-567e-4eeb-bcb6-a1cafaa8bda9

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

We should go to the transaction report if there'e one transaction

Update this line to

const transactionThreadReportID = ReportActionsUtils.getOneTransactionThreadReportID(iouReportID ?? '', ReportActionsUtils.getAllReportActions(iouReportID), isOffline);
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(transactionThreadReportID ?? iouReportID));

We also need to update the header to back to ws/1:1 chat

What alternative solutions did you explore? (Optional)

dukenv0307 commented 5 days ago

@truph01 Thanks for your proposal. I think we did it on purpose here: https://github.com/Expensify/App/issues/38655

But I'm not sure why we have to do that. Can you elaborate on why we should open the transaction report for one transaction?

truph01 commented 5 days ago

@dukenv0307 As you can see here's the expense report with one transaction. It's actually the UI of transaction thread report, so I believe when users add new comments here, they would like to add them in transaction report instead of iou report.

Screenshot 2024-10-17 at 10 14 02

Furthermore, opening transaction report can make our implementation clearer since we're combining 2 reportActions, but just the actions from transaction thread are shown at the beginning -> Users should realize it's the transaction report

dukenv0307 commented 5 days ago

That makes sense to me, but I need to hear other thoughts @NikkiWines @shawnborton @dannymcclain

BTW, if we decide to open the thread transaction, we can fix many bugs related to this. For example, this one: https://github.com/Expensify/App/issues/49959 (I'm C+ on that) can be fixed with a minor change

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 5 days ago

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

shawnborton commented 5 days ago

We don't want to change anything about how the transaction or report is displayed. Let's just try to address the fact that the unread marker isn't showing up as it should. But perhaps I am not understanding correctly, so feel free to rephrase if needed :)

truph01 commented 5 days ago

Let's just try to address the fact that the unread marker isn't showing up as it should

That because we're showing the iou report, but the action is transaction report

thienlnam commented 4 days ago

Will let @NikkiWines chime in here, but I am not sure we want to change the view to just the transaction directly. I feel like the intention was to consolidate the one-expense chat experience but anything that happens there is still part of the "report"

shawnborton commented 4 days ago

Yeah, we definitely do not want to change the view. We need to fix the unread marker without changing anything about the view itself.

truph01 commented 4 days ago

@shawnborton No, I don't want to change the view itself, just change the reportID in the URL cc @NikkiWines

truph01 commented 4 days ago

As you can see in the below video, iou report (first report) has the same view as transaction report (second one). Currently we're showing iou report, so I would like to show transaction report and update the header to be the same as iouReport.

https://github.com/user-attachments/assets/8b4491f7-8662-434d-82a8-2b88d7b986d5

shawnborton commented 3 days ago

Curious what our engineers think about that. I'm mostly just chiming in from a design perspective that we need to keep the view looking exactly the same but add the ability to have the correct unread/new marker here.

dannymcclain commented 3 days ago

Curious what our engineers think about that. I'm mostly just chiming in from a design perspective that we need to keep the view looking exactly the same but add the ability to have the correct unread/new marker here.

+1. It would be great to get some engineering perspectives on this—I feel like there was a fair bit of discussion around how to handle the whole report thread vs transaction thread when we initially implemented this consolidated view.

NikkiWines commented 3 days ago

Furthermore, opening transaction report can make our implementation clearer since we're combining 2 reportActions, but just the actions from transaction thread are shown at the beginning -> Users should realize it's the transaction report

This was originally proposed as a flow for the one-transaction view when we first discussed how to implement it way back when, but ultimately the implementation went a different way and opted to combine the view on the IOU report level instead. I don't think we want to change this now (though I'm not opposed to revisiting the need for this view), and so I don't think changing the reportID is the correct route here.

The intended behavior based on the design brief is that we show the IOU report, and pull in parts of the transaction thread (the ability to select categories, displaying transaction thread report actions, etc.) for a more comprehensive view. However, the location of where those actions occur stay on their requesite reports. If the user changes the category, that's a transaction-level action and the reportAction is ultimately created on the transactionThread. If they comment on the IOU report (even while the view is combined for the one-transaction view) that comment should exist on the IOU report level.

To me, it sounds like we need to check the report.lastReadTime for both the IOU report and transaction thread report to determine if the unread indicator needs to be displayed on the combined view.