Extract form submission code out of the checkout component #6805
Conversation
rupertbates
force-pushed
the
refactor-checkout-component
branch
from
2bba0da to
75f7253
Compare
|
Size Change: +258 B (+0.01%) Total Size: 1.94 MB ℹ️ View Unchanged
|
rupertbates
force-pushed
the
refactor-checkout-component
branch
from
53313f5 to
5475bb6
Compare
rupertbates
changed the title
rupertbates
changed the title
| @@ -248,7 +168,7 @@ export function CheckoutComponent({ | |||
| abParticipations, | |||
| }: CheckoutComponentProps) { | |||
| /** we unset any previous orders that have been made */ | |||
| unsetThankYouOrder(); | |||
| // unsetThankYouOrder(); TODO: after refactor this clears the session state after the submit is finished but before we get to the thank you page which causes an error | |||
There was a problem hiding this comment.
I don't think we need this? Does anyone know why it was added?
There was a problem hiding this comment.
I'm not sure why this was originally added tbh, we use the thankYouOrder item in session storage to retrieve info about the order to be rendered on the TY page such as firstName, email and paymentMethod. The only reason to unset AFAICT would be to prevent rendering incorrect info on the TY page if they order two products in the same session? However I can't see how this scenario would be possible as we call setThankYouOrder on form submission, so it should always have the latest correct details by the time a user lands on the TY page: `https://github.com/guardian/support-frontend/blob/pd/one-time-checkout-email-verification/support-frontend/assets/pages/%5BcountryGroupId%5D/components/checkoutComponent.tsx#L638
There was a problem hiding this comment.
Yes that was the conclusion I came to as well
| } | ||
| }; | ||
|
|
||
| const createNewSupportWorkersExecution = async ( |
There was a problem hiding this comment.
Would it make sense to move these methods around creating the support workers request and polling for the status to a separate file?
| } | ||
| }; | ||
|
|
||
| const goToThankYouPage = ( |
There was a problem hiding this comment.
Not necessarily for now, but perhaps in future the return value of submitForm could be used to represent success or failure to the caller? The success status could even include the URL to redirect to, e.g.
{
status: "success",
redirectPath: "/uk/thank-you..."
}
Then we'd decouple this code from needing to know about the window.
| ), | ||
| ); | ||
| setIsProcessingPayment(false); | ||
| setErrorMessage('Sorry, something went wrong'); |
There was a problem hiding this comment.
I wonder whether we should log errors that aren't FormSubmissionError in Sentry? I don't think we'd want to do that for FormSubmissionError errors as they're known errors that could occur and are handled with meaningful messages displayed.
What are you doing in this PR?
This PR moves the logic related to submitting the generic checkout form to the server and handling the response, out of checkoutComponent.tsx and into separate files.
Why are you doing this?
checkoutComponent.tsx currently contains several unrelated concerns from presentation through to the server interaction code mentioned above. By separating them out the aim is to apply the single responsibility principle - that "A module should be responsible to one, and only one, actor".
The extracted functions are fairly different to the original version because the original used React hooks extensively, so to move the logic out of the component and to introduce a separation of concerns, I have had to refactor that dependency on React out of the code.
Testing
I have deployed this to CODE and run the E2E smoke tests against it successfully