[web] processing ThreadStore ops on worker
ClosedPublic
Actions

Authored by kamil on Nov 22 2023, 4:20 PM.

Details

Reviewers

tomek
michal

Commits

rCOMMf9a9964d0af5: [web] processing ThreadStore ops on worker

Summary

Start processing ops on worker, the same API as on native.

Depends on D9952

Test Plan

Test if each operation works properly

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Nov 22 2023, 4:20 PM

kamil held this revision as a draft.

Herald added a subscriber: ashoat. · View Herald TranscriptNov 22 2023, 4:20 PM

kamil added a child revision: D9955: [web] refactor processing store ops to separate function.Nov 22 2023, 4:28 PM

Harbormaster completed remote builds in B24346: Diff 33514.Nov 22 2023, 5:06 PM

kamil published this revision for review.Nov 23 2023, 11:15 AM

kamil added inline comments.

web/database/utils/store.js
36 ↗	(On Diff #33514)	We need this ( `> 0`) to avoid overriding thread store for non-staff users (we start with putting thread data to DB only for staff, so for others, it will be an empty array and could override data).

kamil added inline comments.Nov 23 2023, 2:07 PM

web/redux/redux-setup.js
240 ↗	(On Diff #33514)	this line should be conditional depending on `canUseDatabaseOnWeb` introduced in D9948

tomek accepted this revision.Nov 24 2023, 9:17 AM

tomek added inline comments.

web/database/utils/store.js
36 ↗	(On Diff #33514)	It doesn't sound possible, but just to make sure: is it possible for a staff user to have `threads.length === 0` in the DB? Do we have a way of determining if a user is a staff other than checking the length?
web/redux/redux-setup.js
240 ↗	(On Diff #33514)	Is this something that will be added in this diff, or later in the stack?

This revision is now accepted and ready to land.Nov 24 2023, 9:17 AM

tomek mentioned this in D9959: [web] start using threads from database.Nov 24 2023, 11:55 AM

ashoat added inline comments.Nov 26 2023, 5:00 PM

web/database/utils/store.js
36 ↗	(On Diff #33514)	It might be worth considering a scenario where a staff user hasn't accepted updated policies when they load the web app. In that case, would `threads.length === 0` for that staff user? (Not sure if this is a concern or not, just figured it's relevant to @tomek's question)

add missing condition

kamil added inline comments.Nov 27 2023, 1:31 PM

web/database/utils/store.js
36 ↗	(On Diff #33514)	is it possible for a staff user to have `threads.length === 0` in the DB? Yes, this is the case pointed out by @ashoat. Do we have a way of determining if a user is a staff other than checking the length? It's not for determining if a user is a staff, it's for determining whether threads are in a table or not If threads are in DB, we read it and populate the store If threads are not in DB (length 0) we could have 4 scenarios: a. this is a non-staff user, we don't want to treat an empty array as a source of truth and override the state with empty threads, so we return a `null` b. this is a staff user, so it means threads are not migrated yet, we return `null` to fetch it from the keyserver (D9959) c. user has not acknowledged policies, so returning `null` is also okay, but doesn't change anything d. user doesn't have any threads, and if it's possible or not, in the worst case it will ask the keyserver for threads because logic from D9959 will think it's case b, and keyserver will return an empty object again, but this is a really corner case, as right now each user has at least one thread. so I think it's safe to keep this condition with `length > 0`
36 ↗	(On Diff #33514)	It might be worth considering a scenario where a staff user hasn't accepted updated policies when they load the web app. In that case, would `threads.length === 0` for that staff user? (Not sure if this is a concern or not, just figured it's relevant to @tomek's question) That case applies to only changed policies when the user is logged out, when the user logs in thread length is 0 anyway, so this change has no influence on policies logic.
web/redux/redux-setup.js
240 ↗	(On Diff #33514)	In this diff, I just discovered I missed this change while creating a diff and made a note to myself and the reviewers. Adding this right now