Page MenuHomePhabricator

[lib] Check the timestamps when updating read status
ClosedPublic

Authored by tomek on Sep 4 2024, 7:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 6:25 AM
Unknown Object (File)
Fri, Nov 22, 5:10 AM
Unknown Object (File)
Thu, Nov 21, 11:55 PM
Unknown Object (File)
Wed, Nov 13, 2:46 AM
Unknown Object (File)
Sun, Nov 10, 7:06 AM
Unknown Object (File)
Sat, Nov 9, 11:53 PM
Unknown Object (File)
Sat, Nov 9, 4:21 PM
Unknown Object (File)
Thu, Nov 7, 6:59 PM
Subscribers

Details

Summary

Check the timestamp before setting the value. We need to use a different update type so that we can update also the timestamps.

https://linear.app/comm/issue/ENG-9120/update-thread-unread-status-spec

Depends on D13241

Test Plan

Process a couple of operations setting to read / unread.

Diff Detail

Repository
rCOMM Comm
Branch
timestamps
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested review of this revision.Sep 4 2024, 7:45 AM
lib/shared/dm-ops/change-thread-read-status-spec.js
42

Why change update type?

lib/shared/dm-ops/change-thread-read-status-spec.js
29

So this type will never be used in DMs? Is it not possible to make this type accept timestamp change?

lib/shared/dm-ops/change-thread-read-status-spec.js
29

It is possible, but I'm not sure if it is a good idea. This update type also comes from keyservers and I don't like the idea of adding the timestamp as a property to such updates. Also, we're using UPDATE_THREAD in all the other specs, so it is consistent with them. Overall, I'm open to making this change, but I'm not sure if it is worth doing.

42

The previous type updates only the status and can't update any other property of the thread.

Don't override the timestamps

lib/shared/dm-ops/process-dm-ops.js
196 ↗(On Diff #43915)

This change should be made in the previous diff.

This revision is now accepted and ready to land.Sep 6 2024, 5:06 AM