feat(fxa-admin-panel): add email bounce diagnosticCode to FXA Admin Panel #12551

millmason · 2022-04-18T21:35:45Z

Because

We want to have the diagnostic code from an email bounce available in the fxa admin panel. Currently, it only shows up in the logs.

This pull request

Adds a diagnosticCode column to the emailBounces panel
Adds a diagnosticCode key onto the appropriate models/gql queries
Sets diagnosticCode as a nullable, optional value (to safely allow for querying past records which will not have this value.)
Adds an optional --hasDiagnosticCode flag to the script for generating fake email bounces (fxa/packages/fxa-admin-server/scripts/email-bounce.ts). This allowed me to create email bounce records missing the new field, to make sure they would display properly on the frontend.
Displays the diagnostic code for an email bounce record within the fxa admin panel, if present on the record.
Updates the README that describes generating test email bounces

Issue that this pull request solves

Closes: #12137

Checklist

Put an x in the boxes that apply

My commit is GPG signed.
If applicable, I have modified or added tests which pass locally.
I have added necessary documentation (if appropriate).
I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Before:
Has the same data as the "after" image (i.e., a mix of email bounce records, some with diagnostic codes and some without) but does not display any of them.

After:
Here we can see diagnostic codes when they're present on the record.

Other information (Optional)

Do you think we should explicitly say when there is no diagnostic code present on the record? For example, it might be clearer if, instead of hiding the field when the diagnostic code is absent, we put something like diagnostic code: none present on record or something like that.

I'd also especially appreciate eyes on the database and gql code.

