Update VxAdmin to support importing new CVR export format #3956

arsalansufi · 2023-09-12T17:09:25Z

Easiest reviewed by commit

Overview

Issue link: #3926

This PR completes the end-to-end wiring of continuous export! It specifically updates VxAdmin to support importing the new CVR export format. The user experience is unchanged by this PR. Just a lot of under-the-hood tweaks.

All of the new logic is feature flag gated. Once I merge this PR, I'll work on pruning the feature flag and old CVR export/import logic.

Testing

Tested CVR export and import manually
Updated some unit tests

I've mostly punted on automated tests, which I'll add in follow-up PRs, since this PR is large enough as is.

Checklist

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

Also rename apps/admin/backend/src/cvr_files.ts legacy_cast_vote_records.ts

unsafeParse expects an object, not a JSON string. Use safeParseJson instead, which expects a JSON string.

Where these helpers focus on reading in CVR exports but leave the actual persisting logic to consumers

arsalansufi · 2023-09-12T17:13:08Z

libs/utils/src/filenames.test.ts

+      expectedDirectoryName
+    );
+  }
+);


This test.each is just moved down from above. The one below is new.

arsalansufi · 2023-09-12T17:13:27Z

libs/utils/src/filenames.ts

+    directoryNameComponents.unshift('TEST');
+  }
+  return directoryNameComponents.join(SECTION_SEPARATOR);
+}


Similarly, this function is just moved down from above. The one below is new.

arsalansufi · 2023-09-12T17:19:38Z

libs/auth/src/artifact_authentication.ts

      );
+      const parseResult = safeParseJson(


As noted in the commit description, the mistake here was that unsafeParse expects an object, not a JSON string. safeParseJson, on the other hand, expects a JSON string, which is what we're providing.

Tests missed this because of how I'd incorrectly mocked unsafeParse 🙈

I recognize that I should just not mock, but here's why I mocked for context:

// Avoid having to prepare a complete CVR.CastVoteRecordReport object for // CastVoteRecordExportMetadata

arsalansufi · 2023-09-12T17:29:51Z

libs/backend/src/cast_vote_records/import.ts

+ * are not read/parsed, but their existence is validated such that consumers can safely access
+ * them.
+ */
+export async function readCastVoteRecordExport(


My mental model for the separation of responsibilities between libs/backend and apps/admin/backend re import is that:

libs/backend reads/parses data and performs validation internal to an export (i.e. it doesn't perform contextual validation like whether the fields in an export and the election definition correspond)

apps/admin/backend actually persists data and performs contextual validation

This separation will allow us to reuse libs/backend import code in VxScan, when we build support for bootstrapping a VxScan with another VxScan's CVRs. The persistence logic and contextual validation for VxScan bootstrapping differ from the persistence logic and contextual validation for VxAdmin tabulation, so it's nice that those pieces are separated out.

arsalansufi · 2023-09-12T17:38:33Z

apps/admin/backend/src/legacy_cast_vote_records.ts

+          scannerIds: [directoryNameComponents.machineId],
+        });
+        continue;
+      }


Whereas previously, we could extract everything that we needed for the "list of exports on the USB" UI from the directory name, now, we have to pull some info from the metadata file.

Note that I think it's okay for us to extract this information without authenticating the export (which can be time consuming since we have to re-hash), as we aren't actually importing anything yet. If someone tries to trick the system by changing a directory name or the metadata file, the worst that they'll be able to do is get the "list of exports on the USB" UI to display something misleading. Importing will still fail.

We've applied similar principles to Java Card auth (e.g. here).

arsalansufi · 2023-09-12T17:42:09Z

apps/admin/frontend/src/components/import_cvrfiles_modal.tsx

@@ -62,6 +65,89 @@ const Content = styled.div`
  overflow: hidden;
 `;

+/* c8 ignore start */
+function userReadableMessageFromError(


Note that this function lives on the frontend. The old import code has an equivalent function live on the backend and sends a user-readable message down to the frontend for it to use. Ultimately, this message-preparation is a UI/frontend concern and not a backend concern, hence my moving it to the frontend.

This will become even more necessary once we start working on multi-language support (as noted here).

arsalansufi · 2023-09-12T17:42:45Z

apps/admin/frontend/src/components/import_cvrfiles_modal.tsx

+    }
+  }
+}
+/* c8 ignore stop */


I know I've added a lot of code coverage ignores. I've opened issues to track all the tests that I'm deferring to later PRs.

arsalansufi · 2023-09-12T18:03:55Z

apps/admin/backend/src/cast_vote_records.ts

