Kevin/vxmarkscan warn when interpretation fails #3981

kshen0 · 2023-09-18T20:49:34Z

Overview

Adds a flow for when the ballot interpretation is BlankPageInterpretation. Barring hardware failures, we expect this to happen only when the thermal paper is inserted upside down.

Demo Video or Screenshot

In this video I load non-thermal paper first, then load thermal paper in the correct orientation second.

IMG_4256.MOV

Known issues in this video:

"Printing Ballot" screen shows when ballot is being cast "Printing ballot" screen shows during ballot cast #3945
Validation screen says "You may still vote in this contest" for contest with no votes. We should hide that copy because this suggests the voter can still edit their selections by pressing a contest card, which doesn't work. They have to press the "My Ballot is Incorrect" button and enter the ballot spoil flow. (Github Issues is down; I'll file this ticket once it's back up).

Testing Plan

Added tests for new frontend state

Checklist

I have added logging where appropriate to any new user actions, system updates such as file reads or storage writes, or errors introduced.

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 · 2023-09-19T22:21:39Z

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

+            },
+            on: {
+              PAPER_PARKED: 'done',
+              NO_PAPER_ANYWHERE: 'accepting_paper',


Handle case of user pulling paper out of the infeed before it's fully loaded. Jammed paper is handled by top-level on: { PAPER_JAM: 'jammed' } transition

kshen0 · 2023-09-19T22:53:29Z

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

+  };
+}
+
+function pollAuthStatus(): InvokeConfig<Context, PaperHandlerStatusEvent> {


I don't love adding more polling, but this seemed cleaner than expecting the frontend to "watch" auth changes and change the backend state machine state via useEffect and mutation

Agree that polling on the backend is cleaner

kshen0 · 2023-09-19T22:58:43Z

apps/mark-scan/frontend/src/pages/poll_worker_screen.tsx

+    pollWorkerAuth.cardlessVoterUser &&
+    // It's expected there are votes in app state if the state machine reports a blank page after printing.
+    // The paper was likely inserted upside down so the solution is to reload paper correctly and go back to
+    // the voting screen


I don't like leaving super specific comments like this that might diverge from implementation, but I think it's important to explain why we're ok with breaking a previous constraint (that you can't enter a voting session with existing frontend ballot state)

jonahkagan · 2023-09-20T15:59:51Z

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

+      timeout({
+        each: PAPER_HANDLER_STATUS_POLLING_TIMEOUT_MS,
+        with: () => throwError(() => new Error('paper_status_timed_out')),
+      })


Should these be updated for auth status?

jonahkagan · 2023-09-20T16:01:02Z

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

