MNTOR-3435 - Convert Breaches.js to Typescript #4876

codemist · 2024-07-26T13:15:40Z

References:

Jira: MNTOR-3435

Description

Screenshot (if applicable)

Not applicable.

How to test

Checklist (Definition of Done)

Localization strings (if needed) have been added.
Commits in this PR are minimal and have descriptive commit messages.
I've added or updated the relevant sections in readme and/or code comments
I've added a unit test to test for potential regressions of this bug.
If this PR implements a feature flag or experimentation, the Ship Behind Feature Flag status in Jira has been set
Product Owner accepted the User Story (demo of functionality completed) or waived the privilege.
All acceptance criteria are met.
Jira ticket has been updated (if needed) to match changes made during the development process.
Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.

codemist · 2024-07-26T13:17:01Z

src/utils/breaches.ts

+    if (useBreachId) {
+      breach.IsResolved = breachResolutionV2[breach.Id]?.isResolved;
+      breach.ResolutionsChecked =
+        breachResolutionV2[breach.Id]?.resolutionsChecked || [];
+    } else {
+      // TODO: remove after MNTOR-978
+      breach.IsResolved = breachResolutionV2[breach.recencyIndex]?.isResolved;
+      breach.ResolutionsChecked =
+        breachResolutionV2[breach.recencyIndex]?.resolutionsChecked || [];
+    }
+


@Vinnl Had an issue here where the expected type is HibpLikeDbBreach but I think this expects the SubscriberBreach object instead, which would contain the IsResolved etc. property. Is this something worth refactoring?

Yeah, I think instead of trying to modify the foundBreachesWithRecency array (of type HibpLikeDbBreach[]) in place to try to make it look like SubscriberBreach[], it's probably best to just create a new array of SubscriberBreaches.

codemist · 2024-07-29T14:20:31Z

src/app/functions/server/breachResolution.ts

