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.36k stars 2.78k forks source link

[$250] Android - Chat - If we alter transaction details, new message green line disappears #41211

Closed lanitochka17 closed 2 months ago

lanitochka17 commented 5 months 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: 1.4.67 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4522562 Issue reported by: Applause - Internal Team

Action Performed:

  1. Launch app
  2. Tap on a report with more than one expense
  3. Tap on a expense
  4. Enter merchant and save it
  5. Mark as unread the system message displayed for entering merchant
  6. Note green line with new message tag above system message
  7. Now edit the merchant and save it
  8. Note green line with new message tag above system message disappears

Expected Result:

In transaction details page, after mark as unread, if we alter transaction details, new message green line must not disappear

Actual Result:

In transaction details page, after mark as unread, if we alter transaction details, new message green line disappears

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/f65f916e-15b8-4346-b18b-16895f7e29c1

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0152b49882e3aa4a66
  • Upwork Job ID: 1792850190027993088
  • Last Price Increase: 2024-06-04
  • Automatic offers:
    • tienifr | Contributor | 102600483
Issue OwnerCurrent Issue Owner: @rushatgabhane
melvin-bot[bot] commented 5 months ago

Triggered auto assignment to @dylanexpensify (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 5 months ago

@dylanexpensify 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 bug and can be handled by external contributors

dylanexpensify commented 5 months ago

Reviewing today!

melvin-bot[bot] commented 4 months ago

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

dylanexpensify commented 4 months ago

Trying again today

melvin-bot[bot] commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

@dylanexpensify 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!

dylanexpensify commented 4 months ago

Couldn't repro. @lanitochka17 can you confirm if you can?

lanitochka17 commented 4 months ago

@dylanexpensify Issue is still reproducible

https://github.com/Expensify/App/assets/78819774/df87d184-d5a9-4bfa-9d31-157ab7e8de19

melvin-bot[bot] commented 4 months ago

@dylanexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

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

dylanexpensify commented 4 months ago

made external!

tienifr commented 4 months ago

Proposal

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

  1. In transaction details page, after mark as unread, if we alter transaction details, new message green line disappears
  2. When altering transaction details in offline mode, the transaction is not ordered on top, it stays in the same order until we come offline and the back-end response comes

What is the root cause of that problem?

If we look at other places that add report action to the report like here, we'll see that the lastVisibleActionCreated and lastReadTime of the report will be set to currentTime (also the same time of the added optimistic report action), because lastVisibleActionCreated should be of the added optimistic report action, and since the user is in the report and adding the action, they must be reading that created report action right then, hence the lastReadTime update. The response from the back-end is also the same.

However, when editing the transaction details, when adding the modified expense report action here, we currently don't set the lastVisibleActionCreated and lastReadTime of the transaction thread report to the time of the newly created modified expense report action. The back-end response for this case is also incorrect (the lastVisibleActionCreated and lastReadTime returned from the back-end is not equal).

This causes the isUnread here to be true (which is wrong because the user already read that modified expense report action), causing readNewestAction to be run and the New message marker is cleared.

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

  1. In https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/libs/actions/IOU.ts#L2415 Set the lastVisibleActionCreated and lastReadTime to the current time/or optimistically added report action's time

    optimisticData.push({
    onyxMethod: Onyx.METHOD.MERGE,
    key: `${ONYXKEYS.COLLECTION.REPORT}${transactionThread?.reportID}`,
    value: {
        lastVisibleActionCreated: updatedReportAction.created,
        lastReadTime: updatedReportAction.created,
    },
    })

    (of course we'll need to reset the data in failureData)

  2. Optionally, fix the back-end to return the same (lastVisibleActionCreated and lastReadTime should be the same and equal to the modified expense's report action created time)

What alternative solutions did you explore? (Optional)

Use currentTime instead of updatedReportAction.created for the optimistic data update, and optionally can add other fields like lastMessageText to the report too

dylanexpensify commented 4 months ago

@rushatgabhane to review!

rushatgabhane commented 4 months ago

@dylanexpensify @tienifr why is this a bug? after editing transaction details, we come back to the IOU page and it clears the unread marker because we're on the page.

What do you think?

rushatgabhane commented 4 months ago

I think we can close this issue

tienifr commented 4 months ago

@dylanexpensify @tienifr why is this a bug?

@rushatgabhane How the unread marker works is, it will only disappear if you navigate to another report and back, or if you close the browser tab and open it again.

Please try attempting to edit the transaction details, but actually do not save and just dismiss the modal, you'll see that the unread marker stays there.

it clears the unread marker because we're on the page.

We're back on the page, but the marker is still there (as it should be, both when you actually did the edit and you just close the modal and did not do any edit)

Also, there's a second bug due to the same root cause (2 in my proposal), that's noticeable and definitely a bug.

melvin-bot[bot] commented 4 months ago

@rushatgabhane @dylanexpensify this issue is now 4 weeks old, please consider:

Thanks!

melvin-bot[bot] commented 4 months ago

@rushatgabhane, @dylanexpensify Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 4 months ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

dylanexpensify commented 4 months ago

@rushatgabhane thoughts on this?

melvin-bot[bot] commented 4 months ago

@rushatgabhane, @dylanexpensify 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

melvin-bot[bot] commented 4 months ago

@rushatgabhane, @dylanexpensify Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

melvin-bot[bot] commented 3 months ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

melvin-bot[bot] commented 3 months ago

@rushatgabhane, @dylanexpensify 12 days overdue now... This issue's end is nigh!

rushatgabhane commented 3 months ago

okay that makes sense

rushatgabhane commented 3 months ago

https://github.com/Expensify/App/issues/41211#issuecomment-2122370121

@tienifr 's proposal looks good πŸŽ€πŸ‘€πŸŽ€

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

πŸ“£ @tienifr πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

tienifr commented 3 months ago

@rushatgabhane PR https://github.com/Expensify/App/pull/43162 is ready

dylanexpensify commented 3 months ago

Ongoing!

melvin-bot[bot] commented 3 months ago

This issue has not been updated in over 15 days. @rushatgabhane, @srikarparsi, @dylanexpensify, @tienifr 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!

rushatgabhane commented 2 months ago

PR on production

rushatgabhane commented 2 months ago

PR on production last week. Payment due today.

cc: @dylanexpensify could you please attach payment summary πŸ™‡

JmillsExpensify commented 2 months ago

Do we need a regression test for this one?

rushatgabhane commented 2 months ago

This was caught by QA and is a very minor bug. So I think not needed.

But in case you disagree -

  1. Tap on a report with more than one expense
  2. Tap on a expense
  3. Enter merchant and save it
  4. Mark as unread the system message displayed for entering merchant
  5. Note green line with new message tag above system message
  6. Now edit the merchant and save it
  7. Verify the green line with new message tag above system message does not disappear
JmillsExpensify commented 2 months ago

Bump on that last comment

mallenexpensify commented 2 months ago

Contributor: @tienifr due $250 via NewDot Contributor+: @rushatgabhane due $250 via NewDot

TestRail GH (I prefer to create them then let QA decide what to do with them, if anything (cuz we have the option to check monthly for edge cases))

JmillsExpensify commented 2 months ago

$250 approved for @rushatgabhane

JmillsExpensify commented 1 month ago

$250 approved for @tienifr