Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Revert "Webpack migration for pages that use Stripe"" #36123

Merged
merged 4 commits into from
Apr 2, 2025

Conversation

orangejenny
Copy link
Contributor

@orangejenny orangejenny commented Mar 31, 2025

Reverts #36121, restoring #36073

I didn't realize there's a different UI for users who don't have any cards saved. This PR has a few additional commits to add support for that UI. The js is a bit finicky, as there are several knockout vars controlling the existence of the credit card inputs, and Stripe has to mount the UI whenever that area is made visible.

Safety Assurance

Safety story

Moderate risk due to affecting payments. Risk is limited to the updated pages.

Automated test coverage

Accounting has tests, but I don't think it has javascript coverage.

QA Plan

Using the same ticket as for the previous PR: https://dimagi.atlassian.net/browse/QA-7597

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@orangejenny orangejenny added the Open for review: do not merge A work in progress label Mar 31, 2025
@orangejenny orangejenny requested review from a team, biyeun, nospame and dannyroberts as code owners March 31, 2025 20:12
@dimagimon dimagimon added the dependencies Pull requests that update a dependency file label Mar 31, 2025
@orangejenny orangejenny added the awaiting QA QA in progress. Do not merge label Mar 31, 2025
@orangejenny
Copy link
Contributor Author

This is ready for review now and is also going to have another pass of QA.

@orangejenny orangejenny added QA Passed and removed awaiting QA QA in progress. Do not merge Open for review: do not merge A work in progress labels Apr 1, 2025
@@ -204,6 +211,7 @@ hqDefine('accounting/js/payment_method_handler', [
if (self.savedCards().length > 0) {
self.selectedCardType('saved');
}
self.showOrHideStripeUI(self.mustCreateNewCard());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we already subscribe to this observable, is this line still necessary?

Copy link
Contributor

@jingcheng16 jingcheng16 Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm! I saw you add a comment in a later commit

@orangejenny orangejenny merged commit e965de8 into master Apr 2, 2025
14 checks passed
@orangejenny orangejenny deleted the revert-36121-revert-36073-jls/webpack-stripe branch April 2, 2025 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file QA Passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants