Page MenuHomePhabricator

[keyserver] disable injecting redux state when policies are not accepted
ClosedPublic

Authored by kamil on Dec 29 2022, 4:26 AM.
Tags
None
Referenced Files
F1787133: D6094.id20857.diff
Sat, May 18, 8:23 PM
F1787132: D6094.id21355.diff
Sat, May 18, 8:23 PM
F1787131: D6094.id21351.diff
Sat, May 18, 8:23 PM
F1787130: D6094.id21144.diff
Sat, May 18, 8:23 PM
F1787129: D6094.id20325.diff
Sat, May 18, 8:23 PM
F1787128: D6094.id20333.diff
Sat, May 18, 8:23 PM
F1787115: D6094.id.diff
Sat, May 18, 8:23 PM
F1787107: D6094.diff
Sat, May 18, 8:17 PM
Subscribers

Details

Summary

Follow up to: https://phab.comm.dev/D5875#inline-39619, but even if data will not be sent and modal will be visible after refresh keyserver will inject data to html response - this change will prevent that.

Test Plan
  1. Logout user
  2. Set terms policy as not accepted for user in DB
  3. Login via web app
  4. Refresh - data should not be present (empty app under modal, no data in redux)
  5. After accepting data should be fetched properly.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Dec 29 2022, 5:29 AM
ashoat requested changes to this revision.Dec 29 2022, 9:37 AM
ashoat added inline comments.
keyserver/src/responders/website-responders.js
313 ↗(On Diff #20333)

You should start this promise way earlier, basically as soon as you can... otherwise it waits until other things complete (eg. assetInfoPromise) to start

319–347 ↗(On Diff #20333)

This is messy. Instead, can you follow the existing convention and just update entryStorePromise, threadStorePromise, etc. to await the same fetchNotAcknowledgedPolicies promise, and return the correct results? Eg. take a look at how messageStorePromise depends on threadInfoPromise

This revision now requires changes to proceed.Dec 29 2022, 9:37 AM
keyserver/src/responders/website-responders.js
357 ↗(On Diff #20857)

wondering if this shouldn't be a promise too to await this via Promise.all (not in line 345)

313 ↗(On Diff #20333)

Good call, thanks

ashoat requested changes to this revision.Jan 12 2023, 7:52 AM

You need to brush up on Promises and async / await. Try to take some time and understand how the code in this file is written, and WHY it is written that way

keyserver/src/responders/website-responders.js
172–176 ↗(On Diff #20857)

Why are you awaiting this sequentially?

180 ↗(On Diff #20857)

Why are you awaiting this sequentially?

202 ↗(On Diff #20857)

Why are you awaiting this sequentially?

218 ↗(On Diff #20857)

Why are you awaiting this sequentially?

345 ↗(On Diff #20857)

Why are you awaiting this sequentially?

This revision now requires changes to proceed.Jan 12 2023, 7:52 AM
This revision is now accepted and ready to land.Jan 20 2023, 10:50 AM