There are some code scanning checks that are failing (for lines I didn't alter) but it looks like the alert is for "Cryptographically insecure randomness in a security context" and it seems to be pointing at randomness used in tests (as opposed to something security-related in production) so I think we can dismiss that alert.

Finally, I'd like opinions on whether there should be specific test coverage of this. It didn't look to me like we were testing other individual fields on the emailBounce queries, but I'm open to other opinions.

dschom · 2022-04-21T17:57:14Z

@millsoper Concerning the diagnostic code, I'd favor explicitly stating it. Since this is an admin panel, I think a more verbose UX is better. Concerning the security warnings, since these are not lines you altered, so it shouldn't be a concern of this PR. Concerning further test coverage, if I see spots were we could improve coverage, I'll call this out in the code as I review it, thanks for bring this to attention though.

millmason · 2022-04-21T18:57:37Z

@dschom what would you think of using the ResultListItem component here to clean up the list of Email Bounce fields? It's pretty much a perfect fit, except for the fact that it expects a test id, which in the current format would go in as undefined. It's really not necessary but seemed like a nice little clean-up, since I'm adjusting that list anyway

dschom

Nice work! For the time being, I am going ahead and posting some nitpicks. I still need to run some tests and do a bit of exploratory testing, but I think this PR is basically ready to go. I will do another pass after lunch.

dschom · 2022-04-21T18:02:27Z

packages/fxa-admin-server/scripts/email-bounce.ts

-      bounce = randomEmailBounce(argv.email as string);
+      bounce = randomEmailBounce(
+        argv.email as string,
+        argv.withDiagnosticCode as boolean


Should withDiagnosticCode be added to yargs.options on line 17?

dschom · 2022-04-21T18:12:48Z

packages/fxa-admin-server/src/schema.gql

@@ -36,6 +36,7 @@ type EmailBounce {
  bounceType: BounceType!
  bounceSubType: BounceSubType!
  createdAt: Float!
+  diagnosticCode: String!


In the graphql.ts diagnosticCode is nullable. :String! indicates the field is not nullable whereas a type of :String would be nullable. Perhaps this should be consistent between the typescript and the graphql schema?

It definitely should, thanks!

dschom · 2022-04-21T18:20:46Z

packages/fxa-shared/db/models/auth/email-bounce.ts

  }: Pick<EmailBounce, 'email'> & {
    templateName: string;
    bounceType: BounceType;
    bounceSubType: BounceSubType;
+    diagnosticCode?: string;


Random nit, you could also define diagnosticCode in the Pick, which is a more type centric way of declaring this, e.g. Pick<EmailBounce, 'email' | 'diagnosticCode'>. Note that the set of unioned types here are either not present on EmailBounce, or they are being represented with a more specific type, whereas diagnosticCode's type is already aligned with EmailBounce.diagnosticCode.

dschom · 2022-04-21T18:23:12Z

packages/fxa-shared/test/db/models/auth/email-bounces.sql

@@ -4,5 +4,6 @@ CREATE TABLE `emailBounces` (
  `bounceType` tinyint(3) unsigned NOT NULL,
  `bounceSubType` tinyint(3) unsigned NOT NULL,
  `createdAt` bigint(20) unsigned NOT NULL,
+  `diagnosticCode` varchar(255) NOT NULL,


Just checking on state again. This collumn is explictily being declared NOT NULL, yet in graphql.ts the field is marked as Nullable.

Ahhh that's an error, I'll fix that. Thank you!!

dschom · 2022-04-21T18:32:14Z

packages/fxa-shared/test/db/models/auth/helpers.ts

-export function randomEmailBounce(email: string): BounceIsh {
+export function randomEmailBounce(
+  email: string,
+  withDiagnosticCode: boolean = false


So I feel a bit conflicted suggesting this, since IMO randomness in tests is bad. However, this is already the approach being used, so I would suggest leaving the daignosticCode up to chance as well. As far as I can tell, nothing ever sets withDiagnosticCode to true, so this path isn't actually being invoked or tested at the moment.

Ohh I see what you mean -- I set it as the default here because it's being used in the fxa-admin-server script that we use for manually faking bounces, so I was thinking of it more as a dev tool than a test helper, and setting the default to false allowed it to keep being used the way it had been. It is confusing though. Would it be clearer if I added randomization to the tests? Or should I drop this altogether?

…e obj feat(fxa-admin-panel): add diagnosticCode to auth bounce model feat(fxa-admin-panel): add diagnosticCode col to emailBounces table feat(fxa-admin-panel): fix patch to include procedures and use them feat(fxa-admin-panel): update helpers/email bounce faker to include diagnostic code feat(fxa-admin-panel): add diagnostic code to admin panel display feat(fxa-admin-panel): make diagnosticCode field optional chore(fxa-admin-server) update README with new script flag feat(fxa-admin-panel) explicitly show lack of diagnostic code if none present fix(fxa-admin-panel) fix types for diagnostic code fix(fxa-admin-server) gql type fixes fix(fxa-admin-server) gql fix(fxa-admin-panel) set diagnosticCode default value to empty string

dschom · 2022-04-26T19:37:40Z

packages/db-migrations/databases/fxa/patches/patch-126-127.sql

+CALL assertPatchLevel('126');
+
+ALTER TABLE emailBounces
+ADD COLUMN diagnosticCode VARCHAR(255) DEFAULT '',


dschom

LGTM!

millmason changed the title ~~4700 add diagnosticcode~~ Add email bounce diagnosticCode to FXA Admin Panel Apr 18, 2022

millmason marked this pull request as ready for review April 18, 2022 22:36

millmason requested a review from a team as a code owner April 18, 2022 22:36

millmason changed the title ~~Add email bounce diagnosticCode to FXA Admin Panel~~ feat(fxa-admin-panel): add email bounce diagnosticCode to FXA Admin Panel Apr 19, 2022

dschom self-requested a review April 21, 2022 17:44

dschom reviewed Apr 21, 2022

View reviewed changes

millmason force-pushed the 4700-add-diagnosticcode branch from 9063344 to 523c87c Compare April 25, 2022 22:27

millmason closed this Apr 25, 2022

millmason reopened this Apr 25, 2022

millmason force-pushed the 4700-add-diagnosticcode branch from 523c87c to 24e4b40 Compare April 26, 2022 17:09

dschom reviewed Apr 26, 2022

View reviewed changes

dschom self-requested a review April 26, 2022 19:41

dschom approved these changes Apr 26, 2022

View reviewed changes

millmason merged commit 3a1c36f into main Apr 26, 2022

millmason deleted the 4700-add-diagnosticcode branch May 4, 2022 18:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(fxa-admin-panel): add email bounce diagnosticCode to FXA Admin Panel #12551

feat(fxa-admin-panel): add email bounce diagnosticCode to FXA Admin Panel #12551

millmason commented Apr 18, 2022 •

edited

Loading

dschom commented Apr 21, 2022

millmason commented Apr 21, 2022

dschom left a comment

dschom Apr 21, 2022

dschom Apr 21, 2022

millmason Apr 21, 2022

dschom Apr 21, 2022

dschom Apr 21, 2022

millmason Apr 21, 2022

dschom Apr 21, 2022

millmason Apr 21, 2022

dschom Apr 26, 2022

dschom left a comment

feat(fxa-admin-panel): add email bounce diagnosticCode to FXA Admin Panel #12551

feat(fxa-admin-panel): add email bounce diagnosticCode to FXA Admin Panel #12551

Conversation

millmason commented Apr 18, 2022 • edited Loading

Because

This pull request

Issue that this pull request solves

Checklist

Screenshots (Optional)

Other information (Optional)

dschom commented Apr 21, 2022

millmason commented Apr 21, 2022

dschom 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

dschom left a comment

Choose a reason for hiding this comment

millmason commented Apr 18, 2022 •

edited

Loading