Page MenuHomePhabricator
Differential D5513

[native] Introduce `StaffContextProvider`
ClosedPublic
Actions

Authored by atul on Oct 31 2022, 2:00 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 26, 5:23 PM
Unknown Object (File)
Tue, Nov 26, 5:23 PM
Unknown Object (File)
Tue, Nov 26, 5:23 PM
Unknown Object (File)
Mon, Nov 25, 10:25 AM
Unknown Object (File)
Mon, Nov 25, 8:30 AM
Unknown Object (File)
Mon, Nov 25, 3:50 AM
Unknown Object (File)
Mon, Nov 25, 3:50 AM
Unknown Object (File)
Mon, Nov 25, 3:50 AM
View All Files
Subscribers
• abosh

Details

Reviewers
tomek
ginsu
rohan
ashoat
marcin
Commits
rCOMM22ac9645d0eb: [native] Introduce `StaffContextProvider`
Summary

Context: https://linear.app/comm/issue/ENG-2134/have-a-flag-that-indicates-whether-the-build-is-a-staff-release

We want to maintain a staffUserHasBeenLoggedIn value so we know if a staff member is currently, or has previously logged in.

This provider will set staffUserHasBeenLoggedIn to true, but won't do the reverse. Once it's been set to true it will remain true indefinitely.

Test Plan
  1. Log value of provider
  2. Log into non-staff account
  3. Ensure that staffUserHasBeenLoggedIn is false
  4. Log into a staff account
  5. Ensure that staffUserHasBeenLoggedIn is true
  6. Log back into a non-staff account
  7. Ensure that staffUserHasBeenLoggedIn is still true

95828c.png (858×1 px, 361 KB)

Diff Detail

Repository
rCOMM Comm
Branch
arcpatch-D5513 (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul created this revision.Oct 31 2022, 2:00 PM
Herald added a subscriber: • abosh. · View Herald TranscriptOct 31 2022, 2:00 PM
Harbormaster completed remote builds in B13115: Diff 18004.Oct 31 2022, 2:14 PM
atul requested review of this revision.Oct 31 2022, 2:14 PM
atul added a comment.Oct 31 2022, 2:14 PM

Added @ashoat as blocking since he has context, feel free to resign if you don't think it makes sense to be first-pass reviewer

atul updated this revision to Diff 18005.Oct 31 2022, 2:24 PM

Add StaffContextProvider to root.react.js

No point in making this a separate diff

atul edited the test plan for this revision. (Show Details)Oct 31 2022, 2:30 PM
Harbormaster completed remote builds in B13116: Diff 18005.Oct 31 2022, 2:38 PM
ashoat accepted this revision.Nov 1 2022, 5:24 AM

Was thinking about this more, and I think it would useful if the "button" in LoggedOutModal actually showed an Alert when pressed (similar thing could be done in "Build info") that explained eg.:

Advanced logging enabled
Since a staff user was logged in earlier, advanced logging is still enabled. To disable, kill the app and reopen.

OR:

Advanced logging enabled
Since this is a debug build, advanced logged is enabled.

OR:

Advanced logging enabled
Since this is a beta build, advanced logged is enabled.

In order to figure out which message we should display, we'd want to know which of the scenarios apply. So perhaps we should expand the context here to include a bit more info?

Accepting since it's not a hard requirement and could be handled in a later diff.

This revision is now accepted and ready to land.Nov 1 2022, 5:24 AM
ashoat added a comment.EditedNov 1 2022, 5:25 AM

For the first message, if the user popped up the Alert from a logged-in state (BuildInfo) we'd actually want to say:

Advanced logging enabled
Since a staff user is logged in, advanced logging is enabled. To disable, log out and then kill the app and reopen.

ashoat mentioned this in D5514: [native] Add `staffUserHasBeenLoggedInIndicator` to `LoggedInModal`.Nov 1 2022, 5:28 AM
atul added a comment.EditedNov 1 2022, 10:38 AM
In D5513#163029, @ashoat wrote:

Was thinking about this more, and I think it would useful if the "button" in LoggedOutModal actually showed an Alert when pressed (similar thing could be done in "Build info") that explained eg.:

Advanced logging enabled
Since a staff user was logged in earlier, advanced logging is still enabled. To disable, kill the app and reopen.

OR:

Advanced logging enabled
Since this is a debug build, advanced logged is enabled.

OR:

Advanced logging enabled
Since this is a beta build, advanced logged is enabled.

In order to figure out which message we should display, we'd want to know which of the scenarios apply. So perhaps we should expand the context here to include a bit more info?

Accepting since it's not a hard requirement and could be handled in a later diff.

Thanks for speccing this out. I'll handle this after I add staffUserHasBeenLoggedInIndicator to the Build Info page of Profile tab.

Linear task: https://linear.app/comm/issue/ENG-2151/[native]-advanced-logging-modal

atul updated this revision to Diff 18014.Nov 1 2022, 12:59 PM
atul edited the test plan for this revision. (Show Details)

rebase before landing

This revision was landed with ongoing or failed builds.Nov 1 2022, 1:02 PM
Closed by commit rCOMM22ac9645d0eb: [native] Introduce `StaffContextProvider` (authored by atul). · Explain Why
This revision was automatically updated to reflect the committed changes.
atul added a commit: rCOMM22ac9645d0eb: [native] Introduce `StaffContextProvider`.
Harbormaster completed remote builds in B13123: Diff 18014.Nov 1 2022, 1:13 PM
inka mentioned this in D5530: [keyserver] Add a script for triggering socket crash loop.Nov 3 2022, 8:52 AM
• InkaSokolowska mentioned this in rCOMM50d12886e7e7: [keyserver] Add a script for triggering socket crash loop.Nov 4 2022, 7:10 AM

Revision Contents
Changeset List

PathSize
native/
57 lines
staff/
44 lines

Diff 18014

View Options

native/root.react.js

Loading...
View Options

native/staff/staff-context.provider.react.js

Loading...