Add a billing tab to the settings #2811
Conversation
WalkthroughThis update introduces a new "Billing" section within the application settings, replacing the previous "Membership" page and associated routes. It adds new routes, controllers, templates, and components to manage and display billing, membership status, payment history, renewal information, and support options. The navigation and redirection logic throughout the application has been updated to reference the new billing section instead of the old membership route. Associated tests, page objects, and helper functions have been added or updated to reflect these changes, while obsolete membership-related files and tests have been removed. A permanent redirect from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Router
participant SettingsBillingRoute
participant BillingController
participant BillingPageComponents
User->>Router: Navigate to /settings/billing
Router->>SettingsBillingRoute: Enter billing route
SettingsBillingRoute->>SettingsBillingRoute: Check authentication
SettingsBillingRoute->>BillingController: Provide user model
BillingController->>BillingPageComponents: Pass model/user to components
BillingPageComponents->>User: Render membership, payment, renewal, support sections
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)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 8m 35s ⏱️ +42s Results for commit 5e42545. ± 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 177 bytes (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: client-array-pushAssets Changed:
Files in
|
![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.
Arpan-206
force-pushed
the
arpan/cc-1703-revamp-membership-page
branch
from
78a7cb9 to
4586692
Compare
|
@rohitpaulk for now the get help button just opens an email client but I assume we'd want a different action. Can you tell me what |
| @@ -23,7 +23,6 @@ | |||
| {{else}} | |||
| <Header::AccountDropdownLink @text="Subscribe" @icon="credit-card" {{on "click" (fn this.handleSubscribeClick dd.actions)}} /> | |||
| {{/if}} | |||
|
|
|||
There was a problem hiding this comment.
@Arpan-206 make sure to review and remove unrelated changes from a PR - we usually do keep empty lines between blocks where indentation changes for readability
app/components/settings/billing-page/payment-history-section.hbs
Outdated
Show resolved
Hide resolved
app/components/settings/billing-page/payment-history-section.hbs
Outdated
Show resolved
Hide resolved
Co-authored-by: Paul Kuruvilla <rohitpaulk@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/templates/settings/billing.hbs (2)
3-3: Pass auto-renewal data to the RenewalSection componentThe RenewalSection component is rendered without any arguments, but it should display different UI states based on whether auto-renewal is enabled or not.
Consider passing the appropriate auto-renewal data:
-<Settings::BillingPage::RenewalSection /> +<Settings::BillingPage::RenewalSection + @autoRenewEnabled={{@model.user.autoRenewEnabled}} + @renewalDate={{@model.user.membershipExpiresAt}} +/>You'll need to update the RenewalSection component to accept these arguments.
1-7: 🛠️ Refactor suggestionConsider moving form section wrappers into the respective components
The current structure passes only the user model to components, but lacks the form section wrappers that would provide proper titles and descriptions for each section.
For consistency with
settings/profile.hbsand better component encapsulation, move theSettings::FormSectionwrappers into each component:-<Settings::BillingPage::MembershipSection @user={{@model.user}} /> -<Settings::FormDivider /> -<Settings::BillingPage::RenewalSection /> -<Settings::FormDivider /> -<Settings::BillingPage::SupportSection @user={{@model.user}} /> -<Settings::FormDivider /> -<Settings::BillingPage::PaymentHistorySection @user={{@model.user}} /> +<Settings::BillingPage::MembershipSection @user={{@model.user}} /> +<Settings::BillingPage::RenewalSection + @autoRenewEnabled={{@model.user.autoRenewEnabled}} + @renewalDate={{@model.user.membershipExpiresAt}} +/> +<Settings::BillingPage::SupportSection @user={{@model.user}} /> +<Settings::BillingPage::PaymentHistorySection @user={{@model.user}} />This approach allows each component to:
- Control its own title and description
- Make customizations based on user state (member/free user)
- Provide a more intuitive component hierarchy
🧹 Nitpick comments (3)
app/components/settings/billing-page/payment-history-section.ts (3)
28-45: Add a retry mechanism for failed charge loadingThe error handling is good, but users have no way to retry when loading fails except refreshing the entire page.
Consider adding a retry mechanism and exposing it to the template:
@tracked charges: ChargeModel[] = []; @tracked isLoading = true; @tracked errorMessage: string | null = null; + @tracked attemptCount = 0; constructor(owner: unknown, args: Signature['Args']) { super(owner, args); this.loadCharges(); } + get hasAttemptedMultipleTimes() { + return this.attemptCount > 1; + } async loadCharges() { this.isLoading = true; this.errorMessage = null; + this.attemptCount++; try { const result = await this.store.query('charge', { filter: { user_id: this.args.user.id }, }); this.charges = result.toArray(); } catch (error) { console.error('Failed to fetch charges:', error); - this.errorMessage = 'Failed to load payment history. Please try again later.'; + this.errorMessage = this.hasAttemptedMultipleTimes + ? 'Failed to load payment history. Please try again later or contact support.' + : 'Failed to load payment history. Please try again.'; Sentry.captureException(error); - this.charges = []; } finally { this.isLoading = false; } }Then in your template, add a retry button when there's an error:
41-41: Consider preserving charges on errorCurrently, when an error occurs during loading, the charges array is cleared. This could create a confusing experience if a user had successfully loaded charges before but a subsequent refresh fails.
Remove the line that clears charges on error to preserve any previously loaded data:
} catch (error) { console.error('Failed to fetch charges:', error); this.errorMessage = 'Failed to load payment history. Please try again later.'; Sentry.captureException(error); - this.charges = []; } finally {
28-45: Add mechanism to handle race conditions in loadChargesIf
loadChargesis called multiple times in quick succession (e.g., from a retry button), race conditions could occur where an older request completes after a newer one.Add a request ID to track the latest request and ignore results from older requests:
@tracked charges: ChargeModel[] = []; @tracked isLoading = true; @tracked errorMessage: string | null = null; + @tracked currentRequestId = 0; constructor(owner: unknown, args: Signature['Args']) { super(owner, args); this.loadCharges(); } async loadCharges() { this.isLoading = true; this.errorMessage = null; + const requestId = ++this.currentRequestId; try { const result = await this.store.query('charge', { filter: { user_id: this.args.user.id }, }); + // Ignore results from older requests + if (requestId !== this.currentRequestId) return; this.charges = result.toArray(); } catch (error) { + // Ignore errors from older requests + if (requestId !== this.currentRequestId) return; console.error('Failed to fetch charges:', error); this.errorMessage = 'Failed to load payment history. Please try again later.'; Sentry.captureException(error); this.charges = []; } finally { + // Only update loading state for the current request + if (requestId === this.currentRequestId) { this.isLoading = false; + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (10)
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/renewal-section.hbs(1 hunks)app/components/settings/billing-page/support-section.hbs(1 hunks)app/components/settings/billing-page/support-section.ts(1 hunks)app/routes/settings/billing.ts(1 hunks)app/templates/settings/billing.hbs(1 hunks)tests/acceptance/settings-page/billing-test.js(1 hunks)tests/pages/settings/billing-page.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/components/settings/billing-page/membership-section.hbs
🚧 Files skipped from review as they are similar to previous changes (7)
- app/components/settings/billing-page/support-section.hbs
- app/components/settings/billing-page/renewal-section.hbs
- app/routes/settings/billing.ts
- app/components/settings/billing-page/support-section.ts
- tests/acceptance/settings-page/billing-test.js
- app/components/settings/billing-page/payment-history-section.hbs
- tests/pages/settings/billing-page.ts
🔇 Additional comments (1)
app/components/settings/billing-page/payment-history-section.ts (1)
32-36: LGTM: Good practice for error handling and data fetchingThe error handling, Sentry integration, and query structure follow good practices. The component properly manages loading states and provides user-friendly error messages.
Checklist:
[percy]in the message to trigger)Summary by CodeRabbit
New Features
Bug Fixes
Chores