Update report_updateLastRead to not set the sequenceNumber backwards unless specifically told to by the user

[johnmlee101]

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

---

Stale update requests are causing people's messages to be marked as unread on accident. This change will prevent stale requests from doing that, while still letting markAsUnread work as intended

Changes:

1. Add a new param setByUser
2. Add a check that prevents the new lastReadSequenceNumber from being less than the previous lastReadSequenceNumber
3. If setByUser is passed, then we ignore that check and allow it to be set to whatever the user chose

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @muttmuure (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[johnmlee101]

cc @kidroca does this look fine for the requirements?

[kidroca]

@johnmlee101 looks good 👍

[melvin-bot]

Triggered auto assignment to @stitesExpensify (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[stitesExpensify]

Maybe I'm confusing myself but after looking at this for a while but:

1. What is different about either of these cases?
2. How do these differ from updateLastRead?

updateLastRead just sets the current sequenceNumber read on the report so:

Mark everything up to this comment as read and if it's read anyway it'll just do nothing

To accomplish this we set the lastRead sequenceNumber to the sequenceNumber of that comment which can be accomplished by updateLastRead

Mark this comment as unread then and only then it would mark everything past that point as unread

This is the same thing right? We just set the lastRead sequenceNumber to that comment which makes everything past it unread

[kidroca]

It's way too easy to make a mistake and mark messages as unread the way updateLastRead works currently

The aim for this change is to avoid auto setting possibly read messages as unread, and instead have a specific action that would only be used after user deliberately marks an older message as unread

The linked comment and the discussion points out such cases

The discussion and overview of how this helps is from #3981 (comment)

[stitesExpensify]

Gotcha. So I don't think we need new API commands based on that comment. Like I mentioned above the functionality of both could be done with the current updateLastRead command. All it does is take a sequenceNumber and set that as the lastRead.

What do you think of doing this update to updateLastRead instead:

1. Add a new param setByUser
2. Add a check that prevents the new lastReadSequenceNumber from being less than the previous lastReadSequenceNumber
3. If setByUser is passed, then we ignore that check and allow it to be set to whatever the user chose

Thoughts?

[kidroca]

What do you think of doing this update to updateLastRead instead:

1. Add a new param setByUser
2. Add a check that prevents the new lastReadSequenceNumber from being less than the previous lastReadSequenceNumber
3. If setByUser is passed, then we ignore that check and allow it to be set to whatever the user chose

I remember it was proposed to create a new action or add a parameter to the existing, like you say, but I don't remember why we decided to go with separate action

Maybe the intent is clearer

• markAsUnread -> marks everything since the sequence number as unread
• markAsRead -> marks everything before the sequence number as read

Whereas updateLastRead doesn't sound like something that would result in unread messages, though it works more like markAsUnread - everything since the provided seq number would be considered unread

[johnmlee101]

After talking to @stitesExpensify 1:1 we decided that having 2 API commands isn't strictly necessary. One flow is a soft-set of the unread/read marker pivot point, and the other is a force set. If the user is marking manually, that will force the sequence number no matter what, while if it isn't manually set it will do a soft check to make sure its not setting below the existing sequenceNumber.

Given that, I will be putting this on hold as we'll be pursuing a potential high-level refactor of this logic to simplify and inevitably fix these issues.

[stitesExpensify]

After thinking about this some more and chatting with @marcaaron I decided that this is a very easy change that should significantly improve the experience WRT stalled requests. PRs are up

[stitesExpensify]

Not done yet

[stitesExpensify]

App PR is on staging, waiting for it to go to prod before we merge the web-e PR

[stitesExpensify]

Web PR is about to be on prod

[stitesExpensify]

Web PR is on prod, unsure why it didn't get posted here, but that means this is done!