Webpack migration for pages that use Stripe #36073
Conversation
The other credit card attributes are still used, to display the name of the card in the dropdown.
orangejenny
added
awaiting QA
orangejenny
requested review from
a team,
biyeun,
nospame and
dannyroberts
as code owners
@orangejenny If you go to Current Subsscription > Prepay by credit card, does the stripe widget work for you? More context: I have a saved credit card, and I remove the saved credit card, I can see the new Stripe widget, but after I close the modal without creating a new card, and reopen the modal again. The widget disappeared for me. |
|
@jingcheng16 Thank you, you found a bug. I just added a commit that should fix it. |
There was a problem hiding this comment.
Overall looks good to me and glad to see this work!
I saw your concern in QA tickets about using token.
We handle autopay in backend here, we will get the card from Stripe.
My guess is, Stripe use token to collect the card information, and store it in for each customer...For future payments, as long as we don't use token saved in front end to make a new payment request, we should be fine
Do you know whether the token is used later, after sending it to Stripe?
corehq/apps/accounting/static/accounting/js/payment_method_handler.js
Outdated
Show resolved
Hide resolved
|
I probably don't have enough js knowledge to know why the Stripe element is still missing, sorry cannot be more helpful here instead of just flagging it again. |
orangejenny
added
QA Passed
and removed
awaiting QA
|
@jingcheng16 Where is that screenshot from? Staging looks okay to me. I'm fixing the translation text issue in #36109 |
|
@coderabbitai review |
WalkthroughThis pull request makes several updates across the codebase that impact both tooling and payment processing. Two obsolete ESLint configuration files have been removed. Several JavaScript modules now include the new dependency Sequence Diagram(s)sequenceDiagram
participant User
participant PaymentHandler
participant CardUI
participant StripeModule
User->>PaymentHandler: Initiate new card entry
PaymentHandler->>CardUI: Check isNewCard observable
CardUI->>StripeModule: getCardElementPromise(stripe_public_key)
StripeModule-->>CardUI: Return card element promise
CardUI->>PaymentHandler: Mount card element on container
User->>PaymentHandler: Submit card info
PaymentHandler->>StripeModule: createStripeToken(handleResponse)
StripeModule-->>PaymentHandler: Return token response
PaymentHandler->>User: Confirm payment token received
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.31.1)corehq/apps/domain/templates/domain/bootstrap3/billing_statements.htmlcorehq/apps/domain/templates/domain/bootstrap5/billing_statements.html✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (3)
corehq/apps/domain/templates/domain/bootstrap5/confirm_billing_info.html (1)
10-11: 💡 Verification agent🧩 Analysis chain
Verify that JavaScript can access Stripe data.
Ensure that the JavaScript code can properly access the initial page data for cards and stripe_public_key after the migration.
🏁 Script executed:
#!/bin/bash # Check how the JS code accesses stripe data after the migration rg -A 3 "initialPageData.get\(\"stripe" corehq/apps/domain/static/domain/js/confirm_billing_info.jsLength of output: 100
Below is a proposed updated review comment. The original grep for a stripe-related key in the JS file didn’t return any evidence of usage. To be sure that the JavaScript layer is correctly retrieving the Stripe data, please verify that the JS explicitly calls for both keys (e.g., via
initialPageData.get("cards")andinitialPageData.get("stripe_public_key")). If those calls aren’t clearly present, either update the JS implementation or confirm that another retrieval mechanism is in place.Please run the following additional checks to locate these calls in
corehq/apps/domain/static/domain/js/confirm_billing_info.js:Once you’ve confirmed that the JavaScript properly accesses these values, please update this review comment accordingly.
Rewritten Review Comment:
Action Required: Verify JS Access to Stripe Data
Ensure that the JavaScript code (in
corehq/apps/domain/static/domain/js/confirm_billing_info.js) correctly retrieves the initial page data for both"cards"and"stripe_public_key", as set incorehq/apps/domain/templates/domain/bootstrap5/confirm_billing_info.html(lines 10–11). Specifically:
- Confirm that a call like
initialPageData.get("cards")is present where expected.- Confirm that a call like
initialPageData.get("stripe_public_key")is used to retrieve the Stripe public key.If these calls are missing or incorrect, adjust the JS to match the new migration requirements. Otherwise, please verify that the JS access meets the required functionality.
🏁 Script executed:
#!/bin/bash echo "Searching for initialPageData usage for 'cards':" rg -A 3 'initialPageData.get\("cards"' corehq/apps/domain/static/domain/js/confirm_billing_info.js echo "Searching for initialPageData usage for 'stripe_public_key':" rg -A 3 'initialPageData.get\("stripe_public_key"' corehq/apps/domain/static/domain/js/confirm_billing_info.jsLength of output: 710
Action Required: Ensure Stripe Data is Accessible in JS
The template correctly initializes both
"cards"and"stripe_public_key", and our search confirms thatinitialPageData.get("cards")is used inconfirm_billing_info.js. However, there’s no corresponding call to access"stripe_public_key"in that JavaScript file. Please verify whether the Stripe public key should be accessed by the JS code:
- If it is required for Stripe functionality, update
corehq/apps/domain/static/domain/js/confirm_billing_info.jsto include a call likeinitialPageData.get("stripe_public_key").- If the public key is not used on this page, consider removing its initialization from the template to avoid confusion.
corehq/apps/domain/static/domain/js/bootstrap3/update_billing_contact_info.js (1)
13-24: 🛠️ Refactor suggestionAdd error handling for card operations.
The card manager implementation lacks explicit error handling for failed API operations. Since this is dealing with payment information, robust error handling is important.
Consider adding error handling:
$(function () { var cardManager = stripeCardManager.stripeCardManager({ cards: initialPageData.get("cards"), url: initialPageData.reverse("cards_view"), + error: function(response) { + console.error('Error managing cards:', response); + // Display user-friendly error message + } }); $("#card-manager").koApplyBindings(cardManager); $("#show_emails").click(function () { $('#emails-text').show(); $(this).parent().hide(); }); });corehq/apps/domain/static/domain/js/bootstrap5/update_billing_contact_info.js (1)
13-24: 🛠️ Refactor suggestionAdd error handling for card operations.
The card manager implementation lacks explicit error handling for failed API operations. Since this is dealing with payment information, robust error handling is important.
Consider adding error handling:
$(function () { var cardManager = stripeCardManager.stripeCardManager({ cards: initialPageData.get("cards"), url: initialPageData.reverse("cards_view"), + error: function(response) { + console.error('Error managing cards:', response); + // Display user-friendly error message + } }); $("#card-manager").koApplyBindings(cardManager); $("#show_emails").click(function () { $('#emails-text').show(); $(this).parent().hide(); }); });
🧹 Nitpick comments (9)
corehq/apps/domain/templates/domain/bootstrap5/update_billing_contact_info.html (1)
16-18: Attention to Pending Styling TODOs.
There are inline TODO comments such as{# todo B5: crispy #}and{# todo B5: inline-style #}. While they do not block the migration, please consider addressing them in a follow-up update to ensure that Bootstrap 5 styling conventions are fully applied and to improve overall UI consistency.corehq/apps/domain/templates/domain/bootstrap5/current_subscription.html (1)
21-396: Review Pending Styling TODOs Across the Template.
There are several inline{# todo B5: ... #}comments (e.g., for CSS form groups and inline styles) throughout the template. While these do not affect functionality, addressing them in a follow-up PR will help ensure visual consistency and better leverage Bootstrap 5’s capabilities.corehq/apps/domain/static/domain/js/bootstrap3/update_billing_contact_info.js (1)
7-7: Dependency added but not directly utilized in this module.You've added 'commcarehq' as a dependency, but it's not included in the function parameters or explicitly used in this module. This appears to be part of the broader Stripe migration mentioned in the PR objectives, where the direct Stripe library usage is being replaced with the CommCareHQ wrapper.
If the 'commcarehq' module is being used indirectly (via one of the other imported modules), consider adding a comment explaining this relationship for better code clarity.
corehq/apps/domain/static/domain/js/bootstrap5/update_billing_contact_info.js (1)
7-7: Dependency added but not directly utilized in this module.You've added 'commcarehq' as a dependency, but it's not included in the function parameters or explicitly used in this module. This appears to be part of the broader Stripe migration mentioned in the PR objectives, where the direct Stripe library usage is being replaced with the CommCareHQ wrapper.
If the 'commcarehq' module is being used indirectly (via one of the other imported modules), consider adding a comment explaining this relationship for better code clarity.
corehq/apps/accounting/static/accounting/js/stripe.js (2)
6-22: Consider improving error handling and documentation.The function correctly handles the case when a key is not provided, but consider adding documentation about the function's purpose and parameters.
+/** + * Creates a Stripe card element and returns a promise that resolves to the element + * @param {string} key - The Stripe public key + * @returns {Promise} - Promise that resolves to the card element + * @throws {Error} - If the key is not provided + */ function getCardElementPromise(key) { if (!key) { throw new Error("Cannot load Stripe, key not provided"); } let promise = new Promise((resolve) => { self.stripePromise = loadStripe(key); self.stripePromise.then(function (stripe) { self.cardElement = stripe.elements().create('card', { hidePostalCode: true, }); resolve(self.cardElement); }); }); return promise; }
3-4: Consider scoping variables to avoid global state.Using
selfvariables creates global state that could cause issues if this module is imported in multiple contexts. Consider using module-level variables instead.-self.stripePromise = undefined; -self.cardElement = undefined; +let stripePromise = undefined; +let cardElement = undefined;Then update all references to
self.stripePromiseandself.cardElementthroughout the file.corehq/apps/accounting/static/accounting/js/payment_method_handler.js (1)
140-156: Mounting/unmounting card UI on subscription changes.
This approach looks good. One minor nitpick: consider confirming no race condition occurs if users toggle rapidly.corehq/apps/accounting/static/accounting/js/stripe_card_manager.js (2)
14-15: Correct injection of new modules.It’s appropriate to inject
hqStripeandinitialPageDatain the function signature for use throughout the file. Ensure all references to external modules remain consistent with any future naming or file path adjustments.
87-87: Simplified token creation call.Directly calling
hqStripe.createStripeToken(handleStripeResponse);is clearer than having a separate local function. Just confirm no additional parameters or configurations are needed for different use cases (e.g., test mode).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
corehq/apps/hqwebapp/tests/data/bootstrap5_diffs/domain/billing_statements.html.diff.txtis excluded by none and included by nonecorehq/apps/hqwebapp/tests/data/bootstrap5_diffs/domain/confirm_billing_info.html.diff.txtis excluded by none and included by nonecorehq/apps/hqwebapp/tests/data/bootstrap5_diffs/domain/current_subscription.html.diff.txtis excluded by none and included by nonecorehq/apps/hqwebapp/tests/data/bootstrap5_diffs/domain/update_billing_contact_info.html.diff.txtis excluded by none and included by nonecorehq/apps/hqwebapp/tests/data/bootstrap5_diffs/javascript/domain/js/billing_statements.js.diff.txtis excluded by none and included by nonecorehq/apps/hqwebapp/tests/data/bootstrap5_diffs/javascript/domain/js/update_billing_contact_info.js.diff.txtis excluded by none and included by nonepackage.jsonis excluded by none and included by noneyarn.lockis excluded by!**/yarn.lock,!**/*.lockand included by none
📒 Files selected for processing (31)
corehq/apps/accounting/.eslintrc.js(0 hunks)corehq/apps/accounting/static/accounting/js/billing_account_form.js(1 hunks)corehq/apps/accounting/static/accounting/js/payment_method_handler.js(5 hunks)corehq/apps/accounting/static/accounting/js/software_plan_version_handler.js(1 hunks)corehq/apps/accounting/static/accounting/js/stripe.js(1 hunks)corehq/apps/accounting/static/accounting/js/stripe_card_manager.js(3 hunks)corehq/apps/accounting/static/accounting/js/subscriptions_main.js(1 hunks)corehq/apps/accounting/templates/accounting/accounts.html(1 hunks)corehq/apps/accounting/templates/accounting/accounts_base.html(1 hunks)corehq/apps/accounting/templates/accounting/invoice.html(1 hunks)corehq/apps/accounting/templates/accounting/partials/new_stripe_card_template.html(2 hunks)corehq/apps/accounting/templates/accounting/partials/stripe_card_ko_template.html(1 hunks)corehq/apps/accounting/templates/accounting/plans.html(1 hunks)corehq/apps/accounting/templates/accounting/subscriptions.html(1 hunks)corehq/apps/domain/.eslintrc.js(0 hunks)corehq/apps/domain/static/domain/js/bootstrap3/billing_statements.js(6 hunks)corehq/apps/domain/static/domain/js/bootstrap3/update_billing_contact_info.js(1 hunks)corehq/apps/domain/static/domain/js/bootstrap5/billing_statements.js(6 hunks)corehq/apps/domain/static/domain/js/bootstrap5/update_billing_contact_info.js(1 hunks)corehq/apps/domain/static/domain/js/confirm_billing_info.js(1 hunks)corehq/apps/domain/static/domain/js/current_subscription.js(3 hunks)corehq/apps/domain/templates/domain/bootstrap3/billing_statements.html(1 hunks)corehq/apps/domain/templates/domain/bootstrap3/confirm_billing_info.html(1 hunks)corehq/apps/domain/templates/domain/bootstrap3/current_subscription.html(1 hunks)corehq/apps/domain/templates/domain/bootstrap3/update_billing_contact_info.html(1 hunks)corehq/apps/domain/templates/domain/bootstrap5/billing_statements.html(1 hunks)corehq/apps/domain/templates/domain/bootstrap5/confirm_billing_info.html(1 hunks)corehq/apps/domain/templates/domain/bootstrap5/current_subscription.html(1 hunks)corehq/apps/domain/templates/domain/bootstrap5/update_billing_contact_info.html(1 hunks)corehq/apps/domain/views/accounting.py(1 hunks)corehq/apps/enterprise/views.py(1 hunks)
💤 Files with no reviewable changes (2)
- corehq/apps/domain/.eslintrc.js
- corehq/apps/accounting/.eslintrc.js
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.js`: - Review the javascript code against the google javascript style guide and point out any mismatches. - User-facing language should be clear. - All user-facing text must ...
**/*.js: - Review the javascript code against the google javascript style guide and point out any mismatches. - User-facing language should be clear. - All user-facing text must be translated usinggettext; use_.templatefor variable-containing strings. - User-facing dates and times must be in a relevant time zone, usingUserTimeorcorehq.util.timezones. - Use<%- ... %>in Underscore templates to HTML escape values; useDomPurifyfor escaping user input outside templates. - AJAX requests must show a spinner and have anerrorcallback. - Naming should be consistent, avoiding different capitalizations of the same identifier. - JavaScript should be modular, with explicit dependencies (except in app manager, reports, and web apps). - Avoid long parameter lists; prefer kwargs-style objects and useassert_propertiesfor validation. - Ensure JavaScript accessing initial page data waits until the page is fully loaded to prevent race conditions. - Prefer Knockout over jQuery and avoid mixing the two.
corehq/apps/accounting/static/accounting/js/subscriptions_main.jscorehq/apps/accounting/static/accounting/js/software_plan_version_handler.jscorehq/apps/accounting/static/accounting/js/stripe.jscorehq/apps/domain/static/domain/js/current_subscription.jscorehq/apps/accounting/static/accounting/js/billing_account_form.jscorehq/apps/domain/static/domain/js/bootstrap3/billing_statements.jscorehq/apps/domain/static/domain/js/bootstrap5/billing_statements.jscorehq/apps/domain/static/domain/js/bootstrap5/update_billing_contact_info.jscorehq/apps/domain/static/domain/js/confirm_billing_info.jscorehq/apps/domain/static/domain/js/bootstrap3/update_billing_contact_info.jscorehq/apps/accounting/static/accounting/js/payment_method_handler.jscorehq/apps/accounting/static/accounting/js/stripe_card_manager.js
`**/*.py`: - Review the code following best practises and standards
**/*.py: - Review the code following best practises and standards
corehq/apps/domain/views/accounting.pycorehq/apps/enterprise/views.py
🧬 Code Definitions (2)
corehq/apps/domain/views/accounting.py (1)
corehq/apps/enterprise/views.py (1)
stripe_cards(306-307)
corehq/apps/enterprise/views.py (1)
corehq/apps/domain/views/accounting.py (1)
stripe_cards(456-457)
🔇 Additional comments (56)
corehq/apps/domain/templates/domain/bootstrap3/confirm_billing_info.html (1)
6-6: Updated JavaScript Inclusion Tag.
The switch from the old{% requirejs_main 'domain/js/confirm_billing_info' %}to{% js_entry_b3 'domain/js/confirm_billing_info' %}is correctly implemented. This change aligns with the new Webpack-based resource management and helps standardize the way JavaScript is loaded. Please ensure that the new entry point is consistently defined in your Webpack configuration.corehq/apps/domain/templates/domain/bootstrap3/update_billing_contact_info.html (1)
6-6: Migrated JavaScript Loading for Billing Contact Info (Bootstrap3).
Replacing{% requirejs_main 'domain/js/bootstrap3/update_billing_contact_info' %}with{% js_entry_b3 'domain/js/bootstrap3/update_billing_contact_info' %}is a clean move. It maintains consistency across templates now that the project has shifted away from RequireJS.corehq/apps/domain/templates/domain/bootstrap5/update_billing_contact_info.html (1)
6-6: Adopted New JavaScript Entry Tag for Bootstrap5 Template.
The update from{% requirejs_main_b5 'domain/js/bootstrap5/update_billing_contact_info' %}to{% js_entry 'domain/js/bootstrap5/update_billing_contact_info' %}is exactly what’s needed for the migration. This makes the JavaScript resource loading consistent with the new module bundling approach.corehq/apps/domain/templates/domain/bootstrap3/current_subscription.html (1)
5-5: JavaScript Resource Loader Updated.
Changing the tag from the old RequireJS implementation to{% js_entry_b3 'domain/js/current_subscription' %}is implemented correctly and follows the migration strategy. This update should streamline script loading and improve maintainability.corehq/apps/domain/templates/domain/bootstrap5/current_subscription.html (1)
5-5: Migrated JavaScript Entry for Current Subscription (Bootstrap5).
Switching from{% requirejs_main 'domain/js/current_subscription' %}to{% js_entry 'domain/js/current_subscription' %}adheres to the new standards for JavaScript module loading and is properly done.corehq/apps/domain/static/domain/js/confirm_billing_info.js (3)
7-7: Addition of CommCareHQ dependency is appropriate.The addition of the 'commcarehq' module dependency aligns with the migration to Webpack and the updated Stripe integration approach mentioned in the PR objectives.
12-12: Stripe parameter removal is consistent with integration changes.Removing the Stripe parameter from the function signature is consistent with the PR's objective to use Stripe's module wrapper instead of directly accessing the Stripe library.
27-30:✅ Verification successful
Verify Stripe card manager integration.
The card manager is still being used, but now it depends on the new abstracted Stripe integration. Ensure that all Stripe functionality works correctly after these changes.
🏁 Script executed:
#!/bin/bash # Look for any references to Stripe in the card manager to ensure proper integration rg -A 5 "stripeCardManager" corehq/apps/accounting/static/accounting/js/stripe_card_manager.jsLength of output: 383
Stripe Integration Confirmed
The verification indicates that the new abstracted Stripe integration is in use correctly. The
stripe_card_manager.jsfile exposes thestripeCardManagerfunction as expected, and its usage inconfirm_billing_info.jsnow correctly initializes the card manager with the updated Stripe integration.
- The
stripeCardManagerfunction is defined and properly exported incorehq/apps/accounting/static/accounting/js/stripe_card_manager.js.- The code in
confirm_billing_info.jscorrectly instantiates the card manager using the new integration, with the proper configuration (e.g., cards and URL).Please ensure that you also run end-to-end tests to fully validate the Stripe functionality in the UI. Overall, no changes are necessary based on the current integration implementation.
corehq/apps/domain/templates/domain/bootstrap5/confirm_billing_info.html (1)
6-6: Appropriate migration from RequireJS to JS Entry.The change from
requirejs_maintojs_entryis consistent with the Webpack migration mentioned in the PR objectives and matches similar changes across other templates.corehq/apps/domain/templates/domain/bootstrap3/billing_statements.html (3)
5-5: Appropriate migration from RequireJS to JS Entry Bootstrap3.The change from
requirejs_maintojs_entry_b3is consistent with the Webpack migration mentioned in the PR objectives and matches similar changes across other templates.
9-10:✅ Verification successful
Flattened Stripe configuration structure.
The initial page data has been restructured to directly expose stripe_cards and stripe_public_key instead of nesting them in a stripe_options object. Ensure the JavaScript has been updated to access these values directly.
🏁 Script executed:
#!/bin/bash # Check how the JS code accesses the restructured stripe data rg -A 3 "initialPageData.get\(\"stripe" corehq/apps/domain/static/domain/js/bootstrap3/billing_statements.jsLength of output: 289
Stripe configuration flattening verified.
- The HTML file now directly exposes
stripe_cardsandstripe_public_keyvia{% initial_page_data %}.- The JS code in
corehq/apps/domain/static/domain/js/bootstrap3/billing_statements.jscorrectly retrievesstripe_cardsusinginitialPageData.get("stripe_cards"), confirming that the flattened structure is properly handled.- Please also verify that
stripe_public_keyis accessed similarly where required.
99-99:✅ Verification successful
Verify Stripe card template integration.
Ensure that the Stripe card template is properly integrated with the new Stripe implementation.
🏁 Script executed:
#!/bin/bash # Check the Stripe card KO template for any required updates cat corehq/apps/accounting/templates/accounting/partials/stripe_card_ko_template.htmlLength of output: 5212
Stripe Card Template Integration Verified
I reviewed the contents of
corehq/apps/accounting/templates/accounting/partials/stripe_card_ko_template.htmland confirmed that both the saved card and new card sections are correctly implemented. The template correctly defines the container for the new Stripe card element (stripe-card-container) and includes the expected KnockoutJS bindings to manage UI state and errors. The integration appears to be in line with the new Stripe implementation requirements.
- Verified that both the saved card (
saved-stripe-card-ko-template) and new card (new-stripe-card-ko-template) sections are properly set up.- Confirmed the presence and appropriate configuration of the
stripe-card-containerelement.- The inclusion in
billing_statements.htmlis correctly referencing this template.No further changes are required.
corehq/apps/accounting/templates/accounting/invoice.html (1)
6-6: Appropriate migration from RequireJS to JS Entry Bootstrap3.The change from
requirejs_maintojs_entry_b3is consistent with the Webpack migration mentioned in the PR objectives and matches similar changes across other templates.corehq/apps/accounting/templates/accounting/plans.html (1)
6-6: Webpack migration successfully implementedThe change from RequireJS to Webpack loading is correctly implemented with the new js_entry_b3 tag, maintaining the same module reference.
corehq/apps/accounting/templates/accounting/accounts_base.html (1)
6-6: Consistent Webpack migration implementationThe js_entry_b3 tag has been correctly implemented to replace RequireJS, maintaining consistency with other template changes in this PR.
corehq/apps/accounting/static/accounting/js/subscriptions_main.js (2)
6-6: Appropriate dependency addition for Webpack migrationThe addition of the 'commcarehq' dependency aligns with the Webpack migration strategy outlined in the PR objectives.
10-10: Syntax improvementThe added comma after 'widgets' improves code consistency and maintainability, following JavaScript best practices.
corehq/apps/accounting/templates/accounting/subscriptions.html (1)
7-7: Consistent Webpack migration implementationThe js_entry_b3 tag correctly replaces RequireJS for loading the subscriptions_main module, maintaining functionality while upgrading the loading mechanism as intended by the PR.
corehq/apps/domain/views/accounting.py (1)
519-520: Structure simplification for Stripe configuration.The flattened structure for Stripe configuration (moving away from the nested
stripe_optionsdictionary) makes the code cleaner and more direct. This change is part of the broader Stripe integration refactoring.corehq/apps/accounting/static/accounting/js/billing_account_form.js (1)
7-7: Improved dependency management.The addition of 'commcarehq' as a dependency and explicit inclusion of 'initialPageData' in the function parameters improves code consistency. The initialPageData parameter was previously used in the code but wasn't explicitly included in the function signature.
Also applies to: 11-11
corehq/apps/enterprise/views.py (1)
368-369: Structure refactoring for Stripe optionsThe Stripe options structure has been flattened, moving
stripe_public_keyandstripe_cardsto the root level of the pagination context instead of being nested under astripe_optionsdictionary. This change simplifies the data structure and is consistent with similar modifications in other views.corehq/apps/accounting/templates/accounting/accounts.html (1)
6-6: JavaScript loading mechanism migrationReplaced
requirejs_maintag withjs_entry_b3tag, aligning with the Webpack migration efforts across the codebase. This change is part of the broader update to JavaScript module loading.corehq/apps/accounting/static/accounting/js/software_plan_version_handler.js (3)
9-9: Added new CommCareHQ dependencyThe addition of the 'commcarehq' dependency is part of the Webpack migration effort and consistent with similar changes in other JavaScript modules.
16-16: Fixed comma in parameter listAdded a missing comma after 'multiselectUtils' in the parameter list to ensure proper syntax.
22-23: Formatting improvementMinor formatting change that improves code readability.
corehq/apps/domain/static/domain/js/bootstrap3/billing_statements.js (3)
8-8: Replaced Stripe with CommCareHQ dependencyReplaced the 'stripe' dependency with 'commcarehq' as part of the migration to use Webpack and the updated Stripe integration through the CommCareHQ library.
30-30: Improved code formattingRemoved trailing commas to improve code consistency and formatting.
Also applies to: 43-43, 53-53, 62-62
71-75: Updated Stripe cards retrievalModified how Stripe cards are retrieved to align with the flattened data structure from the backend. Now directly accessing
initialPageData.get("stripe_cards")instead of going through a nestedstripe_optionsobject.corehq/apps/accounting/templates/accounting/partials/new_stripe_card_template.html (2)
25-25: Stripe integration looks good.The addition of the Stripe card container element aligns with the PR objective of upgrading the Stripe integration by using their UI elements instead of custom HTML inputs.
50-50: Minor formatting improvement.The data-bind attribute has been consolidated to a single line, improving readability without affecting functionality.
corehq/apps/domain/static/domain/js/current_subscription.js (2)
6-6: Dependency update for Stripe integration.The replacement of the direct Stripe dependency with 'commcarehq' aligns with the PR objective of migrating to use Webpack and upgrading the Stripe integration through a custom wrapper.
22-22: Code style improvement.The consistent formatting of object literals with proper trailing commas improves code maintainability and follows good JavaScript coding practices.
Also applies to: 34-35
corehq/apps/accounting/templates/accounting/partials/stripe_card_ko_template.html (1)
111-112: Stripe integration looks good.The addition of the Stripe card container element and removal of individual card input fields aligns with the PR objective of upgrading the Stripe integration to use their UI elements instead of custom HTML inputs.
corehq/apps/accounting/static/accounting/js/stripe.js (2)
1-5: Good module structure.The import of the Stripe module and setup of necessary variables follows good JavaScript practices. This aligns with the PR objective of utilizing Stripe's module wrapper.
1-31: Verify Stripe script is loaded asynchronously.Since the PR objective mentions that "the JavaScript implementation is designed to be asynchronous, as the Stripe script must be loaded from Stripe's servers," it's important to ensure this implementation correctly handles the asynchronous loading.
The
loadStripefunction appears to return a Promise, which supports asynchronous loading. The implementation uses Promise chains to handle the asynchronous operations, which is appropriate. However, it would be beneficial to test this implementation to ensure it behaves correctly in different network conditions and page load scenarios.corehq/apps/domain/static/domain/js/bootstrap5/billing_statements.js (5)
8-8: Dependency addition looks good.
You're importing'commcarehq'here, which aligns with the refactoring elsewhere for a centralized payment library.
30-30: No concerns about trailing comma.
This trailing comma appears to follow your existing pattern, and there's no indication from static analysis that it's an issue.
43-43: Similar trailing comma usage.
Continues the same style; no immediate issue flagged by static analysis.
53-53: Trailing comma remains consistent.
No static analysis concerns raised, so this is acceptable.
71-71: Proper retrieval of stripe card data.
Accessing"stripe_cards"from initial page data is clear and straightforward.corehq/apps/domain/templates/domain/bootstrap5/billing_statements.html (2)
5-5: Use of js_entry is consistent with Webpack migration.
Switching from{% requirejs_main_b5 ... %}to{% js_entry ... %}is in line with the new approach.
9-10: Initialization of new page data keys is correct.
Exposingstripe_cardsandstripe_public_keyensures the JavaScript can properly manage card handling.corehq/apps/accounting/static/accounting/js/payment_method_handler.js (8)
1-5: Helpful documentation block.
These clarifications about thestripe_public_keyrequirement and container ID improve maintainability.
10-11: New dependencies are consistent.
The addition of'accounting/js/stripe'and'hqwebapp/js/initial_page_data'aligns with your refactored Stripe flow.
16-17: Integration with hqStripe and page data is clear.
ImportinghqStripeandinitialPageDataat the top clarifies dependencies for token creation and retrieving keys.
211-216: Reset method properly clears the form and card element.
Preventing stale data is crucial. This ensures a fresh state upon subsequent uses.
524-533: Well-structured reset logic in stripeCardModel.
Resetting all relevant observables avoids leakage of sensitive data across sessions.
573-574: Switch to hqStripe’s createStripeToken.
Matches the new centralized approach for token generation.
579-580: Token and environment mode assigned cleanly.
Storingresponse.token.idand checkinglivemodeis appropriate.
585-585: Properly localized error message.
Usinggettextto surface user-facing errors ensures translations remain consistent.corehq/apps/accounting/static/accounting/js/stripe_card_manager.js (6)
1-5: Good documentation for required container and usage.These doc comments effectively communicate the need for a container element and public key. No issues identified here.
9-10: New dependencies added.The addition of
'accounting/js/stripe'and'hqwebapp/js/initial_page_data'aligns with the new approach of retrieving the Stripe public key and managing asynchronous loading.
30-30: Reduced observable fields.Switching to observe only
isAutopayandtokensimplifies the model. Verify that removing other fields (e.g., card details) will not impact previous functionalities.
37-42: Concise reset logic.Resetting
isAutopayandtokenalong with conditionally clearing the card element is clear and maintains proper state. This looks good; just ensure no race condition occurs if this is invoked before the promise resolves.
74-80: Updated Stripe response handling logic.Switching from
response.idtoresponse.token.idaligns with Stripe’s response structure. Be sure the server-side code expects the token under the same field name. Note that any user-facing error messages fromresponse.error.messagemay need to be localized if they are surfaced directly.
179-181: Instantiation ofnewStripeCardModel.Passing
url: data.urlinto the model improves clarity and maintainability by avoiding positional arguments. No immediate concerns identified.
| function createStripeToken(handleResponse) { | ||
| self.stripePromise.then(function (stripe) { | ||
| stripe.createToken(self.cardElement).then(handleResponse); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for promise rejections.
The function should handle potential rejections in the Promise chain to prevent unhandled promise rejections.
function createStripeToken(handleResponse) {
self.stripePromise.then(function (stripe) {
- stripe.createToken(self.cardElement).then(handleResponse);
+ stripe.createToken(self.cardElement)
+ .then(handleResponse)
+ .catch(function(error) {
+ handleResponse({error: {message: error.message || "Failed to create token"}});
+ });
+ }).catch(function(error) {
+ handleResponse({error: {message: "Failed to load Stripe"}});
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function createStripeToken(handleResponse) { | |
| self.stripePromise.then(function (stripe) { | |
| stripe.createToken(self.cardElement).then(handleResponse); | |
| }); | |
| } | |
| function createStripeToken(handleResponse) { | |
| self.stripePromise.then(function (stripe) { | |
| stripe.createToken(self.cardElement) | |
| .then(handleResponse) | |
| .catch(function(error) { | |
| handleResponse({error: {message: error.message || "Failed to create token"}}); | |
| }); | |
| }).catch(function(error) { | |
| handleResponse({error: {message: "Failed to load Stripe"}}); | |
| }); | |
| } |
| // This assumes this model won't be created until the page is loaded, | ||
| // which is reasonable because knockout bindings don't get applied until then. | ||
| self.cardElementMounted = false; | ||
| self.cardElementPromise = hqStripe.getCardElementPromise(initialPageData.get("stripe_public_key")); | ||
| self.cardElementPromise.then(function (cardElement) { | ||
| cardElement.mount('#stripe-card-container'); | ||
| self.cardElementMounted = true; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential promise rejections for cardElementPromise.
Currently, if the getCardElementPromise call fails (e.g., invalid public key or network issues), there's no catch block or error handling. This can lead to unhandled promise rejections. Consider adding error handling to improve resilience.
self.cardElementPromise = hqStripe.getCardElementPromise(initialPageData.get("stripe_public_key"));
self.cardElementPromise.then(function (cardElement) {
cardElement.mount('#stripe-card-container');
self.cardElementMounted = true;
+}).catch(function (error) {
+ console.error("Failed to mount card element:", error);
+ self.errorMsg("Unable to load Stripe card element. Please try again.");
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // This assumes this model won't be created until the page is loaded, | |
| // which is reasonable because knockout bindings don't get applied until then. | |
| self.cardElementMounted = false; | |
| self.cardElementPromise = hqStripe.getCardElementPromise(initialPageData.get("stripe_public_key")); | |
| self.cardElementPromise.then(function (cardElement) { | |
| cardElement.mount('#stripe-card-container'); | |
| self.cardElementMounted = true; | |
| }); | |
| // This assumes this model won't be created until the page is loaded, | |
| // which is reasonable because knockout bindings don't get applied until then. | |
| self.cardElementMounted = false; | |
| self.cardElementPromise = hqStripe.getCardElementPromise(initialPageData.get("stripe_public_key")); | |
| self.cardElementPromise.then(function (cardElement) { | |
| cardElement.mount('#stripe-card-container'); | |
| self.cardElementMounted = true; | |
| }).catch(function (error) { | |
| console.error("Failed to mount card element:", error); | |
| self.errorMsg("Unable to load Stripe card element. Please try again."); | |
| }); |
Product Description
https://dimagi.atlassian.net/browse/SAAS-15999
This migrates to webpack and upgrades our usage of Stripe to use their module wrapper and newer API. The newer API requires using Stripe UI elements rather than our own, so there's a bunch of HTML changes in here. The js is pretty async, as the Stripe script needs to be leaded from Stripe's servers, rather than having HQ host it.
Safety Assurance
Safety story
This area feels rather high risk, and it's my first time working with Stripe. I'm fairly reliant on code review and QA here.
Automated test coverage
Accounting has tests, but I don't think it has javascript coverage.
QA Plan
https://dimagi.atlassian.net/browse/QA-7597
Rollback instructions
Labels & Review