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

Add support for billing for apps #1455

Merged
merged 1 commit into from
Jun 17, 2022
Merged

Add support for billing for apps #1455

merged 1 commit into from
Jun 17, 2022

Conversation

paulomarg
Copy link
Contributor

@paulomarg paulomarg commented Jun 17, 2022

What this PR does

Partners might want to charge for their apps, but this gem currently doesn't really add any specific support for that scenario.

This PR adds that support by appending a concern to Authenticated to check if there is an active payment for the merchant, or requesting one if there isn't, and redirecting the merchant to it.

If the new billing configuration setting isn't present, this does nothing.

Reviewer's guide to testing

This was mostly tested by unit tests, and by activating the configuration in a new app.

Things to focus on

  1. Does this break any pre-existing flows? As in, are there cases where we might try to apply billing when we shouldn't?
  2. Is the test coverage good?

Checklist

Before submitting the PR, please consider if any of the following are needed:

  • Update CHANGELOG.md if the changes would impact users

@@ -75,7 +75,6 @@ class << self
@initialized = false

def with_session(test_class, is_embedded: false, is_private: false, &block)
WebMock.enable!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was making some tests flaky - webmock is already blocking all requests so we don't need to do this here.

Copy link
Contributor

@hannachen hannachen left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! I was able to 🎩 and do a smoke test with the existing flow without encountering issues.
Screen Shot 2022-06-17 at 1 06 17 PM

I did encounter an interesting flow, where if the app was initially installed without billing, but later introduced to the app. The billing screen won't show up until the next merchant log in/session, is this expected behaviour?

@paulomarg
Copy link
Contributor Author

paulomarg commented Jun 17, 2022

The billing screen won't show up until the next merchant log in/session, is this expected behaviour?

Yes - because we need to query Shopify for the billing status for a merchant, we need to have access to a token, which only happens on authenticated requests, or when coming back from OAuth. Those are the two points where we check for billing.

I'm going to merge the PR but we can fix forward if there are any other scenarios we want to cover!

@paulomarg paulomarg merged commit 9a3e49b into main Jun 17, 2022
@paulomarg paulomarg deleted the add_billing_support branch June 17, 2022 18:35
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems June 20, 2022 14:03 Inactive
@immakdas
Copy link

The main scenario is not covered so it is useless

  1. merchant user clicks to Install the app
    1inst
  2. on the payment page customer click cancel
    2cancels
  3. as a result App is installed but not paid and the user can Open it for free
    3free

tested with the shopify_app 20.0.2
I believe the Trial scenario is also not covered

config.billing = ShopifyApp::BillingConfiguration.new(
     charge_name: "My app billing charge",
     amount: 5,
     interval: ShopifyApp::BillingConfiguration::INTERVAL_EVERY_30_DAYS,
     currency_code: "USD",
     trial: 1,
  )```

return
`gems/shopify_app-20.0.2/lib/shopify_app/configuration.rb:119:in `initialize': unknown keyword: :trial (ArgumentError)`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants