Standardize CVR identification across audit logs and RCTab output #913

nurse-the-code · 2025-01-09T00:29:37Z

Key Changes:

Clean up CastVoteRecord identifier logic
- getId - guaranteed unique identifier per ballot across tabulations. coalesce(computedId, suppliedId)
- getSuppliedId - vendor provided Id. Not reliably unique. Nullable
Uses getId throughout audit log and rctab_cvr.csv
rctab_cvr.csv now explicitly includes both. getId provides the "RCTab CVR Id" and getSuppliedId provides the "Vendor Id" (formerly "Ballot Id")
Changes Dominion computedId delimiter from pipe (|) to dash (-) for improved readability

Testing Needed:

Verify ID consistency:
- ✅ Compare IDs in audit log with rctab_cvr.csv output
- ✅ Ensure id returned from getId method is consistent across multiple tabulations
- ✅ Check ID format follows tabulator-batch-record pattern for records from Dominion CVR sources
Test fallback behavior:
- ✅ Cases where computedId is null
- ✅ Cases where computedId is empty or whitespace
- ✅ Cases where only suppliedId is available
Test with each CVR provider
- ✅ CDF/Unisyn
- ✅ Clear Ballot
- ✅ Dominion (tested with 2024 Alaska US House race)
- ✅ ES&S (in test suite)
- ✅ Hart
- ✅ generic CSV
Review and update failing tests:
- ✅ Done for now

Feature Request: Add Standardized Unique Identifiers for Cast Vote Records #911

Replace UUID-based identification with a more consistent ID system that uses the same identifier across audit logs and RCTab CVR output. The ID is primarily derived from the computedId (tabulator-batch-record) with fallback to suppliedId when computedId is unavailable. Unlike UUIDs which change between tabulations, this approach ensures the same ballot maintains the same identifier across multiple tabulations, making it easier to track specific ballots across different runs. Changes: - Remove UUID generation for CVR identification - Add getPrimaryId() method to centralize ID logic - Update audit log and RCTab CVR to use the same ID format - Change ID delimiter from pipe to dash for better readability - Add TODO to review legacy getId() usage Note: This change ensures that CVR IDs are consistent between audit logs, RCTab CVR output, and repeated tabulations, making it easier to track individual ballots across different output formats and multiple runs.

src/main/java/network/brightspots/rcv/CastVoteRecord.java

…ture/issue-911-rctab-cvr-primary-id

src/main/java/network/brightspots/rcv/CastVoteRecord.java

src/main/java/network/brightspots/rcv/OutputWriter.java

Dominion"

src/main/java/network/brightspots/rcv/OutputWriter.java

...ces/network/brightspots/rcv/test_data/conversions_from_ess/conversions_from_ess_expected.csv

methods.

src/test/java/network/brightspots/rcv/CastVoteRecordTests.java

…ture/issue-911-rctab-cvr-primary-id

src/main/java/network/brightspots/rcv/OutputWriter.java

null. Null handling is now handled where getSuppliedId is called.

src/main/java/network/brightspots/rcv/OutputWriter.java

src/test/java/network/brightspots/rcv/CastVoteRecordTests.java

artoonie · 2025-02-20T13:00:14Z

src/main/java/network/brightspots/rcv/DominionCvrReader.java

          Stream.of(tabulatorId, batchId, Integer.toString(recordId))
              .filter(s -> s != null && !s.isBlank())
-              .collect(Collectors.joining("|"));
+              .collect(Collectors.joining("-")); // using a dash since this is an Id


Not sure I understand why - vs | is important, but the comment makes me think it matters? Is it because it will be used in filenames? If so, perhaps that should be explicit.

It's an aesthetic choice. Originally I had used pipes as they aren't really used in the wild (certainly less so than -), and I wanted to signal that it was computed. I'm going to revert to pipe to signal that.

I believe we discovered another reason why pipes won't work, but I forget what it was. Can we change the comment to be the reason you discovered?

artoonie

Some [nit]s but this looks good

src/main/java/network/brightspots/rcv/OutputWriter.java

artoonie · 2025-03-26T00:58:30Z

src/main/java/network/brightspots/rcv/DominionCvrReader.java

          Stream.of(tabulatorId, batchId, Integer.toString(recordId))
              .filter(s -> s != null && !s.isBlank())
-              .collect(Collectors.joining("|"));
+              .collect(Collectors.joining("-")); // using a dash since this is an Id


I believe we discovered another reason why pipes won't work, but I forget what it was. Can we change the comment to be the reason you discovered?

nurse-the-code added 2 commits January 8, 2025 02:47

Experimental implementation of GitHub issue 991:

7e7e29b

Feature Request: Add Standardized Unique Identifiers for Cast Vote Records #911

nurse-the-code mentioned this pull request Jan 9, 2025

Add Standardized Unique Identifiers for Cast Vote Records #912

