Page MenuHomePhabricator
Differential D13240

[lib] Check the timestamps when updating read status
ClosedPublic
Actions

Authored by tomek on Sep 4 2024, 7:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 12, 6:03 AM
Unknown Object (File)
Tue, Mar 11, 9:30 AM
Unknown Object (File)
Feb 22 2025, 6:32 AM
Unknown Object (File)
Feb 17 2025, 11:11 PM
Unknown Object (File)
Feb 17 2025, 11:11 PM
Unknown Object (File)
Feb 17 2025, 11:11 PM
Unknown Object (File)
Feb 17 2025, 11:11 PM
Unknown Object (File)
Feb 17 2025, 11:11 PM
View All 87 Files
Subscribers
ashoat

Details

Reviewers
kamil
inka
marcin
Commits
rCOMM694c0441f6ba: [lib] Check the timestamps when updating read status
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 created this revision.Sep 4 2024, 7:28 AM
Herald added a subscriber: ashoat. · View Herald TranscriptSep 4 2024, 7:28 AM
Harbormaster completed remote builds in B31428: Diff 43883.Sep 4 2024, 7:45 AM
tomek requested review of this revision.Sep 4 2024, 7:45 AM
marcin added inline comments.Sep 5 2024, 12:49 AM
lib/shared/dm-ops/change-thread-read-status-spec.js
42 ↗(On Diff #43883)

Why change update type?

inka added inline comments.Sep 5 2024, 12:59 AM
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?

tomek edited the summary of this revision. (Show Details)Sep 5 2024, 3:11 AM
tomek added a parent revision: D13241: [lib] Check the timestamps when updating read status for all the specs.
tomek removed a parent revision: D13234: [lib] Check the timestamps when updating thread subscription.
tomek added inline comments.Sep 6 2024, 4:26 AM
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.

tomek updated this revision to Diff 43915.Sep 6 2024, 4:42 AM

Don't override the timestamps

tomek added inline comments.Sep 6 2024, 4:46 AM
lib/shared/dm-ops/process-dm-ops.js
196 ↗(On Diff #43915)

This change should be made in the previous diff.

Harbormaster failed remote builds in B31450: Diff 43915!Sep 6 2024, 4:57 AM
inka accepted this revision.Sep 6 2024, 5:06 AM
This revision is now accepted and ready to land.Sep 6 2024, 5:06 AM
tomek updated this revision to Diff 43916.Sep 6 2024, 5:22 AM

Roll back change

Harbormaster failed remote builds in B31451: Diff 43916!Sep 6 2024, 5:35 AM
tomek updated this revision to Diff 43928.Sep 6 2024, 7:28 AM

Rebase

Harbormaster completed remote builds in B31462: Diff 43928.Sep 6 2024, 7:55 AM
Closed by commit rCOMM694c0441f6ba: [lib] Check the timestamps when updating read status (authored by tomek). · Explain WhySep 6 2024, 8:10 AM
This revision was automatically updated to reflect the committed changes.
tomek added a commit: rCOMM694c0441f6ba: [lib] Check the timestamps when updating read status.

Revision Contents
Changeset List

PathSize
lib/
shared/
dm-ops/
31 lines

Diff 43916

View Options

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

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines