PROD 2450 pay and redirect to thank you page -> Intake form #204
Conversation
src-ts/tools/work/work-lib/work-provider/work-functions/work.functions.ts
Outdated
Show resolved
Hide resolved
|
Something small I noticed, the Country input field says Date instead of Country or Region |
| createAsync as workStoreCreateAsync, | ||
| confirmCustomerPayment as workStoreConfirmCustomerPaymentAsync, |
There was a problem hiding this comment.
This is not about this PR, but about the discussion below about single imports - this is an example of why I think the current structure is a bit obfuscated - we are not only masking where code lives, but renaming it along the way, which means finding the actual source of a function is a multi-step search throughout the codebase.
It is fine for the PR - I did it myself in an earlier PR. Just a discussion worth having at some point.
There was a problem hiding this comment.
yes I agree 👍🏻. I just followed the existing convention here. Personally, the function name aliasing is definitely confusing.
| const [isPaymentFailed, setPaymentFailed]: [boolean, Dispatch<SetStateAction<boolean>>] = useState<boolean>(false) | ||
| const navigate: NavigateFunction = useNavigate() | ||
|
|
||
| const stripe: Stripe | null = useStripe() |
There was a problem hiding this comment.
Curious - Does the ' | null' imply that useStripe is async and might not have a value immediately?
There was a problem hiding this comment.
afaik, it is not async. The stripe type definition define it in a way it can be a Stripe object or null. Possibly the function would return null if the stripe configs are incorrect. If this is an async function they would have made the return type as Promise.
| return workBugHuntConfig.priceConfig.getPrice(workBugHuntConfig.priceConfig, packageType) | ||
| } | ||
|
|
||
| function renderWorkServicePrice(): JSX.Element { |
There was a problem hiding this comment.
Why pull this into a function instead of just wrapping the basicInfo check around the JSX tag?
There was a problem hiding this comment.
@dave-armstrong-topcoder I had to do this to resolve cyclomatic complexity error :(
There was a problem hiding this comment.
I was afraid that was going to be the answer. @casey-tc and I discussed that rule a bit - I believe it is set too low. If we are actually making our code more complex to reduce complexity... something ain't right. 🤣
There was a problem hiding this comment.
@dave-armstrong-topcoder - yup, we will be changing the level in PTC so we are not blocked by errors like this.
| } | ||
|
|
||
| async function onPay(): Promise<any> { | ||
| if (!stripe || !elements || !challenge) { |
There was a problem hiding this comment.
This gets back to why I asked if useStripe was async above - if it can be null, and someone pays while it is still null, it will bail here - but to the user it would look like a button did nothing.
This feels like it would be an extreme edge case for someone to submit that quickly, so it is probably fine. But if there is a realistic chance of it being null, we might want a message to the user of some kind.
What's in this PR?
Screenshots