-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reorganize management of voter settings in Scan #5473
Conversation
} | ||
|
||
/** | ||
* useVoterSettingsControls adds Scan-specific settings to the base settings provided by VoterSettingsManagerContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really it aggregates three different contexts, will change comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to see both comments changed to describe what this piece of code actually does, which IMO is:
- saving and reapplying a voter's theme and language settings when their voting session is interrupted by an election official
- resetting the settings after a voter finishes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for exploring this refactor! I like the direction
useQueryChangeListener(scannerStatusQuery, { | ||
select: ({ state }) => state, | ||
onChange: (newState, previousState) => { | ||
// If we transition from paused to no_paper we are just returning from an election official screen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the paused
state is triggered whenever an election official logs in and ends whenever they log out, I think we can unite these two listeners to just use the paused
state instead of also checking auth. A slightly riskier change, so worth looking into whether there is good test coverage if we decide to pursue it.
} | ||
|
||
/** | ||
* useVoterSettingsControls adds Scan-specific settings to the base settings provided by VoterSettingsManagerContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to see both comments changed to describe what this piece of code actually does, which IMO is:
- saving and reapplying a voter's theme and language settings when their voting session is interrupted by an election official
- resetting the settings after a voter finishes
apps/scan/frontend/src/app_root.tsx
Outdated
@@ -228,6 +230,7 @@ export function AppRoot(): JSX.Element | null { | |||
return ( | |||
<PollWorkerScreen | |||
electionDefinition={electionDefinition} | |||
onPollsClose={() => voterSettingsControls.resetVoterSettings()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be clearer if PollWorkerScreen
used the useVoterSettingsControl
hook directly rather than passing in a generic onPollsClose
handler, since we don't really need a generic handler.
That being said, the way the hook is currently written, that will also create a side effect of registering the change listeners again. I'm not sure if that would cause problems or not, but I'm a bit concerned about double side effects. I think to protect against this we should separate out the creation of the API to control the voter settings (keeping this in the hook) from the change listeners that uses the API (putting these directly in AppRoot
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point, I agree with that. Moving the listeners out of the hook into AppRoot
feels right. I also didn't like that the hook wasn't reusable across components without re-registering listeners, so this addresses that, as it felt like this hook was owning too much functionalit. Will push a commit soon with these changes
a766fa1
to
620ea35
Compare
a944806
to
f8c585d
Compare
f8c585d
to
19d6f82
Compare
…agement in app, and test for resetting voter settings on polls close
19d6f82
to
fec3b4c
Compare
export interface SessionSettingsManagerProps { | ||
resetVoterSettings: () => void; | ||
cacheAndResetVoterSettings: () => void; | ||
restoreVoterSessionsSettings: () => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way to think about these that might be clearer:
startNewSession
pauseSession
resumeSession
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
apps/scan/frontend/src/app.test.tsx
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if there's a way to test the user-facing behavior we want rather than relying on mocking the hook. E.g. start a voter session, change the theme/language, insert a poll worker card, check that the theme/language are back to default, remove the card, check that the theme/language are back to the voter's choice.
If we could do that, we could even get rid of the tests for the hook. My general philosophy is to mock as little as possible to ensure that all the pieces are working together and to make the tests resilient to changes in implementation. I'll still do small unit tests if there are intricate behaviors in a function or subcomponent that will be easier to test there, but for main behavior I like to test at the highest level that covers the entirety of the behavior. Here's an article that I think lays out a compelling argument for this approach (except the point about coverage).
I'm also ok with what you've got, so gonna approve the PR, in case you'd rather move forward than rework these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the thoughtful point. I like that concept, though I feel like I need to write the tests and see what they look like to feel confident about it. My concern is that it might make tests too complicated, but I am very open to it because I do see how it could actually make it simpler to test the core flows that matter.
That being said, I think I will merge this PR and then work on subsequent PR that reworks these tests. Thinking that so 1) we can QA these changes on Scan because writing these tests will still take me some time (I was running into some roadblocks earlier when I tried to use less mocks), and I'd love to do it properly without being rushed so i can learn along the way and 2) since it will only be updating tests and not behavior, it seems okay to have the test rework PR not make the certification cut.
Let me know if something in that logic seems off to you, otherwise I'll move forward with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good to me!
Overview
Follow up to #5462. Thread conversation here
Main issue:
Our current approach of
VoterSettingsManager
being a component that reacts to side effects is not the best frontend practice.First, as Jonah points out in the thread linked above, we are having a user action (ex. closing polls) that could directly trigger a state change (ex. resetting settings), instead trigger a side effect. But, right now
<VoterSettingsManager>
manages state that is not exposed to the app and actually is created all the way in<App>
.Second, the idea of a component that has no UI but only manages state indicates that it may be a better fit as a custom hook, as hooks can encapsulate state.
So, this PR suggests converting the component into a hook, which is created in
App_root
and provides the needed state management function toPollWorkerScreen
to reset the settings.Note: One upside about out current approach is that we do have all resetting/restoring of settings due to log ins/outs in the same place, so it is easy to reason about. With this change, we are following best practices for directly triggering state updates rather than relying on side effects, but we are also no longer co-locating these updates.
Demo Video or Screenshot
Behavior should not change from this PR. The following video shows:
Screen.Recording.2024-10-08.at.3.00.33.PM.mov
Testing Plan
Will add tests if we decide this is an approach we want to take.
Checklist