Save email preferences changes #4548
Conversation
|
Preview URL 🚀 : https://blurts-server-pr-4548-mgjlpikfea-uk.a.run.app |
|
Preview URL 🚀 : https://blurts-server-pr-4548-mgjlpikfea-uk.a.run.app |
| function isEmailUpdateCommType( | ||
| value: string, | ||
| ): value is EmailUpdateCommTypeOfOptions { | ||
| return ["null", "affected", "primary"].includes(value); |
There was a problem hiding this comment.
Q: Should we create an enum for these since we seem to semi-duplicate some values here and on L42+L45?
There was a problem hiding this comment.
Also, are there other possible values for value that aren't one of these three?
There was a problem hiding this comment.
TypeScript enums are incompatible with the enum proposal for JS and changes runtime behaviour, and unions of literal strings (which we use here - EmailUpdateCommTypeOfOptions) provide the same type safety, so it's recommended to use those.
That type also indicates that indeed, these are the only three valid values :)
That said... See my other comments.
| function isEmailUpdateCommType( | ||
| value: string, | ||
| ): value is EmailUpdateCommTypeOfOptions { | ||
| return ["null", "affected", "primary"].includes(value); |
There was a problem hiding this comment.
TypeScript enums are incompatible with the enum proposal for JS and changes runtime behaviour, and unions of literal strings (which we use here - EmailUpdateCommTypeOfOptions) provide the same type safety, so it's recommended to use those.
That type also indicates that indeed, these are the only three valid values :)
That said... See my other comments.
| @@ -48,8 +48,13 @@ export async function POST(req: NextRequest) { | |||
| default: | |||
| allEmailsToPrimary = null; | |||
| } | |||
| await setAllEmailsToPrimary(subscriber, allEmailsToPrimary); | |||
| await setMonthlyMonitorReport(subscriber, monthlyMonitorReport); | |||
|
|
|||
There was a problem hiding this comment.
I can't leave a comment on the relevant lines because they're unchanged, but it's best to update the type representing the request body to indicate that one of the properties should now be left empty:
export interface EmailUpdateCommOptionRequest {
instantBreachAlerts?: EmailUpdateCommTypeOfOptions;
monthlyMonitorReport?: boolean;
}And then TS will show you what code to wrap in an if (typeof <x> !== "undefined"), and you won't need the custom type guard (isEmailUpdateCommType).
| const body: EmailUpdateCommOptionRequest = { | ||
| instantBreachAlerts: chosenOption, | ||
| monthlyMonitorReport: monitorReportAllowed, | ||
| }; |
There was a problem hiding this comment.
If you leave this in and make the change to the type that I suggested in the other file, then if people modify the backend, TS will warn us that we'll have to update the frontend as well.
(In fact, we probably want to do the same for the call to update monthlyMonitorReport, since we have no checks that we're matching the request body type there either.)
| const body: EmailUpdateCommOptionRequest = { | |
| instantBreachAlerts: chosenOption, | |
| monthlyMonitorReport: monitorReportAllowed, | |
| }; | |
| const body: EmailUpdateCommOptionRequest = { | |
| instantBreachAlerts: chosenOption, | |
| }; |
|
Preview URL 🚀 : https://blurts-server-pr-4548-mgjlpikfea-uk.a.run.app |
| if (typeof instantBreachAlerts !== "undefined") { | ||
| await setAllEmailsToPrimary(subscriber, allEmailsToPrimary); | ||
| } | ||
| if (typeof monthlyMonitorReport === "boolean") { |
There was a problem hiding this comment.
@Vinnl Can the condition here be !== undefined too?
There was a problem hiding this comment.
It ended up just being an issue of passing the wrong var! my bad
| if (typeof instantBreachAlerts !== "undefined") { | ||
| await setAllEmailsToPrimary(subscriber, allEmailsToPrimary); | ||
| } | ||
| if (typeof monthlyMonitorReport === "boolean") { |
|
Cleanup completed - database 'blurts-server-pr-4548' destroyed, cloud run service 'blurts-server-pr-4548' destroyed |
References:
Jira: MNTOR-3168
Description
Screenshot (if applicable)
Not applicable.
How to test
Checklist (Definition of Done)