Update libs/cvr-fixture-generator to use new CVR export format (plus other small tweaks) #3988

arsalansufi · 2023-09-21T16:32:49Z

Easiest reviewed by commit

Overview

Issue link: #3890

A very first step in removing old CVR code, this PR updates libs/cvr-fixture-generator to use the new CVR export format. In my next PR, I'll actually generate new fixtures and have app tests use them.

This PR also makes various small tweaks to libs/backend functions, all geared around prep for removal of old CVR code.

Testing

Updated automated CVR fixture generator tests
Ran CVR fixture generator and manually validated the generated contents

Checklist

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

- Remove new CVR functions dependence on legacy CVR functions by moving relevant helpers - Have SKIP_*_AUTHENTICATION feature flags actually skip auth check, instead of still performing the auth check and ignoring the result - On CVR import, properly account for the fact that layout files won't exist for BMD ballots - Fix a buggy error check that was referencing the wrong variable - Rename castVoteRecords castVoteRecordIterator for clarity

arsalansufi · 2023-09-21T16:37:38Z

libs/backend/src/ballot_package/ballot_package_io.ts

+        type: 'election_package',
+        filePath: filepathResult.ok(),
+      });
+  if (artifactAuthenticationResult.isErr()) {


Thought it made more sense to actually skip the auth check, instead of still performing the auth check and ignoring the result, when the relevant feature flag is set

arsalansufi · 2023-09-21T16:38:37Z

libs/backend/src/cast_vote_records/export.ts

+/**
+ * A sheet to be included in a CVR export
+ */
+export interface ResultSheet {


Moved this from legacy_export.ts to export.ts so that export.ts doesn't have to import from legacy_export.ts. A precursor to fully deleting legacy_export.ts in an upcoming PR

arsalansufi · 2023-09-21T16:39:40Z

libs/backend/src/cast_vote_records/import.ts

    if (castVoteRecord.BallotSheetId) {
      const parseBallotSheetIdResult = safeParseNumber(
        castVoteRecord.BallotSheetId
      );
-      if (parseResult.isErr()) {
+      if (parseBallotSheetIdResult.isErr()) {


🙈 Was referencing the wrong result variable here

Will be adding tests soon to get all the new CVR code up to 100% code coverage

arsalansufi · 2023-09-21T16:40:34Z

libs/backend/src/cast_vote_records/import.ts

+              `${path.parse(location).name}.layout.json`
+            )
+          ) as [string, string])
+        : undefined;


Realized that I wasn't accounting for the fact that layout files won't exist for BMD ballots

For now! :)

arsalansufi · 2023-09-21T16:41:48Z

libs/backend/src/cast_vote_records/import.ts

+/**
+ * Determines whether or not a cast vote record report was generated on a machine in test mode
+ */
+export function isTestReport(


Another function that I needed to move to unblock removing an old file, in this case legacy_import.ts, in an upcoming PR

arsalansufi · 2023-09-21T16:49:31Z

libs/cvr-fixture-generator/src/cli/generate/main.ts

@@ -58,6 +52,8 @@ interface IO {

 /**
 * Command line interface for generating a cast vote record file.
+ *
+ * TODO: Make more full use of export functions in libs/backend to avoid duplicating logic.


I'm pretty excited about this idea! My thinking:

Replace the CVR-generating function in this lib with a function that generates ResultSheet objects

Prepare a mock USB drive and mock store and pass them and the above ResultSheet objects to exportCastVoteRecordsToUsbDrive from libs/backend, to have that function do all the heavy lifting of generating a CVR fixture

Then, we wouldn't have to worry about keeping libs/backend and libs/cvr-fixture-generator in sync.

I'd tackle this now, but I want to first finish removing old code, and this is a bit of a tangent.

That would be wonderful. As such it's like we are maintaining a separate CVR format entirely.

My only concern is that ideally, we'd like to be able to auto-generate CVRs in libs/fixtures. A little bit ago I created #3905.

Introducing more dependencies could make that harder.

But perhaps the solution is to allow those dependencies and not merge the two libraries, but rather use scripting to call one library from the other.

Ah yes thanks for pointing that out! I like the idea of using scripts to call from one library to another, instead of a hard dependency

arsalansufi · 2023-09-21T16:53:59Z

libs/cvr-fixture-generator/src/generate_cvrs.ts

-                          Location: backHasWriteIns
-                            ? backImageFileUri
-                            : undefined,
+                          Location: backImageFileUri,


A bit in the weeds but in new CVR code, I'm taking an all or nothing approach to ballot images. For every ballot, either both front and back images are included, or no images are included.

I think that this will ultimately make code easier to reason about. And there's even a product argument to be made. During one of the demos that I attended in NH, a request came up to display both sides of a ballot for write-in adjudication, even if only one side had a write-in, to provide the maximal possible context when adjudicating.

I agree. When I started on the CVR directories, I wanted to do only the relevant pages for write-ins. In the context where everything had to be exported at the end of the day, there was some reason to do that. But with continuous export, I think it makes much more sense to include all images. There's also a way to interpret VVSG that it's necessary to do so. Regardless of whether we buy into that interpretation, Roe and I have previously discussed that including all images is a better story for audit-ability.

Maybe this would also allow us to just use CVRs for some (mostly internal) use cases where we use backups? Not sure.

Awesome, thanks for validating

Maybe this would also allow us to just use CVRs for some (mostly internal) use cases where we use backups?

That's the plan! With some tweaks (#3922), we should be able to replace the old backup format

adghayes · 2023-09-21T21:36:09Z

libs/backend/src/cast_vote_records/import.ts

+              `${path.parse(location).name}.layout.json`
+            )
+          ) as [string, string])
+        : undefined;


For now! :)

adghayes · 2023-09-22T01:11:59Z

libs/cvr-fixture-generator/src/cli/generate/main.ts

@@ -58,6 +52,8 @@ interface IO {

 /**
 * Command line interface for generating a cast vote record file.
+ *
+ * TODO: Make more full use of export functions in libs/backend to avoid duplicating logic.


That would be wonderful. As such it's like we are maintaining a separate CVR format entirely.

adghayes · 2023-09-22T01:20:30Z

libs/cvr-fixture-generator/src/generate_cvrs.ts

-                          Location: backHasWriteIns
-                            ? backImageFileUri
-                            : undefined,
+                          Location: backImageFileUri,


I agree. When I started on the CVR directories, I wanted to do only the relevant pages for write-ins. In the context where everything had to be exported at the end of the day, there was some reason to do that. But with continuous export, I think it makes much more sense to include all images. There's also a way to interpret VVSG that it's necessary to do so. Regardless of whether we buy into that interpretation, Roe and I have previously discussed that including all images is a better story for audit-ability.

Maybe this would also allow us to just use CVRs for some (mostly internal) use cases where we use backups? Not sure.

adghayes · 2023-09-22T01:23:50Z

libs/cvr-fixture-generator/src/cli/generate/main.ts

@@ -58,6 +52,8 @@ interface IO {

 /**
 * Command line interface for generating a cast vote record file.
+ *
+ * TODO: Make more full use of export functions in libs/backend to avoid duplicating logic.


My only concern is that ideally, we'd like to be able to auto-generate CVRs in libs/fixtures. A little bit ago I created #3905.

Introducing more dependencies could make that harder.

But perhaps the solution is to allow those dependencies and not merge the two libraries, but rather use scripting to call one library from the other.

arsalansufi added 5 commits September 21, 2023 12:31

Updave VxAdmin backend to account for libs/backend tweaks

e106e51

Add libs/auth as a libs/cvr-fixture-generator dependency

d056278

Update libs/cvr-fixture-generator to use the new CVR export format

d890424

Update libs/cvr-fixture-generator tests

ac2d25c

arsalansufi commented Sep 21, 2023

View reviewed changes

arsalansufi requested a review from adghayes September 21, 2023 16:54

arsalansufi marked this pull request as ready for review September 21, 2023 16:54

arsalansufi requested a review from a team as a code owner September 21, 2023 16:54

arsalansufi requested review from kofi-q and removed request for a team and kofi-q September 21, 2023 16:54

adghayes approved these changes Sep 22, 2023

View reviewed changes

Have libs/cvr-fixture-generator use UUIDs

867524f

arsalansufi merged commit 3ba81d4 into main Sep 22, 2023

arsalansufi deleted the arsalan/remove-old-code-part-1 branch September 22, 2023 14:54

This was referenced Sep 25, 2023

Remove old CVR code from VxAdmin #3999

Merged

Deprecate VxScan's existing backup functionality in favor of "extended" CVR exports #4011

Merged

arsalansufi mentioned this pull request Oct 23, 2023

Update CVR fixture generator to use actual top-level CVR export functions instead of recreating the logic #4098

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update libs/cvr-fixture-generator to use new CVR export format (plus other small tweaks) #3988

Update libs/cvr-fixture-generator to use new CVR export format (plus other small tweaks) #3988

arsalansufi commented Sep 21, 2023

arsalansufi Sep 21, 2023

arsalansufi Sep 21, 2023

arsalansufi Sep 21, 2023 •

edited

Loading

arsalansufi Sep 21, 2023

adghayes Sep 21, 2023

arsalansufi Sep 21, 2023

arsalansufi Sep 21, 2023

adghayes Sep 22, 2023

adghayes Sep 22, 2023

arsalansufi Sep 22, 2023

arsalansufi Sep 21, 2023

adghayes Sep 22, 2023

arsalansufi Sep 22, 2023

adghayes Sep 21, 2023

adghayes Sep 22, 2023

adghayes Sep 22, 2023

adghayes Sep 22, 2023

Update libs/cvr-fixture-generator to use new CVR export format (plus other small tweaks) #3988

Update libs/cvr-fixture-generator to use new CVR export format (plus other small tweaks) #3988

Conversation

arsalansufi commented Sep 21, 2023

Overview

Testing

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

arsalansufi Sep 21, 2023 •

edited

Loading