+  };
+}
+
+function pollAuthStatus(): InvokeConfig<Context, PaperHandlerStatusEvent> {


Agree that polling on the backend is cleaner

jonahkagan · 2023-09-20T16:05:59Z

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

+            cond: (context) =>
+              context.interpretation
+                ? context.interpretation[0].interpretation.type === 'BlankPage'
+                : false,


What happens if there is no interpretation? It seems like both conds would be false and we'd get stuck in this state. Seems like maybe we expect context.interpretation to always be defined, in which case I think an assert would be more clear

I was thinking with the assert that you could remove the conditional check. So something like this:

assert(context.interpretation) return context.interpretation[0].interpretation.type === 'BlankPage`

or even

assertDefined(context.interpretation)[0].interpretation.type === 'BlankPage'

Sorry I wasn't clear enough!

Oops sorry your original comment was clear, just rushed the fix on my part. 700b335 uses assertDefined and removes the conditional check.

It also adds an assertion on interpretation type in entry. It's redundant with the check in the cond transitions but

it's valuable to throw on unexpected interpretation types. Without this assertion we'd get stuck in transition_interpretation

Typescript isn't aware that entry is guaranteed to precede cond transitions so we need to re-assert with assertDefined

jonahkagan · 2023-09-20T16:08:31Z

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

+      blank_page_interpretation: {
+        invoke: pollPaperStatus(),
+        entry: async (context) => {
+          await context.driver.presentPaper();


Do we need to call presentPaper here if we also call it in presenting_paper?

jonahkagan · 2023-09-20T16:10:09Z

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

+              NO_PAPER_ANYWHERE: 'accepting_paper',
+            },
+          },
+          done: {


Could probably remove this state by adding the assign action contained here to the transition from load_paper

Agree the assign call can go anywhere, but the state machine breaks when I get rid of the done state and transitioning from load_paper: https://app.circleci.com/pipelines/github/votingworks/vxsuite/12078/workflows/b8f38ca0-e4cd-4e24-bc63-48b103b367a0/jobs/400580

It's expecting the target state to be a sibling of load_paper ie. a child of blank_page_interpretation unless we transition from the done state. When transitioning from done it can successfully transition to top-level states.

jonahkagan · 2023-09-20T16:10:50Z

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

+        entry: () => {
+          debug('Placeholder paper_reloaded');
+        },


Remove debug?

Sorry, I think I cleaned this up but accidentally overwrote with a git reset before committing.

jonahkagan · 2023-09-20T16:12:31Z

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

+        case state.matches('transition_interpretation'):
+          return 'transition_interpretation';


Is this a state the frontend needs to know about? Seems internal to the state machine. I generally have taken the approach of only exposing states to the frontend that should show something different to the user

The frontend doesn't need to know about it, but if we don't handle it then the switch statement will fall through to no_hardware and the frontend will show an error. The chances of that happening are pretty slim - it would have to query status at the exact moment the transition state is running.

I think the best way to handle is to just return interpreting here.

jonahkagan · 2023-09-20T16:22:08Z

apps/mark-scan/frontend/src/pages/paper_reloaded_page.tsx

+  const history = useHistory();
+  useEffect(() => {
+    history.push('/ready-to-review');
+  });


Things seem to be getting quite complicated with the rendering of screens and routing on the frontend. Even with the comments, I'm having trouble figuring out what's going on.

The approach that VxScan used of having the frontend manage auth and the backend manage the scanning flow was much simpler because the entire scanning flow lives within the voter view - the poll worker and election manager sections of the app don't interact with the scanner.

I don't think this paradigm works well for BMDs (especially not for this more complex print and scan device), since the poll worker and voter are both deeply involved in the voting flow.

Now that we've got auth setup on the backend, I think we might want to consider managing all of the voting flow on the backend. That would look like having one VotingFlow component on the frontend with a big switch for all of the machine states, and not using frontend routing within that component.

What do you think? I'm not down in the details so this may not be as easy as it sounds from my vantage point.

I agree - if we can get rid of frontend routing it would be a big win. Not sure yet if doing that is blocked by migrating the remaining legacy frontend state to the backend. I think this change would be too big to fit into this PR so I've created #3985 to track.

Cool, I agree it's probably better to do as a separate refactor.

kshen0 · 2023-09-20T18:44:06Z

All suggestions addressed in 7669f7e and fa1cb2b

jonahkagan · 2023-09-20T20:44:58Z

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

+            cond: (context) =>
+              context.interpretation
+                ? context.interpretation[0].interpretation.type === 'BlankPage'
+                : false,


I was thinking with the assert that you could remove the conditional check. So something like this:

assert(context.interpretation) return context.interpretation[0].interpretation.type === 'BlankPage`

or even

assertDefined(context.interpretation)[0].interpretation.type === 'BlankPage'

Sorry I wasn't clear enough!

kshen0 · 2023-09-20T21:38:36Z

QA'd these changes after 700b335

Base automatically changed from kevin/vxmarkscan-alert-pollworker-after-invalidate to main September 18, 2023 20:53

kshen0 force-pushed the kevin/vxmarkscan-warn-when-interpretation-fails branch from 9ea3d8a to 683054b Compare September 18, 2023 22:36

kshen0 added the user_facing_change label Sep 19, 2023

kshen0 marked this pull request as ready for review September 19, 2023 22:08

kshen0 requested a review from a team as a code owner September 19, 2023 22:08

kshen0 requested review from eventualbuddha, jonahkagan, a team and arsalansufi and removed request for a team September 19, 2023 22:08

kshen0 commented Sep 19, 2023

View reviewed changes

kshen0 added 3 commits September 19, 2023 15:51

WIP page to alert poll worker after ballot invalidation

8544995

change blank ballot flow to present paper only (no eject)

e308fad

clarify comments

56a0845

kshen0 force-pushed the kevin/vxmarkscan-warn-when-interpretation-fails branch from 46c904d to 56a0845 Compare September 19, 2023 22:51

kshen0 commented Sep 19, 2023

View reviewed changes

remove unused button text

16825cb

kshen0 commented Sep 19, 2023

View reviewed changes

jonahkagan reviewed Sep 20, 2023

View reviewed changes

PR feedback

7669f7e

kshen0 mentioned this pull request Sep 20, 2023

Use frontend stateMachineState only (no auth) when deciding what page to render #3985

Open

more PR feedback

fa1cb2b

kshen0 requested a review from jonahkagan September 20, 2023 18:43

jonahkagan approved these changes Sep 20, 2023

View reviewed changes

fix done state; better assertions

700b335

kshen0 merged commit 4434cb1 into main Sep 20, 2023

kshen0 deleted the kevin/vxmarkscan-warn-when-interpretation-fails branch September 20, 2023 21:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kevin/vxmarkscan warn when interpretation fails #3981

Kevin/vxmarkscan warn when interpretation fails #3981

kshen0 commented Sep 18, 2023 •

edited

Loading

kshen0 Sep 19, 2023

kshen0 Sep 19, 2023 •

edited

Loading

jonahkagan Sep 20, 2023

kshen0 Sep 19, 2023

jonahkagan Sep 20, 2023

jonahkagan Sep 20, 2023

jonahkagan Sep 20, 2023

jonahkagan Sep 20, 2023

kshen0 Sep 20, 2023

jonahkagan Sep 20, 2023

jonahkagan Sep 20, 2023

kshen0 Sep 20, 2023

jonahkagan Sep 20, 2023

kshen0 Sep 20, 2023

jonahkagan Sep 20, 2023

kshen0 Sep 20, 2023

jonahkagan Sep 20, 2023

kshen0 Sep 20, 2023

jonahkagan Sep 20, 2023

kshen0 commented Sep 20, 2023

jonahkagan Sep 20, 2023

kshen0 commented Sep 20, 2023

		case state.matches('transition_interpretation'):
		return 'transition_interpretation';

Kevin/vxmarkscan warn when interpretation fails #3981

Kevin/vxmarkscan warn when interpretation fails #3981

Conversation

kshen0 commented Sep 18, 2023 • edited Loading

Overview

Demo Video or Screenshot

Testing Plan

Checklist

Choose a reason for hiding this comment

kshen0 Sep 19, 2023 • edited Loading

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

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 Sep 20, 2023

Choose a reason for hiding this comment

kshen0 commented Sep 20, 2023

kshen0 commented Sep 18, 2023 •

edited

Loading

kshen0 Sep 19, 2023 •

edited

Loading