[i18n-Audio] Add DB schemas and shared API/store stubs #3987

kofi-q · 2023-09-21T15:47:01Z

Overview

🔍 Easier to review individual commits.

Related spec here.

Adding:

DB schemas to Mark, Scan, Mark-Scan
- (Technically, Scan doesn't need the audio tables, but it made things simpler to have everything be identical across apps - the apps can then enable/disable audio functionality as needed).
Shared backend API handler and store stubs
- Wanted to avoid duplicating code across 3 apps and keeping them in sync.
- We're not currently architected to naturally support shared API/store logic in this way, so I'm open to pushback if it feels like this is breaking patterns a bit too much.

Testing Plan

Added a shared test runner invoked by each app to verify the shared handlers and store logic work in the context of the app (which, in turn will help ensure the DB schemas stay consistent, since I couldn't find an easy way to extract the table schemas to a common location)

Checklist

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

~~[ ] I have added a screenshot and/or video to this PR to demo the change~~
~~[ ] I have added the "user_facing_change" label to this PR to automate an announcement in #machine-product-updates~~

arsalansufi

Looks great! Really liking the new sharing patterns, with one question around the testing pattern

arsalansufi · 2023-09-21T17:31:40Z

apps/mark-scan/backend/schema.sql

+
+create table ui_string_audio_keys (
+  language_code text primary key,
+  data text not null, -- JSON blob - see libs/types/UiStringTranslationsSchema


I think we want to reference libs/types/UiStringAudioKeysSchema here (and same in the other schema.sql files)

Ah, thanks for the catch!

arsalansufi · 2023-09-21T17:35:56Z

apps/mark-scan/backend/schema.sql

+);
+
+create table audio_clips (
+  id text not null,


Double checking my understanding: These id values are the same as the audio keys as described in the spec, e.g. d5b21f, correct?

Yeah, these correspond the audio keys - and I'm realising the inconsistent naming is confusing (especially once I started building on this PR 😅 ). I think I'm going to settle on id and change the property names in related schemas to id/audioId to make things more consistent

Nice, I like that!

arsalansufi · 2023-09-21T17:44:26Z

apps/mark-scan/backend/src/store.ts

-    return new Store(DbClient.fileClient(dbPath, SchemaPath));
+    const client = DbClient.fileClient(dbPath, SchemaPath);
+    const uiStringsStore = createUiStringStore(client);
+    return new Store(client, uiStringsStore);


Nice! I like these patterns that you're setting for shared store and API code. I'd been with experimenting with similar ideas recently, e.g.

vxsuite/apps/scan/backend/src/store.ts

Lines 36 to 40 in 6c454a5

import {

clearCastVoteRecordHashes,

getCastVoteRecordRootHash,

updateCastVoteRecordHashes,

} from '@votingworks/auth';

vxsuite/apps/scan/backend/src/store.ts

Lines 958 to 968 in 6c454a5

getCastVoteRecordRootHash(): string {

return getCastVoteRecordRootHash(this.client);

}

updateCastVoteRecordHashes(cvrId: string, cvrHash: string): void {

updateCastVoteRecordHashes(this.client, cvrId, cvrHash);

}

clearCastVoteRecordHashes(): void {

clearCastVoteRecordHashes(this.client);

}

I like that what you're setting up is more automatic than what I'd set up. Looks like we can add new methods to libs/backend and the app stores and APIs will automatically update 🪄.

Re SQL schemas, I too just copied the schemas since we don't yet have a clear pattern for sharing those - that seems fine to me.

Nice! I missed those - makes me feel better knowing that we have other instances where we're sharing the query code

arsalansufi · 2023-09-21T18:11:48Z

apps/mark-scan/backend/src/app.test.ts

@@ -240,3 +254,40 @@ test('configureWithSampleBallotPackageForIntegrationTest configures electionGene
  const electionDefinitionResult = await apiClient.getElectionDefinition();
  expect(electionDefinitionResult).toEqual(electionGeneralDefinition);
 });
+
+test('installs UI Strings API', async () => {


I'm taking a sec to wrap my mind around the difference between these tests and the tests in app.ui_strings.test.ts, specifically trying to understand why the former are necessary.

It seems like the main thing tested in this file that's not tested in app.ui_strings.test.ts is the Grout client's ability to access the endpoints. Otherwise, app.ui_strings.test.ts tests everything tested in here and more (installation of the UI strings API + actual implementation since it doesn't mock).

I wonder if we can just assume that the Grout client will work fine, given libs/grout tests. Or alternatively, have the shared ui_strings_api_test_runner.ts create a Grout client for testing.

I might be missing something! Just wondering if we can get rid of these app.test.ts tests so that everything, tests included, really is automatic.

Ah, thanks - I lost perspective on this after a couple iterations. Initially had the shared test runner just taking in a store input and testing that, but you're spot on WRT not needing these tests since the test runner is testing against an app's actual API.
Will go ahead and drop these!

👍 Figured that might've been the case re iterations!

arsalansufi

LGTM!

kofi-q added 5 commits September 20, 2023 18:56

Add DB schemas

9087cdf

Add shared API and store to libs/backend

e8fab0c

Install shared API and store in Mark, Scan, & Mark-Scan

e4f9007

Add app tests, using shared test utils

e5f6511

Update libs/backend package.json

8299d88

kofi-q requested a review from arsalansufi September 21, 2023 15:47

kofi-q requested a review from a team as a code owner September 21, 2023 15:47

kofi-q requested review from adghayes and removed request for a team and adghayes September 21, 2023 15:47

arsalansufi reviewed Sep 21, 2023

View reviewed changes

kofi-q added 2 commits September 21, 2023 13:33

Update zod schema reference in DB schemas

dff26d8

Drop redundant tests

58cbdd3

arsalansufi approved these changes Sep 21, 2023

View reviewed changes

kofi-q merged commit d9a9d06 into main Sep 21, 2023

kofi-q deleted the i18n-audio-db-schemas branch September 21, 2023 19:03

kofi-q mentioned this pull request Sep 21, 2023

[i18n-audio] Use 'audioID' naming instead of 'audioKey' #3989

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[i18n-Audio] Add DB schemas and shared API/store stubs #3987

[i18n-Audio] Add DB schemas and shared API/store stubs #3987

kofi-q commented Sep 21, 2023

arsalansufi left a comment

arsalansufi Sep 21, 2023

kofi-q Sep 21, 2023

arsalansufi Sep 21, 2023

kofi-q Sep 21, 2023

arsalansufi Sep 21, 2023

arsalansufi Sep 21, 2023

kofi-q Sep 21, 2023

arsalansufi Sep 21, 2023

kofi-q Sep 21, 2023

arsalansufi Sep 21, 2023

arsalansufi left a comment

	import {
	clearCastVoteRecordHashes,
	getCastVoteRecordRootHash,
	updateCastVoteRecordHashes,
	} from '@votingworks/auth';

	getCastVoteRecordRootHash(): string {
	return getCastVoteRecordRootHash(this.client);
	}

	updateCastVoteRecordHashes(cvrId: string, cvrHash: string): void {
	updateCastVoteRecordHashes(this.client, cvrId, cvrHash);
	}

	clearCastVoteRecordHashes(): void {
	clearCastVoteRecordHashes(this.client);
	}

[i18n-Audio] Add DB schemas and shared API/store stubs #3987

[i18n-Audio] Add DB schemas and shared API/store stubs #3987

Conversation

kofi-q commented Sep 21, 2023

Overview

Testing Plan

Checklist

arsalansufi left a 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 left a comment

Choose a reason for hiding this comment