@@ -101,7 +101,7 @@ function appendBreachResolutionChecklist(
  for (const { breaches } of verifiedEmails) {


@Vinnl I had to rebase this code, could you check that it did not affect the changes in the hibp.js typescript migration?

I added ec002a1 to restore a couple of changes that were resolved the wrong way around.

FWIW, if I have a merge conflict after rebasing from main, I simply look at the code that is in main - if the conflict is different from main and is not a change that I intentionally made as part of the PR, I restore it back to what's currently in main.

github-actions · 2024-07-29T15:28:20Z

Preview URL 🚀 : https://blurts-server-pr-4876-mgjlpikfea-uk.a.run.app

rhelmer · 2024-07-29T17:38:37Z

src/utils/breaches.ts

+
+  const useBreachId = user.breach_resolution?.useBreachId;
+
+  // Not entirely sure what this section of the code is doing, but it's not typed


From git history, looks like this is backwards-compat code for #2757 - @mansaj should we keep this in place or can it be removed now? If it needs to stay I think having a comment here to explain the situation would be good.

@mansaj Keeping it here but explicitly typing it as any, unless you believe that can be safely removed.

This recency code is no longer necessary, though we can refactor in another PR (as we talked about in the tech meeting)

@mansaj Removed the recency code in this commit b0826a3

This removes the `Breach` type in functions/universal/breaches, which was created when first introducing TypeScript and the flow of data was still unclear, but by now had overlap with other types and no clear provenance. Instead, there are now three breach-related types, that represent where the data came from: - HibpGetBreachesResponse: this is an array of breach elements as returned from the HIBP API, unprocessed. Properties are in PascalCase, so are a breach's data classes. - BreachRow: this is a breach's data as stored in our database, along with some data we added to it, such as a favicon URL. Properties are snake_case, and data classes are lowercased and kebab-cased by the formatDataClassesArray function. - HibpLikeDbBreach: this is a breach's data fetched from the database, but stored in an object meant to look like the ones in HibpGetBreachesResponse. In other words, it contains the same data as BreachRow (including lowercased, kebab-cased data classes), but on PascalCase properties. The latter is somewhat of a historical artefact, because we used to try to load breaches from our database, then if our database didn't contain any breaches yet, fetch them live from the HIBP API and continue working with that. We no longer do that: now, even after fetching them from the HIBP API, we do a new query to get them from the database and process them into HibpLikeDbBreach, so that we can assume a consisent data structure everywhere we work with breaches.

rhelmer

lgtm I only have a few comment-related suggestions.

rhelmer · 2024-08-03T05:08:48Z

src/utils/breaches.ts

+
+  // get new breaches since last shown
+  for (const emailEntry of verifiedEmails) {
+    // /** @type {any[]} */


Can this comment be removed now?

rhelmer · 2024-08-03T05:09:40Z

src/utils/breaches.ts

+
+function addRecencyIndex(foundBreaches: HibpLikeDbBreach[]) {
+  // /**
+  //  * @type {any[]}


Same question re: this type definition - we were using these JSDoc type annotations to get a little type checking for JS files before we fully switched to typescript but we shouldn't need them anymore.

rhelmer · 2024-08-03T05:13:40Z

src/utils/breaches.ts

+    console.error(errMsg);
+    captureMessage(errMsg);
+
+    // @ts-ignore: function will be deprecated


If you put @deprecated in a comment above the function definition then VSCode and other tooling will pick that up.

Heads-up that that needs to be in a JSDoc comment, i.e. one that starts with /** rather than just /* or //.

github-actions · 2024-08-05T12:37:07Z

Cleanup completed - database 'blurts-server-pr-4876' destroyed, cloud run service 'blurts-server-pr-4876' destroyed

codemist changed the base branch from main to MNTOR-3101-ts-utils-hibp July 26, 2024 13:15

codemist commented Jul 26, 2024

View reviewed changes

codemist requested a review from Vinnl July 26, 2024 13:17

Vinnl force-pushed the MNTOR-3101-ts-utils-hibp branch from 0cc2c74 to afdb5d1 Compare July 29, 2024 12:01

Base automatically changed from MNTOR-3101-ts-utils-hibp to main July 29, 2024 12:08

codemist force-pushed the MNTOR-3435-breaches branch from 1808876 to 2a64cd3 Compare July 29, 2024 14:19

codemist marked this pull request as ready for review July 29, 2024 14:19

codemist requested review from rhelmer and mansaj July 29, 2024 14:19

codemist commented Jul 29, 2024

View reviewed changes

codemist force-pushed the MNTOR-3435-breaches branch from a948e05 to d9197e7 Compare July 29, 2024 15:20

rhelmer requested changes Jul 29, 2024

View reviewed changes

Vinnl and others added 8 commits August 2, 2024 07:47

MNTOR-3435 - breaches js to ts

6122347

remove old typedef

056ce4b

explicitly type breach as any

5f8a670

update path

d91e107

update breaches path

34a4a60

Fix mistaken conflict resolutions

114673b

remove recency code

b0826a3

codemist force-pushed the MNTOR-3435-breaches branch from fbcdfcb to b0826a3 Compare August 2, 2024 11:50

update fxa path link

aa84eb3

codemist requested a review from rhelmer August 2, 2024 11:57

rhelmer approved these changes Aug 3, 2024

View reviewed changes

codemist added 2 commits August 5, 2024 08:24

remove comments

104824e

Merge branch 'main' into MNTOR-3435-breaches

87f98de

codemist merged commit 324cebb into main Aug 5, 2024
15 checks passed

codemist deleted the MNTOR-3435-breaches branch August 5, 2024 12:35

flozia mentioned this pull request Aug 9, 2024

Fix resolving breaches for secondary emails (MNTOR-3495) #4935

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MNTOR-3435 - Convert Breaches.js to Typescript #4876

MNTOR-3435 - Convert Breaches.js to Typescript #4876

codemist commented Jul 26, 2024

codemist Jul 26, 2024

Vinnl Jul 29, 2024

codemist Jul 29, 2024

Vinnl Jul 30, 2024

github-actions bot commented Jul 29, 2024

rhelmer Jul 29, 2024

codemist Jul 30, 2024 •

edited

Loading

mansaj Jul 30, 2024

codemist Aug 2, 2024 •

edited

Loading

rhelmer left a comment

rhelmer Aug 3, 2024

rhelmer Aug 3, 2024

rhelmer Aug 3, 2024 •

edited

Loading

Vinnl Aug 4, 2024

github-actions bot commented Aug 5, 2024

		@@ -101,7 +101,7 @@ function appendBreachResolutionChecklist(
		for (const { breaches } of verifiedEmails) {


		const useBreachId = user.breach_resolution?.useBreachId;

		// Not entirely sure what this section of the code is doing, but it's not typed

MNTOR-3435 - Convert Breaches.js to Typescript #4876

MNTOR-3435 - Convert Breaches.js to Typescript #4876

Conversation

codemist commented Jul 26, 2024

References:

Description

Screenshot (if applicable)

How to test

Checklist (Definition of Done)

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

github-actions bot commented Jul 29, 2024

Choose a reason for hiding this comment

codemist Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codemist Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

rhelmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhelmer Aug 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 5, 2024

codemist Jul 30, 2024 •

edited

Loading

codemist Aug 2, 2024 •

edited

Loading

rhelmer Aug 3, 2024 •

edited

Loading