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
F3005680: D13240.id43940.diff
Fri, Oct 18, 5:16 PM
F3005678: D13240.id43928.diff
Fri, Oct 18, 5:16 PM
F3005677: D13240.id43916.diff
Fri, Oct 18, 5:16 PM
F3005676: D13240.id43915.diff
Fri, Oct 18, 5:15 PM
F3005674: D13240.id43883.diff
Fri, Oct 18, 5:15 PM
Unknown Object (File)
Thu, Oct 17, 2:07 PM
Unknown Object (File)
Thu, Oct 10, 12:49 PM
Unknown Object (File)
Sun, Sep 29, 7:13 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