+    return err({ type: 'invalid-mode', currentMode });
+  }
+
+  const existingImportId = store.getCastVoteRecordFileByHash(


Throughout the code and UI, we talk about a singular CVR file per export, which is no longer the case. I'm going to update terminology in a follow-up PR

adghayes · 2023-09-12T21:39:56Z

libs/backend/src/cast_vote_records/import.ts

+ * An error encountered while reading an individual cast vote record
+ */
+export type ReadCastVoteRecordError = { type: 'invalid-cast-vote-record' } & (
+  | { subType: 'batch-id-not-found' }


I appreciate the subType approach.

adghayes · 2023-09-12T21:43:01Z

libs/backend/src/cast_vote_records/import.ts

+      if (!imageFilePaths.every((filePath) => existsSync(filePath))) {
+        yield wrapError({ subType: 'image-file-not-found' });
+        return;
+      }
+      if (!layoutFilePaths.every((filePath) => existsSync(filePath))) {
+        yield wrapError({ subType: 'layout-file-not-found' });
+        return;
+      }


Am I correct in understanding that, unlike before, where we passed in a list of filenames (like you pass in a list of batchIds), now you just check whether the files exist in the file system? Makes sense.

That's correct!

adghayes · 2023-09-12T21:54:49Z

apps/admin/backend/src/cast_vote_records.ts

+  const precinct = getPrecinctById({
+    election,
+    precinctId: castVoteRecord.BallotStyleUnitId,
+  });


If you wanted, you could use the cached versions of these functions from utils to guarantee O(1).

Nice! 84480ad

Haters always gonna hate on the caching but we've got lots of data to process no time for haters.

adghayes · 2023-09-12T22:01:02Z

apps/admin/backend/src/cast_vote_records.ts

+    // TODO: Calculate the precinct list before iterating through cast vote records, once there is
+    // only one geopolitical unit per batch
+    store.updateCastVoteRecordFileRecord({
+      id: importId,
+      precinctIds,
+    });


Now that we have a metadata file, I'm hoping we can eventually get the precinct list in the file up front to avoid this.

👍 Opened an issue for myself so that I don't forget to revisit this: #3968

adghayes · 2023-09-12T22:02:43Z

apps/admin/backend/src/app.ts

+        )
+      ) {
+        const userRole = assertDefined(await getUserRole());
+        const importResult = await importCastVoteRecords(store, input.path);


How are you handling the manual file selection use case? As in line 446 below.

Ah good catch, I'm not handling that case.

That said, it turns out that manual file selection is already broken because it only allows you to select an individual file, but for quite some time now, we've been exporting a directory and not a file.

Gonna revisit this holistically as its own issue, potentially just removing the manual option altogether, after checking in with others: #3967

It's a bit of a hidden feature that I failed to properly publicize, but manual file selection is not broken. When I switched cast vote records to directories, I added the indicated logic so that you could select the cast-vote-record-report.json at the root of the directory, which would import the directory.

I use this escape hatch all the time when doing manual testing in VxAdmin. It's far easier than having to always get a cast vote record report on a real or mocked USB drive, so I'd vote strongly to keep the option or something like it.

Ah really great to know! In that case, will plan on maintaining

arsalansufi added 9 commits September 12, 2023 13:02

Move VxAdmin CVR helpers to libs/utils

05c37b9

Also rename apps/admin/backend/src/cvr_files.ts legacy_cast_vote_records.ts

Add libs/utils helper for parsing new export directory name format

f8abdf5

Fix artifact authentication bug

8da64a4

unsafeParse expects an object, not a JSON string. Use safeParseJson instead, which expects a JSON string.

Specify encoding when reading files, rather than converting after

85674ce

Add libs/backend helpers for importing CVRs

c3ae016

Where these helpers focus on reading in CVR exports but leave the actual persisting logic to consumers

Update logic for listing CVR exports on a USB

6ea6ad4

Add new VxAdmin CVR import function

9474952

Use new CVR import function in API

830a757

Add client-side logic for user readable messages

a668787

arsalansufi commented Sep 12, 2023

View reviewed changes

arsalansufi requested a review from adghayes September 12, 2023 17:44

arsalansufi marked this pull request as ready for review September 12, 2023 17:44

arsalansufi requested a review from a team as a code owner September 12, 2023 17:44

arsalansufi requested review from kofi-q and removed request for a team and kofi-q September 12, 2023 17:44

arsalansufi commented Sep 12, 2023

View reviewed changes

adghayes approved these changes Sep 12, 2023

View reviewed changes

arsalansufi mentioned this pull request Sep 13, 2023

Revisit manual file selection logic for CVR imports #3967

Closed

Use optimized helpers in libs/utils

84480ad

arsalansufi mentioned this pull request Sep 13, 2023

Explore using the CVR export metadata file to support storing the precinct list on import upfront #3968

Closed

arsalansufi merged commit 10ee207 into main Sep 13, 2023

arsalansufi deleted the arsalan/continuous-export-import branch September 13, 2023 21:39

arsalansufi mentioned this pull request Oct 6, 2023

Polish CVR import and export UIs #4047

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update VxAdmin to support importing new CVR export format #3956

Update VxAdmin to support importing new CVR export format #3956

arsalansufi commented Sep 12, 2023

arsalansufi Sep 12, 2023

arsalansufi Sep 12, 2023

arsalansufi Sep 12, 2023

arsalansufi Sep 12, 2023

arsalansufi Sep 12, 2023 •

edited

Loading

arsalansufi Sep 12, 2023

arsalansufi Sep 12, 2023

arsalansufi Sep 12, 2023

adghayes Sep 12, 2023

adghayes Sep 12, 2023

arsalansufi Sep 13, 2023

adghayes Sep 12, 2023

arsalansufi Sep 13, 2023 •

edited

Loading

adghayes Sep 14, 2023

adghayes Sep 12, 2023

arsalansufi Sep 13, 2023 •

edited

Loading

adghayes Sep 12, 2023

arsalansufi Sep 13, 2023

adghayes Sep 14, 2023

arsalansufi Sep 14, 2023

Update VxAdmin to support importing new CVR export format #3956

Update VxAdmin to support importing new CVR export format #3956

Conversation

arsalansufi commented Sep 12, 2023

Overview

Testing

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

arsalansufi Sep 12, 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

arsalansufi Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arsalansufi Sep 13, 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

arsalansufi Sep 12, 2023 •

edited

Loading

arsalansufi Sep 13, 2023 •

edited

Loading

arsalansufi Sep 13, 2023 •

edited

Loading