Page MenuHomePhabricator

[lib] DMOperationSpec for leave thread operation
ClosedPublic

Authored by tomek on Jul 24 2024, 10:12 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Sep 11, 6:58 AM
Unknown Object (File)
Wed, Sep 11, 2:41 AM
Unknown Object (File)
Wed, Sep 11, 2:41 AM
Unknown Object (File)
Tue, Sep 10, 9:08 PM
Unknown Object (File)
Tue, Sep 10, 6:59 PM
Unknown Object (File)
Mon, Sep 9, 7:41 AM
Unknown Object (File)
Mon, Sep 9, 7:22 AM
Unknown Object (File)
Mon, Sep 9, 7:01 AM
Subscribers

Details

Summary
Test Plan

Just the Flow. Further testing will be performed after it is connected with the rest of the app.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 24 2024, 10:40 AM
Harbormaster failed remote builds in B30647: Diff 42743!
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 25 2024, 8:59 AM
Harbormaster failed remote builds in B30670: Diff 42774!
tomek requested review of this revision.Jul 26 2024, 3:20 AM
lib/shared/dm-ops/leave-thread-spec.js
33–46 ↗(On Diff #42806)

Users can see sidebars they don't belong to - in chats they belong to. They should still record messages for those sidebars.
We should instead check if the thread is present in the users store.

50–55 ↗(On Diff #42806)

Is this is correct for sidebars?

68 ↗(On Diff #42806)

Should we set the unread status?

Delete sidebar only if it doesn't have a parent

tomek added inline comments.
lib/shared/dm-ops/leave-thread-spec.js
44–50 ↗(On Diff #42898)

When we leave a thread we should also leave its sidebars. But it can happen that we don't have all the sidebars in our store while leaving the thread.

One solution could be to iterate through every sidebar here and leave them one by one, but it is still a fragile solution. An alternative is to leave only the thread from the operation. Then, someone who generates operations is responsible for generating all the necessary ones.

47–49 ↗(On Diff #42898)

We're deleting only non-sidebar threads and sidebars without a parent. If a sidebar doesn't have a parent, it means that its parent was deleted in the past.

50–55 ↗(On Diff #42806)

Updated the logic

inka added inline comments.
lib/shared/dm-ops/leave-thread-spec.js
44–50 ↗(On Diff #42898)

Do we have this noted somewhere on linear?

This revision is now accepted and ready to land.Jul 31 2024, 2:06 AM
lib/shared/dm-ops/leave-thread-spec.js
44–50 ↗(On Diff #42898)

But it can happen that we don't have all the sidebars in our store while leaving the thread.

In what scenarios might this occur? Would it occur because the user joined the parent chat after the sidebar was created? Or something else?

lib/shared/dm-ops/leave-thread-spec.js
44–50 ↗(On Diff #42898)

This could happen when an update about creating a sidebar was received after the user tried to leave the thread.

lib/shared/dm-ops/leave-thread-spec.js
44–50 ↗(On Diff #42898)

Got it. It seems like in all of these cases (the one I proposed and the one you proposed), it is safe to discard the update