-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
unmarked write-in accounting tweaks #5408
Conversation
…vior in the tally path the same
d3c3702
to
a353458
Compare
@arsalansufi re-assign review if needed, but wanted you tagged for review since this is a candidate for the potential v3.1.1 general patch, whether it's being tracked or not already. |
apps/admin/backend/src/store.ts
Outdated
* summary of write-in adjudication, they should be included because they | ||
* exist in the adjudication flow just as marked write-ins do. The behavior | ||
* can be controlled with the `includeUnadjudicatedAndInvalidUnmarkedWriteIns` | ||
* flag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should "unadjudicated and unmarked write-ins" in this comment specifically read "unadjudicated and invalid unmarked write-ins"?
Even with that change, there's a bit of ambiguity as this could mean "unadjudicated marked write-ins and invalid unmarked write-ins". I think "unmarked write-ins that haven't been adjudicated or are invalid" might be clearer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, change incorporated.
I can see someone making the same complaint about the variable name, but I think it reads a bit clearer as a variable name, so I am inclined to leave it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I had the same thought re variable name and agree that that's fine to leave as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Overview
Closes #5382. Does two things:
Testing Plan
Updated existing test coverage to match new logic.