Skip to content
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

Polish CVR import and export UIs #4047

Merged
merged 7 commits into from
Oct 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions apps/admin/backend/src/app.cvr_files.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,12 @@ import {
electionTwoPartyPrimaryFixtures,
} from '@votingworks/fixtures';
import { LogEventId } from '@votingworks/logging';
import { CVR, CVR as CVRType } from '@votingworks/types';
import { basename } from 'path';
import {
CVR,
CVR as CVRType,
CastVoteRecordExportFileName,
} from '@votingworks/types';
import path, { basename } from 'path';
import { vxBallotType } from '@votingworks/types/src/cdf/cast-vote-records';
import {
BooleanEnvironmentVariableName,
Expand Down Expand Up @@ -605,3 +609,17 @@ test('error if cast vote records from different files share same ballot id but h
expect(await apiClient.getCastVoteRecordFiles()).toHaveLength(1);
await expectCastVoteRecordCount(apiClient, 184);
});

test('specifying path to metadata file instead of path to export directory (for manual file selection)', async () => {
const { apiClient, auth } = buildTestEnvironment();
await configureMachine(apiClient, auth, electionDefinition);
mockElectionManagerAuth(auth, electionDefinition.electionHash);

const importResult = await apiClient.addCastVoteRecordFile({
path: path.join(
castVoteRecordExport.asDirectoryPath(),
CastVoteRecordExportFileName.METADATA
),
});
expect(importResult.isOk()).toEqual(true);
});
18 changes: 14 additions & 4 deletions apps/admin/backend/src/app.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { LogEventId, Logger } from '@votingworks/logging';
import {
BallotPackageFileName,
CastVoteRecordExportFileName,
ContestId,
DEFAULT_SYSTEM_SETTINGS,
Id,
Expand Down Expand Up @@ -30,7 +31,7 @@ import {
import * as grout from '@votingworks/grout';
import { useDevDockRouter } from '@votingworks/dev-dock-backend';
import { createReadStream, createWriteStream, promises as fs } from 'fs';
import { join } from 'path';
import path, { join } from 'path';
import {
BALLOT_PACKAGE_FOLDER,
generateFilenameForBallotExportPackage,
Expand Down Expand Up @@ -433,12 +434,21 @@ function buildApi({
await logger.log(LogEventId.ImportCastVoteRecordsInit, userRole, {
message: 'Importing cast vote records...',
});
const importResult = await importCastVoteRecords(store, input.path);
const exportDirectoryPath =
// For manual export selection, users must select the contained metadata file as a proxy
// for the export directory since the UI doesn't support directory selection
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closing the loop on #3956 (comment)!

Whereas you could previously select the top-level cast-vote-record-report.json, you can now select the top-level metadata.json. I couldn't find an easy way to support directory selection. Though non-ideal, we really only use this feature in dev. If an election official were to have to use it, I'd imagine it be in a context where we were guiding them.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏 🙏 🙏 🙏 🙏 🙏 🙏 🙏 🙏 🙏

path.basename(input.path) === CastVoteRecordExportFileName.METADATA
? path.dirname(input.path)
: input.path;
const importResult = await importCastVoteRecords(
store,
exportDirectoryPath
);
if (importResult.isErr()) {
await logger.log(LogEventId.ImportCastVoteRecordsComplete, userRole, {
disposition: 'failure',
message: 'Error importing cast vote records.',
exportDirectoryPath: input.path,
exportDirectoryPath,
errorDetails: JSON.stringify(importResult.err()),
});
} else {
Expand All @@ -451,7 +461,7 @@ function buildApi({
await logger.log(LogEventId.ImportCastVoteRecordsComplete, userRole, {
disposition: 'success',
message,
exportDirectoryPath: input.path,
exportDirectoryPath,
});
}
return importResult;
Expand Down
12 changes: 6 additions & 6 deletions apps/admin/frontend/src/app.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -423,9 +423,9 @@ test('clearing results', async () => {

fireEvent.click(getByText('Tally'));
expect(
(await screen.findByText('Load CVR Files')).closest('button')
(await screen.findByText('Load CVRs')).closest('button')
).toBeDisabled();
expect(getByText('Remove CVR Files').closest('button')).toBeDisabled();
expect(getByText('Remove CVRs').closest('button')).toBeDisabled();
expect(
getByText('Edit Manually Entered Results').closest('button')
).toBeDisabled();
Expand All @@ -442,27 +442,27 @@ test('clearing results', async () => {
apiMock.expectGetManualResultsMetadata([]);
fireEvent.click(getByText('Clear All Tallies and Results'));
getByText(
'Do you want to remove the 1 loaded CVR file and the manually entered data?'
'Do you want to remove the 1 loaded CVR export and the manually entered data?'
);
fireEvent.click(getByText('Remove All Data'));

await waitFor(() => {
expect(getByText('Load CVR Files').closest('button')).toBeEnabled();
expect(getByText('Load CVRs').closest('button')).toBeEnabled();
});
await waitFor(() => {
expect(
getByText('Add Manually Entered Results').closest('button')
).toBeEnabled();
});

expect(getByText('Remove CVR Files').closest('button')).toBeDisabled();
expect(getByText('Remove CVRs').closest('button')).toBeDisabled();
expect(
getByText('Remove Manually Entered Results').closest('button')
).toBeDisabled();

expect(queryByText('Clear All Tallies and Results')).not.toBeInTheDocument();

getByText('No CVR files loaded.');
getByText('No CVRs loaded.');
});

test('can not view or print ballots', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,14 @@ export function ConfirmRemovingFileModal({
let singleFileRemoval = true;
switch (fileType) {
case ResultsFileType.CastVoteRecord: {
fileTypeName = 'CVRs';
const fileList = castVoteRecordFilesQuery.data;
singleFileRemoval = fileList.length <= 1;
fileTypeName = 'CVR Files';
mainContent = (
<React.Fragment>
<P>
Do you want to remove the {fileList.length} loaded CVR{' '}
{pluralize('files', fileList.length)}?
{pluralize('export', fileList.length)}?
</P>
<P>All reports will be unavailable without CVR data.</P>
</React.Fragment>
Expand All @@ -56,7 +56,7 @@ export function ConfirmRemovingFileModal({
<React.Fragment>
<P>
Do you want to remove the {fileList.length} loaded CVR{' '}
{pluralize('files', fileList.length)}
{pluralize('export', fileList.length)}
{hasManualData && ' and the manually entered data'}?
</P>
<P>All reports will be unavailable without CVR data.</P>
Expand All @@ -70,7 +70,7 @@ export function ConfirmRemovingFileModal({

return (
<Modal
centerContent
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we more often left-align our modals than center-align them, so I've made that consistent for CVR modals.

title={`Remove ${fileTypeName}`}
content={mainContent}
actions={
<React.Fragment>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ describe('when USB is properly mounted', () => {
});
await waitFor(() =>
screen.getByText(
/There were no new CVR files automatically found on this USB drive/
/No new CVR exports were automatically found on this USB drive./
)
);

Expand All @@ -137,7 +137,7 @@ describe('when USB is properly mounted', () => {
apiMock.expectGetCastVoteRecordFileMode('test');
apiMock.expectGetCastVoteRecordFiles([]);

await screen.findByText('1,000 new CVRs Loaded');
await screen.findByText('1,000 New CVRs Loaded');

delete window.kiosk;
});
Expand All @@ -152,9 +152,9 @@ describe('when USB is properly mounted', () => {
usbDriveStatus: mockUsbDriveStatus('mounted'),
apiMock,
});
await screen.findByText('Load CVR Files');
await screen.findByText('Load CVRs');
screen.getByText(
/The following CVR files were automatically found on this USB drive./
/The following CVR exports were automatically found on this USB drive./
);

const tableRows = screen.getAllByTestId('table-row');
Expand Down Expand Up @@ -187,8 +187,8 @@ describe('when USB is properly mounted', () => {
apiMock.expectGetCastVoteRecordFiles([]);

userEvent.click(domGetByText(tableRows[0], 'Load'));
await screen.findByText('Loading');
await screen.findByText('1,000 new CVRs Loaded');
await screen.findByText('Loading CVRs');
await screen.findByText('1,000 New CVRs Loaded');
});

test('locks to test mode when in test mode & shows previously loaded files as loaded', async () => {
Expand All @@ -203,7 +203,7 @@ describe('when USB is properly mounted', () => {
apiMock,
});
await screen.findByRole('heading', {
name: 'Load Test Ballot Mode CVR Files',
name: 'Load Test Ballot Mode CVRs',
});

const tableRows = screen.getAllByTestId('table-row');
Expand Down Expand Up @@ -235,7 +235,7 @@ describe('when USB is properly mounted', () => {
usbDriveStatus: mockUsbDriveStatus('mounted'),
apiMock,
});
await screen.findByText('Load Official Ballot Mode CVR Files');
await screen.findByText('Load Official Ballot Mode CVRs');

const tableRows = screen.getAllByTestId('table-row');
expect(tableRows).toHaveLength(1);
Expand Down
83 changes: 48 additions & 35 deletions apps/admin/frontend/src/components/import_cvrfiles_modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -252,11 +252,11 @@ export function ImportCvrFilesModal({ onClose }: Props): JSX.Element | null {
if (currentState.state === 'duplicate') {
return (
<Modal
title="Duplicate File"
title="Duplicate Export"
content={
<P>
The selected file was ignored as a duplicate of a previously loaded
file.
The selected export was ignored as a duplicate of a previously
loaded export.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because exports are now (and have for a while been) directories instead of single files, I've moved away from "file" as the descriptor for a CVR export and shifted to "export"

</P>
}
onOverlayClick={onClose}
Expand All @@ -266,25 +266,31 @@ export function ImportCvrFilesModal({ onClose }: Props): JSX.Element | null {
}

if (currentState.state === 'success') {
const { alreadyPresent, newlyAdded } = currentState.result;
const total = alreadyPresent + newlyAdded;
const content = (() => {
if (alreadyPresent > 0) {
if (total === 1) {
return <P>The 1 CVR in the selected export was previously loaded.</P>;
}
return (
<P>
Of the {format.count(total)} total CVRs in the selected export,{' '}
{format.count(alreadyPresent)}{' '}
{alreadyPresent === 1 ? 'was' : 'were'} previously loaded.
</P>
);
}
return <P>The CVRs in the selected export were successfully loaded.</P>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from better handling singular vs. plural cases, I've added content for the basic, no-duplicates success case. We used to have no content for the success case, just a title. Which made the modal look a bit funny IMO. I've added content for consistency with the other cases.

Before / After

before after

})();
return (
<Modal
title={`${format.count(
currentState.result.newlyAdded
)} new CVRs Loaded`}
content={
currentState.result.alreadyPresent > 0 && (
<P>
Of the{' '}
{format.count(
currentState.result.newlyAdded +
currentState.result.alreadyPresent
)}{' '}
total CVRs in this file,{' '}
{format.count(currentState.result.alreadyPresent)} were previously
loaded.
</P>
)
title={
newlyAdded === 1
? '1 New CVR Loaded'
: `${format.count(newlyAdded)} New CVRs Loaded`
}
content={content}
onOverlayClick={onClose}
actions={<Button onPress={onClose}>Close</Button>}
/>
Expand All @@ -294,8 +300,7 @@ export function ImportCvrFilesModal({ onClose }: Props): JSX.Element | null {
if (
!castVoteRecordFilesQuery.isSuccess ||
!castVoteRecordFileModeQuery.isSuccess ||
!cvrFilesOnUsbQuery.isSuccess ||
currentState.state === 'loading'
!cvrFilesOnUsbQuery.isSuccess
) {
return (
<Modal
Expand All @@ -310,6 +315,10 @@ export function ImportCvrFilesModal({ onClose }: Props): JSX.Element | null {
);
}

if (currentState.state === 'loading') {
return <Modal content={<Loading>Loading CVRs</Loading>} />;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I'm intentionally not specifying an onOverlayClick prop for saving/loading modals so that clicks outside them don't accidentally close them. This came up a few times during the SLI test as a source of confusion. Clicking outside the modal would dismiss it, but the relevant operation would continue behind the scenes, leaving the machine sluggish without any clear indication as to why it was sluggish.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great.

}

if (
usbDriveStatus.status === 'no_drive' ||
usbDriveStatus.status === 'ejected' ||
Expand All @@ -321,8 +330,7 @@ export function ImportCvrFilesModal({ onClose }: Props): JSX.Element | null {
content={
<P>
<UsbImage src="/assets/usb-drive.svg" alt="Insert USB Image" />
Please insert a USB drive in order to load CVR files from the
scanner.
Please insert a USB drive in order to load CVRs from a scanner.
</P>
}
onOverlayClick={onClose}
Expand All @@ -332,8 +340,9 @@ export function ImportCvrFilesModal({ onClose }: Props): JSX.Element | null {
<FileInputButton
data-testid="manual-input"
onChange={processCastVoteRecordFileFromFilePicker}
accept=".json"
>
Select Files
Select Export Manually
</FileInputButton>
)}
<Button onPress={onClose}>Cancel</Button>
Expand Down Expand Up @@ -367,7 +376,7 @@ export function ImportCvrFilesModal({ onClose }: Props): JSX.Element | null {
<tr key={name} data-testid="table-row">
<td>{moment(exportTimestamp).format(TIME_FORMAT)}</td>
<td>{scannerIds.join(', ')}</td>
<td data-testid="cvr-count">{cvrCount}</td>
<td data-testid="cvr-count">{format.count(cvrCount)}</td>
{!fileModeLocked && (
<td>
<LabelText>
Expand Down Expand Up @@ -410,29 +419,33 @@ export function ImportCvrFilesModal({ onClose }: Props): JSX.Element | null {
if (numberOfNewFiles === 0) {
instructionalText = fileModeLocked ? (
<React.Fragment>
There were no new {headerModeText} CVR files automatically found on
this USB drive. Save CVR files to this USB drive from the scanner.
Optionally, you may manually select files to load.
No new {headerModeText.toLowerCase()} CVR exports were automatically
found on this USB drive.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Erring on the side of more concise text since I know longer text often isn't read

</React.Fragment>
) : (
'There were no new CVR files automatically found on this USB drive. Save CVR files to this USB drive from the scanner. Optionally, you may manually select files to load.'
<React.Fragment>
No new CVR exports were automatically found on this USB drive.
</React.Fragment>
);
} else if (fileModeLocked) {
instructionalText = (
<React.Fragment>
The following {headerModeText} CVR files were automatically found on
this USB drive. Previously loaded CVR entries will be ignored.
The following {headerModeText.toLowerCase()} CVR exports were
automatically found on this USB drive.
</React.Fragment>
);
} else {
instructionalText =
'The following CVR files were automatically found on this USB drive. Previously loaded CVR entries will be ignored.';
instructionalText = (
<React.Fragment>
The following CVR exports were automatically found on this USB drive.
</React.Fragment>
);
}

return (
<Modal
modalWidth={ModalWidth.Wide}
title={`Load ${headerModeText} CVR Files`}
title={`Load ${headerModeText} CVRs`}
content={
<Content>
<P>{instructionalText}</P>
Expand Down Expand Up @@ -462,7 +475,7 @@ export function ImportCvrFilesModal({ onClose }: Props): JSX.Element | null {
onChange={processCastVoteRecordFileFromFilePicker}
accept=".json"
>
Select File Manually…
Select Export Manually…
</FileInputButton>
<Button onPress={onClose}>Cancel</Button>
</React.Fragment>
Expand Down
6 changes: 3 additions & 3 deletions apps/admin/frontend/src/screens/tally_screen.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,9 @@ test('with no data loaded', async () => {
await screen.findByRole('heading', {
name: 'Cast Vote Record (CVR) Management',
});
await screen.findByText('No CVR files loaded.');
expect(screen.getButton('Load CVR Files')).toBeEnabled();
expect(screen.getButton('Remove CVR Files')).toBeDisabled();
await screen.findByText('No CVRs loaded.');
expect(screen.getButton('Load CVRs')).toBeEnabled();
expect(screen.getButton('Remove CVRs')).toBeDisabled();
expect(screen.getButton('Add Manually Entered Results')).toBeEnabled();
expect(screen.getButton('Remove Manually Entered Results')).toBeDisabled();
});
Loading