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

[chiragsalian]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

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

Expected Result:

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

Actual Result:

1. The onyx report data is updated to a past value which is not in sync with the data in the backend.
2. 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

[MelvinBot]

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

[MelvinBot]

Triggered auto assignment to @CortneyOfstad (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[MelvinBot]

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

[MelvinBot]

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

[bernhardoj]

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

[tienifr]

Proposal

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

MarkAsUnread command is not reverting the values correctly when it fails.

What is the root cause of that problem?

The onyx report data is updated to a past value which is not in sync with the data in the backend.

This part is because currently we don't have the failureData set for the MarkAsUnread command

There is a new message marker line.

This is because in this line, when lastReadTime changes on componentDidUpdate, we're only updating the newMarkerReportActionID if the report is currently unread.

App/src/pages/home/report/ReportActionsView.js

Line 222 in d690ee2

&& ReportUtils.isUnread(this.props.report);

This will work for the case of a report comment has been manually "marked as unread" but will fail if the lastReadTime is updated due to the MarkAsUnread command failure.

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

The onyx report data is updated to a past value which is not in sync with the data in the backend.

For this issue, the failure data can be added as suggested in the issue description

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

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

can be added to

App/src/libs/actions/Report.js

Line 690 in d690ee2

function markCommentAsUnread(reportID, reportActionCreated) {

There is a new message marker line.

For this issue

App/src/pages/home/report/ReportActionsView.js

Line 222 in d690ee2

&& ReportUtils.isUnread(this.props.report);

In this line we need to update the condition for this MarkAsUnread command failure case. The condition can be updated to (ReportUtils.isUnread(this.props.report) || ReportUtils.isUnread(prevProps.report)).

When we revert the lastReadTime to the previous value due to the command failure, the this.props.report will be read, the prevProps.report will be unread so the condition above will work for this case.

What alternative solutions did you explore? (Optional)

We can remove this condition

App/src/pages/home/report/ReportActionsView.js

Line 222 in d690ee2

&& ReportUtils.isUnread(this.props.report);

and always update the newMarkerReportActionID if the lastReadTime changes.

Result

Working well after the fix

Screen.Recording.2023-02-18.at.11.11.36.mov

[CortneyOfstad]

@madmax330 how does the above proposal look? Any thoughts?

[madmax330]

Looks good to me @tienifr

[mollfpr]

@madmax330 I have the same thought. Let’s move forward with the @tienifr proposal!

[chiragsalian]

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,

1. UserB sends UserA a couple of messages.
2. 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.

[madmax330]

@chiragsalian I'm not following are you disagreeing with this part

In this line we need to update the condition for this MarkAsUnread command failure case. The condition can be updated to (ReportUtils.isUnread(this.props.report) || ReportUtils.isUnread(prevProps.report)).
When we revert the lastReadTime to the previous value due to the command failure, the this.props.report will be read, the prevProps.report will be unread so the condition above will work for this case.

Or setting the failureData to the old last read timestamp?

Also I don't understand why there wouldn't be a new message line marker if the lastReadTime is further in the past than it was. Or are you saying this has some side effects on other flows?

[tienifr]

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)

[chiragsalian]

I'm not following are you disagreeing with this part

Yes I'm diasgreeing with the logic you linked there i.e.,
(ReportUtils.isUnread(this.props.report) || ReportUtils.isUnread(prevProps.report))

I'm not disagreeing with the failureData. I provided that after all 😛

Also I don't understand why there wouldn't be a new message line marker if the lastReadTime is further in the past than it was. Or are you saying this has some side effects on other flows?

I'm sure i follow your question here. lastReadTime usually moves ahead. So in the issue here failureData will move lastReadTime ahead too. The optimisticData moves it to the past but the issue is moving it to the present has problems with the new message marker because,

1. When failureData is applied and lastReadTime is set to the present we don't want new message marker.
2. But when the user receives messages and clicks into the chat report, even though lastReadtime is set to the present we want the new message marker.

Hope that helps. Let me know if you have any questions with that🙂
In the first proposal suggested, step 2 of what i wrote here "But when the user receives messages.." false and hence its a regression.

To be honest, when I wrote this issue having a new key like isLastReadTimeOptimistic was the only solution I could think of but I'm personally not a fan of it. I am hoping to find another solution not dependent on adding a new key. Especially because there is no flow clearing the key and it's mostly useless except for an extremely short time frame for this one case. I'm also wondering if we would change how marker message functionality works over here and if so then maybe an easier solution would be possible. I've asked in that issue too.

[bondydaa]

going to close this out in favor of centralizing discussion on #15212 since we're going to try and solve all this more holistically with a design doc

[bondydaa]

whoops sorry I jumped the gun on closing, leaving open and on hold for now and dropping priority to monthly

[MelvinBot]

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

[MelvinBot]

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

[madmax330]

Still holding

[Christinadobrzyn]

Still on hold for #15212

[Christinadobrzyn]

Still on hold for #15212

[madmax330]

Still on hold

[madmax330]

Same

[madmax330]

Going to close this. If after the markers are done being fixed it still happens I'm sure someone will open an issue