Page MenuHomePhabricator

[web] policy acknowledgment handler
ClosedPublic

Authored by kamil on Dec 6 2022, 6:53 AM.
Tags
None
Referenced Files
F3489834: D5830.diff
Wed, Dec 18, 1:49 PM
Unknown Object (File)
Sun, Dec 15, 10:18 PM
Unknown Object (File)
Sun, Dec 15, 8:30 PM
Unknown Object (File)
Sun, Dec 15, 8:30 PM
Unknown Object (File)
Sun, Dec 15, 8:30 PM
Unknown Object (File)
Sun, Dec 15, 8:30 PM
Unknown Object (File)
Sun, Dec 15, 8:30 PM
Unknown Object (File)
Sun, Dec 15, 8:30 PM
Subscribers

Details

Summary

Code responsible for showing/hiding modal with policy acceptance. Currently, only with Terms of Use & Privacy because it's the only police we have right now.

Test Plan
  1. Log in to web app
  2. Change confirmed field for logged user in policy table
  3. Modal should appear (not immediately but after any request will be sent to keyserver)
  4. I accept should close modal and update value on keyserver db
  5. User should not be able to close modal using any method

Diff Detail

Repository
rCOMM Comm
Branch
policy-stuff-6
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Dec 6 2022, 8:10 AM
tomek requested changes to this revision.Dec 9 2022, 10:41 AM
tomek added inline comments.
web/redux/policy-acknowledgment-handler.js
12 ↗(On Diff #19184)

Maybe rename this

12 ↗(On Diff #19184)

It would be great if we could avoid duplicating the state. Now we're tracking it here and also in modal context - policyModalPushed is true if and only if the context contains TermsAndPrivacyModal. Maybe we can somehow use that state instead? (it might be challenging though)

13 ↗(On Diff #19184)

Does it sound better?

19–22 ↗(On Diff #19184)

Could you explain why we're hiding the modal in this case? Does !tosAndPrivacyState mean that we don't have to accept anything?

This revision now requires changes to proceed.Dec 9 2022, 10:41 AM
web/redux/policy-acknowledgment-handler.js
13 ↗(On Diff #19184)

We're only using needPolicyAcceptance?.[policyTypes.tosAndPrivacyPolicy] so we can make this more efficient by returning exactly this from selector. By doing that we can avoid unnecessary renders.

use ModalContext as information wether modal is visible, improve code

web/redux/policy-acknowledgment-handler.js
12 ↗(On Diff #19184)

Done

19–22 ↗(On Diff #19184)

I was thinking about a situation when we have no information in store about policies (that's why !tosAndPrivacyState - which is not the same as !isAcknowledged) and somehow modal is visible but it looks like this is not a possible case so removing those lines

tomek added inline comments.
web/redux/policy-acknowledgment-handler.js
15 ↗(On Diff #19322)

I think we should avoid using special strings (even empty) and instead use something more appropriate to express the absence of key.

19 ↗(On Diff #19322)

Instead of using find and converting it to boolean, you can use some

This revision is now accepted and ready to land.Dec 14 2022, 5:45 AM

use some, remove special string

fix: move href update to proper diff