App: [Hold for #15212][$1000] Add FailureData for NewDot MarkAsUnread command so that it can revert values correctly

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

Remove lastReadTime param from MarkAsUnread API. We need to intentionally break the command to easily test the failure data.
Open a chat report. Check its onyx report data for the lastReadTime.
Mark one of the messages as unread.

Expected Result:

The MarkAsUnread network request should fail.
The onyx report data should be the same in step 2 above and should not move backwards because the request failed.
There should be no new message marker line.

Actual Result:

The onyx report data is updated to a past value which is not in sync with the data in the backend.
There is a new message marker line.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

This snippet should help add the failure data, the tricky part is the new message marker line.

const oldLastReadTime = lodashGet(allReports, [reportID, 'lastReadTime']);

failureData: [{
    onyxMethod: CONST.ONYX.METHOD.MERGE,
    key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
    value: {
        lastReadTime: oldLastReadTime,
    },
}],

Platforms:

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

Android / native
Android / Chrome
iOS / native
iOS / Safari
MacOS / Chrome / Safari
MacOS / Desktop

Version Number: Reproducible in staging?: Reproducible in production?: 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 Notes/Photos/Videos: Any additional supporting documentation Expensify/Expensify Issue URL: Issue reported by: Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~010b9ff7fc1c469571
Upwork Job ID: 1626756025884848128
Last Price Increase: 2023-02-18

About this issue

Original URL
State: closed
Created a year ago
Comments: 27 (20 by maintainers)

Most upvoted comments

Re-assigning as I am heading out on sabbatical 👍

CortneyOfstad on Apr 14, 2023

Updated Proposal

@chiragsalian is right about the regression that the approach will cause

Here’s my updated approach for that second part: We’ll add a isLastReadTimeOptimistic: true props to the report when we’re marking the message as read, to indicate that the lastReadTime in the report is actually only optimistic and is being updated in the API.

For both success and failure case we’ll set isLastReadTimeOptimistic to false.

Then in componentDidUpdate of ReportActionView, we’ll always re-evaluate the newMarkerReportActionID value when the lastReadTime change and the isLastReadTimeOptimistic change:

if (prevProps.report.lastReadTime !== this.props.report.lastReadTime && !this.props.report.isLastReadTimeOptimistic && prevProps.report.isLastReadTimeOptimistic) {
            this.setState({newMarkerReportActionID: ReportUtils.getNewMarkerReportActionID(this.props.report, this.sortedAndFilteredReportActions)});
        }

This will still achieve the expected result and will not cause the regression mentioned by @chiragsalian.

(This approach is similar to the isOptimisticData prop of the report where we also have a prop marking if the report is only optimistic)

tienifr on Feb 22, 2023

Sorry, i don’t think that proposal is correct. I’m pretty sure our jest tests will fail too with those changes. Take this scenario,

UserB sends UserA a couple of messages.
When UserA clicks on the chat report they should see the new message line marker.

With the current changes, i think they would not see it which is incorrect. Feel free to correct me if I’m mistaken.

chiragsalian on Feb 21, 2023

I see the failure data is ready. For the marker line issue, maybe we can also put it in a tracker here?

bernhardoj on Feb 18, 2023