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)
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
Unknown Object (File)
Fri, Nov 1, 6:07 AM
Unknown Object (File)
Fri, Nov 1, 5:35 AM
Unknown Object (File)
Oct 22 2024, 6:29 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
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 ↗(On Diff #43883)

Why change update type?

lib/shared/dm-ops/change-thread-read-status-spec.js
29 ↗(On Diff #43883)

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 ↗(On Diff #43883)

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 ↗(On Diff #43883)

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