feat: Add personal org warnings to checkout flow #3896
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #3896 +/- ##
==========================================
- Coverage 98.72% 98.66% -0.06%
==========================================
Files 827 828 +1
Lines 15040 15071 +31
Branches 4306 4312 +6
==========================================
+ Hits 14848 14870 +22
- Misses 185 193 +8
- Partials 7 8 +1
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #3896 +/- ##
==========================================
- Coverage 98.72% 98.66% -0.06%
==========================================
Files 827 828 +1
Lines 15040 15071 +31
Branches 4306 4304 -2
==========================================
+ Hits 14848 14870 +22
- Misses 185 193 +8
- Partials 7 8 +1
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Bundle ReportChanges will increase total bundle size by 6.88kB (0.05%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: gazebo-staging-systemAssets Changed:
Files in
view changes for bundle: gazebo-staging-esmAssets Changed:
Files in
|
Bundle ReportChanges will increase total bundle size by 4.31kB (0.03%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: gazebo-production-esmAssets Changed:
Files in
view changes for bundle: gazebo-production-systemAssets Changed:
Files in
|
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
Sentry detected 1 potential issue in your recent changesThere's a suspicion that `UpdateButton.tsx` at line 78 might encounter an unexpected `TypeError`. This could happen if `getNextBillingDate` returns `null` due to incomplete `accountDetails`, and the non-null assertion operator (`!`) is used, leading to a runtime issue when a string is expected.
Did you find this useful? React with a 👍 or 👎 |
There was a problem hiding this comment.
IMO this is the worst area of this codebase - I'm not going to fix it though because I don't have time 🙃
Pls just review for anything obvious and let's ship it 🚢
|
On it! We are reviewing the PR and will provide feedback shortly. |
PR DescriptionThis pull request refactors the upgrade form to improve user experience and code maintainability. It introduces a step-by-step approach, enhances visual clarity, and reduces code duplication. Click to see moreKey Technical Changes
Architecture Decisions
Dependencies and Interactions
Risk Considerations
Notable Implementation Details
|
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
| <h3 className="font-semibold">Step 2: Choose a billing cycle</h3> | ||
| <div className="inline-flex items-center gap-2"> | ||
| <OptionButton | ||
| type="button" | ||
| active={option} | ||
| onChange={({ text }) => { | ||
| if (text === TimePeriods.ANNUAL) { | ||
| <RadioTileGroup | ||
| value={option} | ||
| onValueChange={(value: OptionPeriod) => { | ||
| if (value === TimePeriods.ANNUAL) { | ||
| setFormValue('newPlan', proPlanYear) | ||
| } else { | ||
| setFormValue('newPlan', proPlanMonth) | ||
| } | ||
|
|
||
| setOption(text) | ||
| setOption(value) | ||
| }} | ||
| options={ | ||
| [ | ||
| { | ||
| text: TimePeriods.ANNUAL, | ||
| }, | ||
| { | ||
| text: TimePeriods.MONTHLY, | ||
| }, | ||
| ] as const | ||
| } | ||
| /> | ||
| > | ||
| <RadioTileGroup.Item value={TimePeriods.ANNUAL} className="w-32"> | ||
| <RadioTileGroup.Label>{TimePeriods.ANNUAL}</RadioTileGroup.Label> | ||
| </RadioTileGroup.Item> | ||
| <RadioTileGroup.Item value={TimePeriods.MONTHLY} className="w-32"> | ||
| <RadioTileGroup.Label>{TimePeriods.MONTHLY}</RadioTileGroup.Label> | ||
| </RadioTileGroup.Item> | ||
| </RadioTileGroup> |
There was a problem hiding this comment.
The RadioTileGroup implementation looks good, but consider extracting the common billing options structure since this pattern is repeated across ProPlan, SentryPlan, and TeamPlan controllers. This would reduce code duplication and make maintenance easier.
There was a problem hiding this comment.
You are correct, but that is out of scope for this change.
| <hr /> | ||
| {/* if you're reading this, I'm sorry */} | ||
| <Card.Content> |
There was a problem hiding this comment.
The comment {/* if you're reading this, I'm sorry */} on line 42 suggests this code structure might not be ideal. Consider refactoring this section to be more maintainable. Also, the <hr /> element should probably be styled with CSS classes for consistency with the design system.
| <hr /> | |
| {/* if you're reading this, I'm sorry */} | |
| <Card.Content> | |
| <Card.Content> | |
| <div className="border-t border-ds-gray-secondary" /> | |
| </Card.Content> |
| const hasOneMember = activatedUserCount + inactiveUserCount === 1 | ||
| return ( | ||
| <p> | ||
| Your organization has {activatedUserCount + inactiveUserCount} members. | ||
| Your organization has {activatedUserCount + inactiveUserCount} member | ||
| {hasOneMember ? '' : 's'}. | ||
| </p> |
There was a problem hiding this comment.
Good improvement with the singular/plural handling! However, consider using a more robust pluralization utility if this pattern is used elsewhere in the codebase. Libraries like pluralize or a custom utility function could handle edge cases better.
There was a problem hiding this comment.
Seems too heavy for our usecase. I can't think of an edge case this doesn't handle
There was a problem hiding this comment.
oh hey, I made a PR against this repo plurals/pluralize#130
| // without this line, the tier selector won't update | ||
| updateParams({ plan: TierNames.PRO }) |
There was a problem hiding this comment.
The import of TierNames should be grouped with other imports from the same module. Also, the comment about updating the tier selector suggests tight coupling that might benefit from refactoring to use a more centralized state management approach.
| // without this line, the tier selector won't update | |
| updateParams({ plan: TierNames.PRO }) | |
| // Update tier selector to reflect the plan change | |
| updateParams({ plan: TierNames.PRO }) |
There was a problem hiding this comment.
Fair but I've decided that is out of scope here.
| <ul className="list-inside list-disc"> | ||
| {diffPlanType && ( | ||
| <li className="pl-2">{`You are changing from the ${ | ||
| currentIsFree ? 'Developer' : currentIsTeam ? 'Team' : 'Pro' | ||
| } plan to the [${selectedIsTeam ? 'Team' : 'Pro'} plan]`}</li> | ||
| )} | ||
| {diffSeats && ( | ||
| <li className="pl-2">{`You are changing seats from ${currentPlan?.planUserCount} to [${seats}]`}</li> | ||
| )} | ||
| {diffBillingType && !currentIsFree && ( | ||
| <li className="pl-2">{`You are changing your billing cycle from ${ | ||
| currentIsAnnual ? 'Annual' : 'Monthly' | ||
| } to [${currentIsAnnual ? 'Monthly' : 'Annual'}]`}</li> | ||
| )} | ||
| </ul> | ||
| <br /> | ||
|
|
||
| <h3 className="font-medium"> |
There was a problem hiding this comment.
Good improvement by wrapping the list items in a proper <ul> element! This enhances accessibility and semantic HTML structure.
|
|
||
| return ( | ||
| <div className="inline-flex"> | ||
| <Button | ||
| data-cy="plan-page-plan-update" | ||
| disabled={disabled} | ||
| type="submit" | ||
| variant="primary" | ||
| hook="submit-upgrade" | ||
| to={undefined} | ||
| > | ||
| {planData?.plan?.isFreePlan ? 'Proceed to checkout' : 'Update'} | ||
| </Button> | ||
| </div> | ||
| <> | ||
| <div className="inline-flex"> | ||
| <Button | ||
| type="button" | ||
| data-cy="plan-page-plan-update" | ||
| disabled={disabled} | ||
| variant="primary" | ||
| hook="confirm-upgrade" | ||
| onClick={() => { | ||
| if (!disabled) { | ||
| setShowConfirmationModal(true) | ||
| } | ||
| }} | ||
| isLoading={isLoading} | ||
| > | ||
| {planData?.plan?.isFreePlan ? 'Proceed to checkout' : 'Update'} | ||
| </Button> | ||
| </div> | ||
| <Modal | ||
| isOpen={showConfirmationModal} | ||
| onClose={() => setShowConfirmationModal(false)} | ||
| body={ | ||
| <div className="flex flex-col gap-4 px-4 py-2"> | ||
| <div className="inline-flex items-center gap-1"> | ||
| By proceeding, you are making the following changes to your plan: | ||
| </div> | ||
| <UpdateBlurb | ||
| currentPlan={planData?.plan} | ||
| newPlan={newPlan} | ||
| seats={Number(seats)} | ||
| nextBillingDate={getNextBillingDate(accountDetails)!} | ||
| /> | ||
| <PersonalOrgWarning /> | ||
| <div className="flex items-center gap-2"> | ||
| <Checkbox | ||
| id="upgrade-confirmation-checkbox" | ||
| checked={confirmationIsChecked} | ||
| onClick={() => setConfirmationIsChecked(!confirmationIsChecked)} | ||
| /> | ||
| <label | ||
| className="flex flex-wrap items-center" | ||
| htmlFor="upgrade-confirmation-checkbox" | ||
| > | ||
| I accept the changes to | ||
| <div className="flex items-center gap-1 pl-1"> | ||
| <Avatar user={ownerData} className="size-4" border="dark" /> | ||
| <span className="font-bold">{owner}</span> | ||
| </div> | ||
| 's plan. | ||
| </label> | ||
| </div> | ||
| <div className="flex justify-end gap-2"> | ||
| <Button | ||
| type="button" | ||
| hook="cancel-upgrade" | ||
| variant="default" | ||
| onClick={() => setShowConfirmationModal(false)} | ||
| > | ||
| Go back | ||
| </Button> | ||
| <Button | ||
| type="submit" | ||
| hook="submit-upgrade" | ||
| variant="primary" | ||
| disabled={!confirmationIsChecked} | ||
| onClick={() => { | ||
| onSubmit() | ||
| setShowConfirmationModal(false) | ||
| }} | ||
| > | ||
| {planData?.plan?.isFreePlan ? 'Proceed to checkout' : 'Update'} | ||
| </Button> | ||
| </div> | ||
| </div> | ||
| } | ||
| title="Review plan changes" |
There was a problem hiding this comment.
The confirmation modal logic is complex and might benefit from being extracted into a custom hook like useUpgradeConfirmation. This would improve testability and reusability.
| <Button | ||
| type="submit" | ||
| hook="submit-upgrade" | ||
| variant="primary" | ||
| disabled={!confirmationIsChecked} |
There was a problem hiding this comment.
The setShowConfirmationModal(false) call inside the submit onClick handler might cause the modal to close before the form submission completes, potentially creating a race condition. Consider handling the modal state in the parent component or after the submission is complete.
| <Button | |
| type="submit" | |
| hook="submit-upgrade" | |
| variant="primary" | |
| disabled={!confirmationIsChecked} | |
| onClick={() => { | |
| setConfirmationIsChecked(false) | |
| onSubmit() | |
| // Modal will be closed by parent component after successful submission | |
| }} |
There was a problem hiding this comment.
The submission and error states are handled by the parent, it's expected that the modal will close right away regardless of successful submission. There is a new spinner on the form submit button to indicate the loading state in the parent.
| <form className="text-ds-gray-default"> | ||
| <Card.Content> | ||
| <PlanTypeOptions | ||
| setFormValue={setFormValue} | ||
| setSelectedPlan={setSelectedPlan} | ||
| newPlan={newPlan} | ||
| /> | ||
| </Card.Content> | ||
| <hr /> | ||
| <Controller | ||
| setSelectedPlan={setSelectedPlan} | ||
| newPlan={newPlan} | ||
| seats={seats} | ||
| setFormValue={setFormValue} | ||
| register={register} | ||
| errors={errors} | ||
| /> | ||
| )} | ||
| </form> | ||
| <Card.Content className="flex flex-col gap-6 pb-6"> | ||
| <PersonalOrgWarning /> | ||
| <UpdateButton | ||
| isValid={isValid} | ||
| newPlan={newPlan} | ||
| seats={seats} | ||
| onSubmit={onSubmit} | ||
| isLoading={isUpgrading} | ||
| /> | ||
| {showPendingUpgradeModal && formData ? ( | ||
| <PendingUpgradeModal | ||
| isOpen={showPendingUpgradeModal} | ||
| onClose={() => setShowPendingUpgradeModal(false)} | ||
| onConfirm={() => { | ||
| setIsUpgrading(true) | ||
| upgradePlan(formData) | ||
| }} | ||
| url={unverifiedPaymentMethods?.[0]?.hostedVerificationUrl || ''} | ||
| isUpgrading={isUpgrading} | ||
| /> | ||
| ) : null} | ||
| </Card.Content> | ||
| </form> |
There was a problem hiding this comment.
The form is no longer wrapped in a <form> element, which could impact accessibility and form submission behavior. Consider wrapping the entire card content in a form element or ensuring proper form semantics are maintained.
|
|
||
| export function PersonalOrgWarning() { | ||
| const { owner } = useParams<URLParams>() | ||
| const { data } = useUser() | ||
| const username = data?.user.username | ||
|
|
||
| if (owner !== username) { | ||
| return null | ||
| } | ||
|
|
||
| return ( | ||
| <Alert variant="info"> | ||
| <Alert.Title> | ||
| You're about to upgrade your personal organization:{' '} | ||
| <span className="font-bold">{username}</span> | ||
| </Alert.Title> | ||
| <Alert.Description> | ||
| If you'd like to upgrade a different organization, click on the | ||
| organization selector at the top left of the page. | ||
| </Alert.Description> | ||
| </Alert> | ||
| ) |
There was a problem hiding this comment.
The component correctly uses early return for better readability. However, consider memoizing this component with React.memo since it only depends on URL params and user data, which don't change frequently.
spalmurray
force-pushed
the
spalmurray/checkout-improvements
branch
from
55ee73d to
9dd285b
Compare
Adds a few changes that draw attention to the org you are about to change the plan of. Most notably, this adds a confirmation modal to the checkout flow and adds a banner letting you know you're about to change the plan of your personal org. This is because we've had several issues caused by users upgrading their personal org by accident. There is another issue that fixes a more core issue, but these are certainly improvements regardless.
Closes codecov/feedback#722
Closes codecov/feedback#651
Related to codecov/engineering-team#3587