Migrate the CreateSalesforceContact Lambda to Typescript #7140
Conversation
rupertbates
force-pushed
the
rb/createSalesforceContact
branch
from
8d7f7bb to
92377c5
Compare
|
Size Change: +15 B (0%) Total Size: 1.53 MB ℹ️ View Unchanged
|
rupertbates
force-pushed
the
rb/createSalesforceContact
branch
from
92377c5 to
7d230d7
Compare
rupertbates
force-pushed
the
rb/createSalesforceContact
branch
from
7d230d7 to
0888a30
Compare
rupertbates
force-pushed
the
rb/createSalesforceContact
branch
2 times, most recently
from
e7787e8 to
52a17e0
Compare
rupertbates
changed the title
rupertbates
force-pushed
the
rb/createSalesforceContact
branch
2 times, most recently
from
75dc2c6 to
cccfd74
Compare
rupertbates
changed the title
| @@ -249,7 +249,7 @@ export class SupportWorkers extends GuStack { | |||
| "CreatePaymentMethod" | |||
| ).addCatch(failureHandler, catchProps); | |||
|
|
|||
| const createSalesforceContactLambda = createScalaLambda( | |||
| const createSalesforceContactLambda = createTypescriptLambda( | |||
There was a problem hiding this comment.
Great that this is a one line change in the CDK!
| case 'Contribution': | ||
| return { | ||
| productSpecificState: { | ||
| productType: 'Contribution', |
There was a problem hiding this comment.
Would it be worth using state.product.productType to avoid duplicating the value? Applies to each branch of this switch statement, and is consistent with how we're retrieving other fields from state.
There was a problem hiding this comment.
Actually there is a difference unfortunately, productType: 'DigitalPack' & createZuoraSubscriptionProductState.productType: 'DigitalSubscription' don't match up. It would be good to refactor that out at some point but not in this PR.
There was a problem hiding this comment.
Ohh sorry I didn't spot that, yeah sounds like a future thing 👍
| salesForceContact: contactRecords.buyer, | ||
| similarProductsConsent: state.similarProductsConsent, | ||
| }, | ||
| ...state, |
There was a problem hiding this comment.
There are a couple of pieces which are common to all branches of this switch - nesting the product specific state under productSpecificState and merging with state at the top level. Would it be worth building up the product specific state separately, and then doing the common pieces in one place which applies to all products?
There was a problem hiding this comment.
I had a go at doing this but ran into difficulties convincing TS that the product returned from the sub function was the same type as the one in the calling function. It didn't save much code anyway because we still need the specific productType as mentioned in a previous comment, so I've left it as is.
| @@ -5,21 +5,23 @@ export const countrySchema = z.enum(isoCountries); | |||
|
|
|||
| export const addressSchema = z.object({ | |||
| lineOne: z.string().nullable(), | |||
| lineTwo: z.string().nullable(), | |||
| lineTwo: z.string().nullish(), | |||
There was a problem hiding this comment.
So this can be null or undefined, unlike the others which can only be null?
There was a problem hiding this comment.
Yes, I want to rework how we're using null and undefined across the whole model in a separate PR because Scala writes options as null but I think it would be neater and more idiomatic to use undefined in the Typescript.
Nullish should let us do that I think but I didn't want to confuse this PR with too many changes.
rupertbates
force-pushed
the
rb/createSalesforceContact
branch
from
cccfd74 to
7a97cfe
Compare
|
Seen on PROD (merged by @rupertbates 9 minutes and 57 seconds ago)
Sentry Release: support-client-side, support |
What are you doing in this PR?
This PR follows on from #7017 and is the next step in moving support-workers from Scala to Typescript.
This is a simpler lambda than the CreatePaymentMethod lambda and most of the schemas and model classes were already in place so this PR is smaller than previous ones.
TODO:
Trello Card
How to test