Reset session if pollworker card is pulled during paper load prompt/intake #4647

kshen0 · 2024-03-04T23:38:22Z

Overview

#3974

During either paper loading state, resets the voter session if poll worker auth is ended before paper is done loading.

If card is pulled before paper load is attempted, the app just returns to the login page
If card is pulled during physical paper load, the app displays a message for 5 seconds then resets

Previously, the code allowed a poll worker to pull their card either

before paper loading was attempted, resulting in a cardless voter session that would eventually fail at print time
in the middle of paper loading. In the event of a jam during paper load, the poll worker would be unaware, so we want the poll worker to remain until loading is successful.

Demo Video or Screenshot

IMG_4793.mov

Testing Plan

add backend tests
add frontend tests

Checklist

I have added logging where appropriate to any new user actions, system updates such as file reads or storage writes, or errors introduced. (logging is already covered by existing auth and state transition logging)

I have added a screenshot and/or video to this PR to demo the change
I have added the "user_facing_change" label to this PR to automate an announcement in #machine-product-updates

kshen0 · 2024-03-07T22:49:49Z

apps/mark-scan/backend/src/custom-paper-handler/state_machine.ts

@@ -43,8 +43,7 @@ import {
  DEVICE_STATUS_POLLING_INTERVAL_MS,
  DEVICE_STATUS_POLLING_TIMEOUT_MS,
  MAX_BALLOT_BOX_CAPACITY,
-  RESET_AFTER_JAM_DELAY_MS,
-  SUCCESS_NOTIFICATION_DURATION_MS,
+  NOTIFICATION_DURATION_MS,


This file's diff is mostly whitespace; recommend turning whitespace diff off to view

jonahkagan · 2024-03-11T17:14:57Z

apps/mark-scan/frontend/src/app_root.tsx

@@ -439,6 +440,18 @@ export function AppRoot({
    return <JamClearedPage stateMachineState={stateMachineState} />;
  }

+  const isInPaperLoadingStage =
+    stateMachineState === 'accepting_paper' ||
+    stateMachineState === 'loading_paper';


In the second video, it seemed a little weird to me that the "start voting" screen is shown while the paper is still being loaded and then if the poll worker removes their card, this error can occur which makes them have to start over. I feel like the "start voting" screen indicates that it's safe to remove the poll worker card. Maybe a "loading paper... please wait" screen would be helpful?

Going to hold off on technical review for now til we resolve the product discussion.

I think I forgot to replace the video - this check should fix it. I'll confirm and upload a new video

Cool, just let me know when you want me to take another look

Uploaded the new video and re-requested review 🙏

One more UX question: Is there a reason to have a delay and show a message when the poll worker card is removed before loading paper? Kind of seems like it could just cancel and go back to the Insert Card screen immediately

No strong reason - I kept them the same for consistency but we can also just end the session. The more I think about it, it's probably less confusing if we skip the message.

To be clear, are you ok with keeping the message for when the poll worker card is pulled during the physical paper load stage?

I'm ok with keeping it, though I'd be curious to see how it feels to you without it.

I think in that case, it'll be more surprising to the poll worker if we end the session. IMO we should leave the message in for the actual paper loading stage, or remove both the message and auth check (so a poll worker can pull their card once the paper has been grasped by the printer).

Of those 2 options, the former is safer, so inclined to leave it as is.

Sounds good to me!

kshen0 · 2024-03-18T23:26:04Z

Must have accidentally requested a review early - not sure how. @jonahkagan this is now ready for review

jonahkagan · 2024-03-19T16:39:35Z

apps/mark-scan/backend/src/custom-paper-handler/state_machine.ts

-                scannedImagePaths: undefined,
-                isPatDeviceConnected: false,
-              });
+            poll_worker_auth_ended_unexpectedly: {


What happens if the poll worker reinserts their card during this state or during the not_accepting_paper state that comes after?

In poll_worker_auth_ended_unexpectedly, nothing happens because we don't poll auth.

not_accepting_paper is the initial state so it starts a new voting session as normal.

jonahkagan · 2024-03-19T16:42:21Z

apps/mark-scan/backend/src/custom-paper-handler/state_machine.ts

+              entry: ['resetContext', 'endCardlessVoterAuth'],
+              after: {
+                // The frontend needs time to idle in this state so the user can read the status message
+                [NOTIFICATION_DURATION_MS]: 'not_accepting_paper',


Watching the video, it feels like the paper should be ejected while this message is showing, rather than after. Though I think it's totally fine to leave as is since this is likely an uncommon case.

Good point. It's a quick change so not a problem to add even if it's uncommon

8b2a91a

jonahkagan · 2024-03-19T16:46:45Z

apps/mark-scan/frontend/src/app_root.tsx

@@ -545,6 +561,9 @@ export function AppRoot({ reload }: Props): JSX.Element | null {
      }
    }

+    // Implicitly map resetting_state_machine_no_delay to return InsertCardScreen
+    // because that state implies we are returning to the starting (default) state.


Could this be made explicit (e.g. with an assert) or by masking resetting_state_machine_no_delay on the backend by returning a state that would go to the insert card screen?

jonahkagan · 2024-03-19T16:49:34Z

apps/mark-scan/frontend/src/app_root.tsx

+        // it means the state hasn't caught up to auth changes. We check that edge case here to avoid flicker ie.
+        // rendering the ballot briefly before rendering the correct "Insert Card" screen
+        stateMachineState !== 'accepting_paper'
+      ) {


I feel like we've run into these cases before where the backend and frontend have out of sync state. It's pretty confusing. Do you think we could make it more straightforward if the backend was solely responsible for deciding what state we should show and the frontend only rendered screens based on the state machine state (rather than also trying to incorporate auth state)?

Yeah I didn't foresee this issue when adding auth polling to the backend. I agree it's pretty awful. In my mind the fix is related to #3985, which would move the frontend to an exhaustive switch to handle all state machine states (and only states, no auth).

cc @arsalansufi who had a question about #3985 - for all states that involve auth, we'll need to patch like this.

Ah yeah, forgot about #3985. I agree that would resolve it.

Thanks for the ping / example! Does make the motivation for #3985 clear

jonahkagan

Thanks for working through the changes!

kshen0 changed the title ~~reset session if pollworker card is pulled during paper load~~ WIP reset session if pollworker card is pulled during paper load Mar 5, 2024

kshen0 force-pushed the kevin/reset-session-if-card-pulled-during-paper-load branch from e538abf to 1437ff2 Compare March 7, 2024 19:47

kshen0 changed the title ~~WIP reset session if pollworker card is pulled during paper load~~ Reset session if pollworker card is pulled during paper load prompt/intake Mar 7, 2024

kshen0 added the user_facing_change label Mar 7, 2024

kshen0 marked this pull request as ready for review March 7, 2024 22:48

kshen0 requested a review from jonahkagan March 7, 2024 22:48

kshen0 commented Mar 7, 2024

View reviewed changes

jonahkagan reviewed Mar 11, 2024

View reviewed changes

kshen0 added 2 commits March 14, 2024 17:28

add state for when poll worker auth is ended too early

a3113de

update references to changed const name

7e3220e

kshen0 force-pushed the kevin/reset-session-if-card-pulled-during-paper-load branch from 1437ff2 to ea91d36 Compare March 15, 2024 00:47

add frontend state

7143038

kshen0 force-pushed the kevin/reset-session-if-card-pulled-during-paper-load branch from ea91d36 to 7143038 Compare March 15, 2024 17:11

kshen0 requested a review from jonahkagan March 18, 2024 17:56

kshen0 added 4 commits March 18, 2024 13:36

return to previous screen if paper load hasn't been attempted

6d08701

reset auth and context

94a4f3e

handle case where state lags behind auth

3ba5c47

update tests

09ffd91

jonahkagan reviewed Mar 19, 2024

View reviewed changes

kshen0 added 2 commits March 19, 2024 11:00

eject paper to front as soon as poll worker card is pulled

8b2a91a

mask resetting_state_machine_no_delay

80b0193

kshen0 requested a review from jonahkagan March 19, 2024 18:58

jonahkagan approved these changes Mar 19, 2024

View reviewed changes

kshen0 merged commit a446811 into main Mar 19, 2024
57 checks passed

kshen0 deleted the kevin/reset-session-if-card-pulled-during-paper-load branch March 19, 2024 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset session if pollworker card is pulled during paper load prompt/intake #4647

Reset session if pollworker card is pulled during paper load prompt/intake #4647

kshen0 commented Mar 4, 2024 •

edited

Loading

kshen0 Mar 7, 2024

jonahkagan Mar 11, 2024

kshen0 Mar 15, 2024

jonahkagan Mar 18, 2024

kshen0 Mar 18, 2024

jonahkagan Mar 18, 2024

kshen0 Mar 18, 2024

jonahkagan Mar 18, 2024

kshen0 Mar 18, 2024

jonahkagan Mar 18, 2024

kshen0 commented Mar 18, 2024

jonahkagan Mar 19, 2024

kshen0 Mar 19, 2024

jonahkagan Mar 19, 2024

kshen0 Mar 19, 2024

jonahkagan Mar 19, 2024

kshen0 Mar 19, 2024

jonahkagan Mar 19, 2024

kshen0 Mar 19, 2024

jonahkagan Mar 19, 2024

arsalansufi Mar 20, 2024

jonahkagan left a comment

Reset session if pollworker card is pulled during paper load prompt/intake #4647

Reset session if pollworker card is pulled during paper load prompt/intake #4647

Conversation

kshen0 commented Mar 4, 2024 • edited Loading

Overview

Demo Video or Screenshot

Testing Plan

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshen0 commented Mar 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahkagan left a comment

Choose a reason for hiding this comment

kshen0 commented Mar 4, 2024 •

edited

Loading