Closed

nurse-the-code linked an issue Jan 9, 2025 that may be closed by this pull request

Feature Request: Add common id to both audit log and rctab_cvr.csv #911

Closed

nurse-the-code commented Jan 9, 2025

View reviewed changes

src/main/java/network/brightspots/rcv/CastVoteRecord.java Outdated Show resolved Hide resolved

nurse-the-code commented Jan 9, 2025

View reviewed changes

src/main/java/network/brightspots/rcv/CastVoteRecord.java Outdated Show resolved Hide resolved

Removed depricated reference to UUID.

3dd2f90

nurse-the-code mentioned this pull request Jan 13, 2025

Filter out dominion partial ballots #910

Merged

nurse-the-code added 2 commits January 15, 2025 01:51

Merge branch 'develop' of https://github.com/BrightSpots/rcv into fea…

dde3587

…ture/issue-911-rctab-cvr-primary-id

Fixed all tests except "Test Convert to rctab_cvr works for Dominion"

d967d53

nurse-the-code commented Jan 15, 2025

View reviewed changes

src/main/java/network/brightspots/rcv/CastVoteRecord.java Outdated Show resolved Hide resolved

nurse-the-code commented Jan 15, 2025

View reviewed changes

src/main/java/network/brightspots/rcv/OutputWriter.java Outdated Show resolved Hide resolved

nurse-the-code commented Jan 15, 2025

View reviewed changes

src/main/java/network/brightspots/rcv/OutputWriter.java Outdated Show resolved Hide resolved

nurse-the-code added 3 commits January 15, 2025 11:41

Fixed broken TabulatorTest "Test Convert to rctab_cvr works for

276a79c

Dominion"

Updates per GitHub comments

dccb737

fixed broken tests

16941f7

nurse-the-code commented Jan 22, 2025

View reviewed changes

src/main/java/network/brightspots/rcv/OutputWriter.java Show resolved Hide resolved

nurse-the-code commented Jan 22, 2025

View reviewed changes

src/main/java/network/brightspots/rcv/OutputWriter.java Outdated Show resolved Hide resolved

nurse-the-code commented Jan 22, 2025

View reviewed changes

...ces/network/brightspots/rcv/test_data/conversions_from_ess/conversions_from_ess_expected.csv Show resolved Hide resolved

nurse-the-code added 2 commits January 24, 2025 08:06

Added unit tests for CastVoteRecord's getId() and getSuppliedId()

944d6ee

methods.

Removing TODOs associated with this PR

9e40a87

yezr reviewed Jan 28, 2025

View reviewed changes

src/test/java/network/brightspots/rcv/CastVoteRecordTests.java Outdated Show resolved Hide resolved

Merge branch 'develop' of https://github.com/BrightSpots/rcv into fea…

f69fb95

…ture/issue-911-rctab-cvr-primary-id

nurse-the-code commented Jan 30, 2025

View reviewed changes

src/main/java/network/brightspots/rcv/OutputWriter.java Show resolved Hide resolved

CastVoteRecord's getSuppliedId now returns null when the suppliedId is

cf52345

null. Null handling is now handled where getSuppliedId is called.

yezr reviewed Jan 31, 2025

View reviewed changes

src/main/java/network/brightspots/rcv/OutputWriter.java Show resolved Hide resolved

Updated rctab_cvr.csv headers

f4272c9

yezr added the ready-for-code-review ready for a live code review label Feb 11, 2025

artoonie reviewed Feb 20, 2025

View reviewed changes

fix variable names and access modifiers

d15c79d

yezr mentioned this pull request Mar 12, 2025

Move some test coverage explicitly to code #922

Open

yezr force-pushed the feature/issue-911-rctab-cvr-primary-id branch from 6669937 to d15c79d Compare March 13, 2025 14:16

defer to existing tests for CVR IDs

77a43b2

yezr marked this pull request as ready for review March 18, 2025 15:04

artoonie previously approved these changes Mar 26, 2025

View reviewed changes

nit single line comment fix

79cbe0e

yezr dismissed artoonie’s stale review via 79cbe0e March 31, 2025 13:59

comment for using hyphens in dominion cvr id

3f7b1ac

artoonie approved these changes Mar 31, 2025

View reviewed changes

Merge branch 'develop' into feature/issue-911-rctab-cvr-primary-id

2929cec

yezr merged commit 926948c into develop Apr 3, 2025
1 check passed

yezr deleted the feature/issue-911-rctab-cvr-primary-id branch April 3, 2025 19:45

Standardize CVR identification across audit logs and RCTab output #913

Standardize CVR identification across audit logs and RCTab output #913

Uh oh!

Conversation

nurse-the-code commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

artoonie Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

yezr Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

artoonie Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

artoonie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

artoonie Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nurse-the-code commented Jan 9, 2025 •

edited

Loading