Add a billing tab to the settings #2811
Conversation
WalkthroughThis update migrates the membership management functionality from the standalone Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AppRouter
participant SettingsController
participant BillingController
participant BillingPageComponents
User->>AppRouter: Navigates to /settings/billing
AppRouter->>SettingsController: Loads settings context
AppRouter->>BillingController: Loads billing model (user, charges)
BillingController->>BillingPageComponents: Passes user and charges
BillingPageComponents->>User: Renders Membership, Renewal, Support, Payment History sections
sequenceDiagram
participant User
participant MemberBadge
participant Router
User->>MemberBadge: Clicks member badge
MemberBadge->>Router: Transitions to 'settings.billing'
Router->>User: Displays billing settings page
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Test Results 1 files ±0 1 suites ±0 7m 59s ⏱️ - 1m 24s Results for commit ac350c2. ± Comparison against base commit 8f14f91. This pull request removes 9 and adds 10 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
📢 Thoughts on this report? Let us know! |
Bundle ReportChanges will decrease total bundle size by 36.58MB (-100.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
app/templates/settings/billing.hbs (1)
20-23: Add handling for empty payment historyThe PaymentHistorySection should handle the case where a user has no payment history. Consider adding a conditional to display an appropriate message when the charges array is empty.
<Settings::FormSection @title="Payment history" @description="Details of your previous payments"> {{#let this.model.charges as |charges|}} - <Settings::BillingPage::PaymentHistorySection @charges={{charges}} /> + {{#if charges.length}} + <Settings::BillingPage::PaymentHistorySection @charges={{charges}} /> + {{else}} + <div class="text-gray-500 py-4"> + No payment history available. + </div> + {{/if}} {{/let}} </Settings::FormSection>app/components/settings/billing-page/payment-history-section.hbs (1)
14-21: Consider adding a tooltip for refund information.For partially refunded charges, consider adding a tooltip that provides additional context about when/why the refund was processed to improve user experience.
{{#if (gt charge.amountRefunded 0)}} {{#if charge.isFullyRefunded}} - <span data-test-refund-text>(refunded)</span> + <span data-test-refund-text title="Fully refunded on {{date-format charge.refundedAt format='PPP'}}">(refunded)</span> {{else}} - <span data-test-refund-text>(<span class="font-semibold">{{charge.refundedAmountDisplayString}}</span> + <span data-test-refund-text title="Partially refunded on {{date-format charge.refundedAt format='PPP'}}">(<span class="font-semibold">{{charge.refundedAmountDisplayString}}</span> refunded)</span> {{/if}} {{/if}}app/components/settings/billing-page/membership-section.hbs (1)
63-65: Consider adding tracking for membership conversions.The "Start membership" buttons are key conversion points. Consider adding analytics tracking to measure user engagement with these buttons.
-<PrimaryLinkButton @size="small" @route="pay" class="mt-3"> +<PrimaryLinkButton @size="small" @route="pay" class="mt-3" data-test-start-membership-button {{on "click" this.trackMembershipButtonClick}}> Start membership → </PrimaryLinkButton>Also applies to: 80-82
tests/acceptance/settings-page/billing-test.js (1)
67-69: Consider mocking email client interaction.Instead of commenting out the contact button test, consider mocking the email client interaction or testing that the correct mailto URL is generated.
- // await billingPage.supportSection.clickContactButton(); - // Commented out because it opens the email client, if behavior changes, uncomment + // Test that the correct mailto URL is used + const contactButton = document.querySelector('[data-test-support-contact-button]'); + assert.ok(contactButton.href.startsWith('mailto:'), 'Contact button has mailto link'); + assert.ok(contactButton.href.includes('subject='), 'Contact button includes subject');mirage/models/charge.js (1)
1-14: Charge model structure looks goodThe model correctly defines the user relationship and includes appropriate fields for a payment charge record.
Consider these improvements:
- Add more detailed documentation about valid values for the
statusfield- Consider whether 'succeeded' is the most appropriate default status (versus 'pending')
- Clarify the comment on line 9 to better explain the specific linting issue
import { Model, belongsTo } from 'miragejs'; export default Model.extend({ user: belongsTo(), amount: 0, amountRefunded: 0, currency: 'usd', createdAt() { - return new Date(); // Done this way due to linting error and shared objects + return new Date(); // Returns a new Date object each time to avoid shared reference issues across instances }, invoiceId: null, - status: 'succeeded', + status: 'succeeded', // Possible values: 'succeeded', 'pending', 'failed', etc. });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/components/settings/billing-page/membership-section.hbs(1 hunks)app/components/settings/billing-page/membership-section.ts(1 hunks)app/components/settings/billing-page/payment-history-section.hbs(1 hunks)app/components/settings/billing-page/payment-history-section.ts(1 hunks)app/components/settings/billing-page/renewal-section.hbs(1 hunks)app/components/settings/billing-page/renewal-section.ts(1 hunks)app/components/settings/billing-page/support-section.hbs(1 hunks)app/components/settings/billing-page/support-section.ts(1 hunks)app/controllers/settings.ts(1 hunks)app/controllers/settings/billing.ts(1 hunks)app/router.ts(1 hunks)app/routes/settings/billing.ts(1 hunks)app/templates/settings/billing.hbs(1 hunks)mirage/models/charge.js(1 hunks)tests/acceptance/settings-page/billing-test.js(1 hunks)tests/pages/settings/billing-page.ts(1 hunks)tests/support/authentication-helpers.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
app/components/settings/billing-page/renewal-section.ts (3)
app/components/settings/billing-page/membership-section.ts (1)
Registry(15-17)app/components/settings/billing-page/payment-history-section.ts (1)
Registry(15-17)app/components/settings/billing-page/support-section.ts (1)
Registry(14-16)
tests/acceptance/settings-page/billing-test.js (1)
tests/support/authentication-helpers.js (4)
signInAsSubscriber(51-61)signIn(1-14)signInAsVipUser(63-69)user(102-102)
app/components/settings/billing-page/payment-history-section.ts (3)
app/components/settings/billing-page/membership-section.ts (1)
Registry(15-17)app/components/settings/billing-page/renewal-section.ts (1)
Registry(10-12)app/components/settings/billing-page/support-section.ts (1)
Registry(14-16)
app/components/settings/billing-page/membership-section.ts (4)
app/models/user.ts (1)
UserModel(28-218)app/components/settings/billing-page/payment-history-section.ts (1)
Registry(15-17)app/components/settings/billing-page/renewal-section.ts (1)
Registry(10-12)app/components/settings/billing-page/support-section.ts (1)
Registry(14-16)
app/components/settings/billing-page/support-section.ts (3)
app/components/settings/billing-page/membership-section.ts (1)
Registry(15-17)app/components/settings/billing-page/payment-history-section.ts (1)
Registry(15-17)app/components/settings/billing-page/renewal-section.ts (1)
Registry(10-12)
🔇 Additional comments (13)
app/controllers/settings.ts (1)
18-18: LGTM! The billing tab is correctly added to the settings controller.This change properly integrates the new billing route into the settings interface navigation, maintaining consistency with the existing tab structure.
app/components/settings/billing-page/renewal-section.ts (1)
1-13: RenewalSectionComponent setup looks solidThe component class and its Glint registry declaration follow the established pattern, matching other billing‐page sections and providing correct typing for the root
<div>. No changes needed here.app/controllers/settings/billing.ts (1)
1-11: BillingController model typing is correctThe
BillingControllerproperly extendsControllerand declares a strongly typedmodelcombiningSettingsModelTypewithcharges: ChargeModel[]. This aligns with the route and ensures type safety.app/components/settings/billing-page/payment-history-section.ts (1)
1-18: PaymentHistorySectionComponent matches the patternThe component signature correctly types
Args.chargesand the registry declaration mirrors other billing‐page sections. Everything is consistent and ready for use in the template.app/components/settings/billing-page/support-section.hbs (1)
1-12: Support section template is well-structuredThe Handlebars markup cleanly renders the mailto link with dynamic
@usernameand the styled support message. It follows accessibility and styling conventions in use elsewhere.app/components/settings/billing-page/membership-section.ts (1)
1-18: MembershipSectionComponent conforms to conventionsThe component signature properly types
Args.user: UserModeland the registry entry is consistent with other billing‐page sections. No further updates required.app/components/settings/billing-page/support-section.ts (1)
3-17: LGTM! Well-structured component signatureThe component signature is well-defined with proper typing for the username argument and correct Glint registry setup for type safety.
app/components/settings/billing-page/payment-history-section.hbs (1)
1-45: Well-structured payment history section with appropriate conditional rendering.The payment history section effectively handles different states (no charges, succeeded/failed charges, and refund scenarios) with clear visual indicators. The conditional logic for displaying "Download Invoice" links or failure messages is implemented correctly.
app/components/settings/billing-page/membership-section.hbs (1)
1-83: Complete and well-structured membership section with clear visual indicators.The membership section effectively handles all user states (VIP, active, inactive, and no membership) with appropriate visual indicators and messaging.
tests/pages/settings/billing-page.ts (1)
1-30: Well-structured page object with appropriate test selectors.The page object correctly implements the necessary selectors and properties for testing the billing page components.
tests/acceptance/settings-page/billing-test.js (1)
1-126: Comprehensive test coverage for billing functionality.The tests cover all major scenarios for the billing page including different membership states and payment history display. Good use of test helpers and Mirage models.
app/router.ts (1)
84-84: LGTM: Clean addition of the billing routeThe addition of the billing route under settings is well-placed and follows the existing routing pattern. This aligns perfectly with the PR objective of adding a billing tab to the settings section.
tests/support/authentication-helpers.js (1)
63-69: LGTM: Well-implemented VIP user authentication helperThe new helper function follows the same pattern as other authentication helpers in the file. It correctly sets the VIP status and expiration date, which will be useful for testing the billing features with different user states.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/components/settings/billing-page/membership-section.hbs (1)
1-96: Move test attribute to parent to avoid duplicationThe
data-test-membership-sectionattribute is currently duplicated in each conditional branch (lines 3, 22, 43, 60, 80). Following the previous review suggestion, move this to the parentFormSectioncomponent.-<Settings::FormSection @title="Membership" @description="Your CodeCrafters membership details"> +<Settings::FormSection @title="Membership" @description="Your CodeCrafters membership details" data-test-membership-section> {{#if (and @user.isVip @user.hasActiveSubscription)}} - <div class="inline-flex items-center gap-2 rounded-full bg-teal-50 px-2 py-1 border border-teal-500" data-test-membership-section> + <div class="inline-flex items-center gap-2 rounded-full bg-teal-50 px-2 py-1 border border-teal-500">And similarly remove it from the other status divs at lines 22, 43, 60, and 80.
🧹 Nitpick comments (2)
app/components/settings/billing-page/membership-section.hbs (2)
21-42: Ensure consistent class usage across status badgesThe active membership badge has an extra class
has-active-planthat isn't present in other badge elements. Either add a comment explaining why this class is only needed here, or ensure consistent class usage across all badge conditions.- <div class="inline-flex items-center gap-2 rounded-full bg-teal-50 px-2 py-1 border border-teal-500 has-active-plan" data-test-membership-section> + <div class="inline-flex items-center gap-2 rounded-full bg-teal-50 px-2 py-1 border border-teal-500" data-test-membership-section>
2-20: Consider consistent messaging about membership between VIP+membership and membership-only sectionsThe text formatting differs between the VIP+membership section and the membership-only section. For consistency, consider using the same format for presenting membership information.
For example, change lines 15-19 to match the format in lines 29-38:
<p> - You are currently subscribed to the - <b class="font-semibold">{{@user.activeSubscription.pricingPlanName}}</b> - plan{{#if @user.activeSubscription.cancelAt}}, valid until - <b class="font-semibold">{{date-format @user.activeSubscription.cancelAt format="PPPp"}}</b>{{/if}}. + {{#if @user.activeSubscription.cancelAt}} + Your CodeCrafters membership is valid until + <b class="font-semibold">{{date-format @user.activeSubscription.cancelAt format="PPPp"}}</b>. + {{else}} + You are currently subscribed to the + <b class="font-semibold">{{@user.activeSubscription.pricingPlanName}}</b> + plan. + {{/if}} </p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/components/settings/billing-page/membership-section.hbs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (2)
app/components/settings/billing-page/membership-section.hbs (2)
2-58:✅ Verification successful
Address VIP and membership interaction concerns
The component correctly prioritizes showing combined VIP+membership status first, then active membership, then VIP-only. However, a previous comment raised concerns about users potentially being confused by seeing only VIP status when they also have purchased a membership that might be expiring.
🏁 Script executed:
#!/bin/bash # Check how the user model handles overlapping VIP and subscription statuses echo "Checking user model properties related to VIP and subscription status:" fd "user.ts" --type file | xargs cat echo "\nChecking how isVip is determined:" rg -A 5 "isVip" --type tsLength of output: 16267
VIP & Membership Display Logic Verified
I’ve confirmed that
isVipis a direct boolean attribute onUserModeland thathasActiveSubscriptionaccurately reflects an active plan. The Handlebars template already prioritizes the combined VIP+membership state, then membership-only, then VIP-only, matching the intended behavior. No changes are needed.
59-79:❓ Verification inconclusive
Verify the expired subscription logic displays correctly
This section correctly uses
@user.expiredSubscriptionas per the previous review comment. However, ensure that expired subscriptions are properly identified and displayed to users, especially when they also have VIP status.
🏁 Script executed:
#!/bin/bash # Check how expiredSubscription is defined and how it interacts with VIP status rg -A 3 -B 3 "expiredSubscription" --type tsLength of output: 379
#!/bin/bash
Show the full expiredSubscription getter
rg -C 10 "get expiredSubscription" app/models/user.ts
Inspect hasActiveSubscription to see VIP interactions
rg -C 5 "get hasActiveSubscription" app/models/user.ts
🏁 Script executed:
#!/bin/bash # Retrieve the full expiredSubscription getter rg -C 10 "get expiredSubscription" app/models/user.ts # Retrieve the full hasActiveSubscription getter for VIP logic context rg -C 10 "get hasActiveSubscription" app/models/user.tsLength of output: 1116
#!/bin/bash
Retrieve the full activeSubscription getter for context
rg -C 10 "get activeSubscription" app/models/user.ts
Search for any VIP-related fields or getters in the User model
rg -i "vip" -C 5 app/models/user.ts
Ensure VIP status is included in the subscription logic
The
expiredSubscriptiongetter only returns a value whenhasActiveSubscriptionis false, buthasActiveSubscriptioncurrently only checks foractiveSubscription. If VIP membership is represented outside ofactiveSubscription, a VIP user with an expired past subscription would still hit the “Membership inactive” UI. Please:
- Review
get activeSubscriptionand any VIP flags (e.g.isVIP) inapp/models/user.ts.- If VIP isn’t surfaced via
activeSubscription, updatehasActiveSubscriptionto include it, for example:get hasActiveSubscription() { return !!this.activeSubscription || this.isVIP; }- Verify in
app/components/settings/billing-page/membership-section.hbsthat VIP users never fall through to the expired-subscription branch.
I've updated to test but window mock isn't working because it doesn't actually change the window location just opens the related app, but this should cover it decently |
| <div class="inline-flex items-center gap-2 rounded-full bg-teal-50 dark:bg-teal-900 px-2 py-1 border border-teal-500 dark:border-teal-400"> | ||
| {{svg-jar "check-circle" class="h-5 w-5 text-teal-500"}} | ||
| <div class="text-teal-500 font-semibold uppercase text-sm"> | ||
| VIP Access + Membership Active |
There was a problem hiding this comment.
This is going to look pretty long & ugly - I'll share a spec on how to refine shortly
There was a problem hiding this comment.
- If a user has an active membership
- Pill is green, says "Membership active"
- Text says "You have access to all CodeCrafters content, valid until XYZ"
- If (a) the user also has VIP access and (b) The VIP access date is longer than the subscription end date, let's keep the above text but cross it out and also say "🎉 You have VIP access to all CodeCrafters content, valid until XYZ".
- Else, If a user is VIP
- Pill is green, says "VIP Access"
- Text says "You have VIP access to...."
- Else, If a user has an expired subscription:
- Pill is red and says "Membership inactive"
- Else, if a user doesn't have any memberships, active or expired
- Pill is yellow and says "No membership found"
We won't use any icons inside the pills - rely solely on color and text to convey meaning.
There was a problem hiding this comment.
Let me know if you think there any cases where this logic would result in confusion from the user's POV btw - can discuss
| </p> | ||
| </div> | ||
| {{else if @user.hasActiveSubscription}} | ||
| <div class="inline-flex items-center gap-2 rounded-full bg-teal-50 px-2 py-1 border border-teal-500 has-active-plan"> |
There was a problem hiding this comment.
Are we still using has-active-plan?
| </PrimaryLinkButton> | ||
| {{else}} | ||
| <div class="inline-flex items-center gap-2 rounded-full bg-yellow-50 px-2 py-1 border border-yellow-500"> | ||
| {{svg-jar "question-mark-circle" class="h-5 w-5 text-yellow-500"}} |
There was a problem hiding this comment.
@Arpan-206 the design for this didn't have SVGs if I'm not wrong, we were relying on just color & text to convey meaning
| </div> | ||
| {{else if @user.expiredSubscription}} | ||
| <div class="inline-flex items-center gap-2 rounded-full bg-red-50 px-2 py-1 border border-red-500"> | ||
| {{svg-jar "x-circle" class="h-5 w-5 text-red-500"}} |
There was a problem hiding this comment.
Same as previous comment, we don't need SVGs here - the color & text is enough to convey the meaning we want, icons would just be distracting here
| </div> | ||
| {{else if this.errorMessage}} | ||
| <div class="bg-red-50 dark:bg-red-900 border border-red-200 dark:border-red-400 rounded-md p-4"> | ||
| <div class="flex"> |
There was a problem hiding this comment.
| <div class="flex"> | |
| <div class="flex gap-3"> |
Instead of using ml-3 on the second child, prefer doing this instead - it's cleaner and prevents mistakes
| <div class="bg-red-50 dark:bg-red-900 border border-red-200 dark:border-red-400 rounded-md p-4"> | ||
| <div class="flex"> | ||
| <div class="flex-shrink-0"> | ||
| {{svg-jar "x-circle" class="h-5 w-5 text-red-400 dark:text-red-300"}} |
There was a problem hiding this comment.
@Arpan-206 this doesn't seem consistent - below I see text-red-700 dark:text-red-300, and here it's text-red-400 dark:text-red-300. Which are we going with?
Usually the direct inverse brightness works for dark mode (300 vs. 700, 800 vs 200), we've only had to make minor +/- 100 changes based on some situations so far.
| </div> | ||
| </div> | ||
| {{else if (gt this.charges.length 0)}} | ||
| <div class="grid grid-cols-3 gap-y-3" data-test-payment-history-section> |
There was a problem hiding this comment.
| <div class="grid grid-cols-3 gap-y-3" data-test-payment-history-section> | |
| <div class="grid grid-cols-3 gap-y-3"> |
(This can go in the outer FormSection component rather than be duplicated)
| } | ||
|
|
||
| export default class SupportSectionComponent extends Component<Signature> { | ||
| get supportEmail() { |
There was a problem hiding this comment.
@Arpan-206 what is this used for?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/pages/settings/billing-page.ts (1)
4-24: 🛠️ Refactor suggestionAdd missing renewalSection to the page object
There appears to be a
renewal-sectioncomponent in the application, but it's not represented in this page object.Add the renewalSection to the page object:
export default createPage({ membershipSection: { scope: '[data-test-membership-section]', }, paymentHistorySection: { charges: collection('[data-test-payment-history-item]', { amount: text('[data-test-amount]'), failed: hasClass('text-red-600'), }), scope: '[data-test-payment-history-section]', }, + renewalSection: { + scope: '[data-test-renewal-section]', + }, + supportSection: { clickOnContactButton: clickable('[data-test-support-contact-button]'), scope: '[data-test-support-section]', }, visit: visitable('/settings/billing'), });Make sure to add the appropriate
data-test-renewal-sectionattribute to the renewal section component's template if it's not already there.🧰 Tools
🪛 ESLint
[error] 15-16: Delete
··⏎··(prettier/prettier)
🧹 Nitpick comments (6)
tests/pages/settings/billing-page.ts (3)
4-7: Consider adding properties to the membershipSectionThe
membershipSectionis currently only scoped but doesn't expose any properties for testing elements within this section. If there are membership status badges or other elements that need testing, consider adding them with appropriate data-test attributes.
9-16: Remove extra whitespace on line 15There's unnecessary whitespace that should be removed to maintain consistent formatting.
charges: collection('[data-test-payment-history-item]', { amount: text('[data-test-amount]'), failed: hasClass('text-red-600'), }), scope: '[data-test-payment-history-section]', - },🧰 Tools
🪛 ESLint
[error] 15-16: Delete
··⏎··(prettier/prettier)
4-24: Consider alphabetical ordering of page object sectionsFor better maintainability and consistency with other files, consider arranging the sections in alphabetical order.
export default createPage({ membershipSection: { scope: '[data-test-membership-section]', }, paymentHistorySection: { charges: collection('[data-test-payment-history-item]', { amount: text('[data-test-amount]'), failed: hasClass('text-red-600'), }), scope: '[data-test-payment-history-section]', }, + renewalSection: { + scope: '[data-test-renewal-section]', + }, + supportSection: { clickOnContactButton: clickable('[data-test-support-contact-button]'), scope: '[data-test-support-section]', }, visit: visitable('/settings/billing'), });🧰 Tools
🪛 ESLint
[error] 15-16: Delete
··⏎··(prettier/prettier)
app/components/settings/billing-page/membership-section.hbs (3)
26-30: Add dark mode support for VIP section badgeThe active subscription badge has dark mode classes but the VIP badge is missing them. For UI consistency, dark mode classes should be added to match other sections.
-<div class="inline-flex items-center gap-2 rounded-full bg-teal-50 px-2 py-1 border border-teal-500"> +<div class="inline-flex items-center gap-2 rounded-full bg-teal-50 dark:bg-teal-900 px-2 py-1 border border-teal-500 dark:border-teal-400">
42-46: Add dark mode support for inactive membership badgeThe inactive membership badge is missing dark mode support classes. For consistency with the active membership section, dark mode classes should be added.
-<div class="inline-flex items-center gap-2 rounded-full bg-red-50 px-2 py-1 border border-red-500"> +<div class="inline-flex items-center gap-2 rounded-full bg-red-50 dark:bg-red-900 px-2 py-1 border border-red-500 dark:border-red-400">
61-65: Add dark mode support for no membership badgeThe "No membership found" badge is missing dark mode support classes. For consistency across all membership status badges, dark mode classes should be added.
-<div class="inline-flex items-center gap-2 rounded-full bg-yellow-50 px-2 py-1 border border-yellow-500"> +<div class="inline-flex items-center gap-2 rounded-full bg-yellow-50 dark:bg-yellow-900 px-2 py-1 border border-yellow-500 dark:border-yellow-400">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
app/components/settings/billing-page/membership-section.hbs(1 hunks)app/components/settings/billing-page/payment-history-section.hbs(1 hunks)app/components/settings/billing-page/payment-history-section.ts(1 hunks)app/components/settings/billing-page/support-section.ts(1 hunks)tests/acceptance/settings-page/billing-test.js(1 hunks)tests/pages/settings/billing-page.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/components/settings/billing-page/payment-history-section.hbs
- app/components/settings/billing-page/support-section.ts
- app/components/settings/billing-page/payment-history-section.ts
- tests/acceptance/settings-page/billing-test.js
🧰 Additional context used
🪛 ESLint
tests/pages/settings/billing-page.ts
[error] 15-16: Delete ··⏎··
(prettier/prettier)
🔇 Additional comments (2)
tests/pages/settings/billing-page.ts (1)
1-24: Good foundation for the billing settings page object!The page object defines the basic structure needed for testing the billing page, with proper data-test selectors for the main sections.
🧰 Tools
🪛 ESLint
[error] 15-16: Delete
··⏎··(prettier/prettier)
app/components/settings/billing-page/membership-section.hbs (1)
1-76: Membership section looks good overallThe membership section correctly implements the requirements from previous review comments:
- Uses the proper conditional logic for different membership states
- Handles the interaction between membership and VIP status appropriately
- Uses color-coded badges and text to convey membership status
- Provides clear calls to action for users without active memberships
The implementation follows the design guidelines and addresses all the feedback from previous reviews.
There was a problem hiding this comment.
Added one more round of comments - think this can be the last, let's aim to wrap up and ship for the next review! Make sure to self-review in detail before opening for review, ensure that there are no inconsistencies.
Also, I still don't see the change where we remove the components used in the old membership page
| <Settings::FormSection @title="Membership" @description="Your CodeCrafters membership details" data-test-membership-section> | ||
| {{#if @user.hasActiveSubscription}} | ||
| <div class="inline-flex items-center gap-2 rounded-full bg-teal-50 dark:bg-teal-900 px-2 py-1 border border-teal-500 dark:border-teal-400"> | ||
| {{svg-jar "circle" class="h-5 w-5 text-teal-500"}} |
There was a problem hiding this comment.
Instead of svg let's just do HTML (h-5 w-5 rounded-full), that's what we use for simple cases like this
| {{/if}} | ||
| </div> | ||
| {{else if @user.isVip}} | ||
| <div class="inline-flex items-center gap-2 rounded-full bg-teal-50 px-2 py-1 border border-teal-500"> |
There was a problem hiding this comment.
@Arpan-206 no dark mode styles. The same green pill above has different styles added.
Maybe you'd want to extract this into a component? Something like BillingPage::MembershipSection::StatusPill?
| Start membership → | ||
| </PrimaryLinkButton> | ||
| {{else}} | ||
| <div class="inline-flex items-center gap-2 rounded-full bg-yellow-50 px-2 py-1 border border-yellow-500"> |
There was a problem hiding this comment.
No dark mode styles (bg-yellow-50 will look extremely bright in dark mode)
| @@ -0,0 +1,63 @@ | |||
| <Settings::FormSection @title="Payment history" @description="Details of your previous payments" data-test-payment-history-section> | |||
| {{#if this.isLoading}} | |||
| <div class="flex justify-center py-8"> | |||
There was a problem hiding this comment.
Let's stay consistent with loading states that we have in other places:
(I searched for isLoading across *.hbs files to find out what we do in other cases)
app/components/settings/billing-page/payment-history-section.ts
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,14 @@ | |||
| <Settings::FormSection @title="Support" @description="How to get help with billing"> | |||
| <a href="mailto:[email protected]?subject=Billing help (account: {{@user.username}})" class="inline-block rounded" data-test-support-section> | |||
There was a problem hiding this comment.
| <a href="mailto:[email protected]?subject=Billing help (account: {{@user.username}})" class="inline-block rounded" data-test-support-section> | |
| <a href="mailto:[email protected]?subject=Billing help (account: {{@user.username}})" class="inline-block rounded"> |
(Place on the outer FormSection, to be consistent with other components)
There was a problem hiding this comment.
This shouldn't be needed, just use HTML with rounded-full
| await billingPage.visit(); | ||
|
|
||
| assert.ok(billingPage.supportSection.isVisible, 'support section is visible'); | ||
| assert.ok(billingPage.supportSection.clickOnContactButton, 'click on contact button'); |
There was a problem hiding this comment.
This isn't asserting anything clickOnContactButton is a function and hence will always be truthy
|
|
||
| assert.ok(billingPage.supportSection.isVisible, 'support section is visible'); | ||
| assert.ok(billingPage.supportSection.clickOnContactButton, 'click on contact button'); | ||
| assert.dom('[data-test-support-section]').hasAttribute('href', /^mailto:[email protected]/); |
There was a problem hiding this comment.
Isn't this just an attribute on the button? Why are we asserting against data-test-support-section here?
There was a problem hiding this comment.
Also, page objects do support checking attributes: https://ember-cli-page-object.js.org/docs/v2.3.x/api/attribute. So you could use that to define something like contactButtonLink or contactButtonHref
Co-authored-by: Paul Kuruvilla <[email protected]>
Checklist:
[percy]in the message to trigger)Summary by CodeRabbit
New Features
Bug Fixes
Chores
/membershipto/settings/billing.Tests