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

Support for Subscriptions API (issue 1067) #1126

Merged
merged 8 commits into from
May 18, 2024

Conversation

kb0
Copy link
Contributor

@kb0 kb0 commented May 13, 2024

PR that fixes #1067

  • loads subscriptions from Google Play as json (like products) into 'play/subscriptions' using monetization.subscriptions/list
    gradlew bootstrapListing --subscriptions
  • publish subscriptions using monetization.subscriptions/create and monetization.subscriptions/patch
    gradlew publishSubscriptions

@kb0 kb0 requested a review from SUPERCILEX as a code owner May 13, 2024 11:07
try {
publisher.monetization().subscriptions().patch(subscription.packageName, subscription.productId, subscription)
.apply {
regionsVersionVersion = SUBSCRIPTIONS_REGIONS_VERSION
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thoughts on sticking this in the subscriptions file name? Otherwise I don't know how we'll ever be able to upgrade people without a breaking release.

The idea is to have insertInAppSubscription and updateInAppSubscription take in a regionsVersion that the bootstrap task and publish subscriptions task serialize and deserialize into the file name (maybe product_id.regionVer.json?). Basically just use the file name but move validation etc outside of the api module which is supposed to be dumb.

Using the file name is bad because it's an implicit place to store untyped data, but good because then each subscription can be on its own version. Of course the more complicated but correct way to do this is with a separate metadata file but that seems way too annoying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May be it's better to use regions version as plugin field, i.e play { subscriptionsRegionsVersion = '2022/02' }?
I cannot find any data on RegionsVersion in 'monetization.subscriptions.list' - so after bootstrapping there is no information about current regions version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, well that sucks. I don't like putting the config in play {} because if you don't specify the subscriptionsRegionsVersion, then that's the same as having a hardcoded default. Unless we default to null and error out if you haven't specified it when bootstrapping/publishing? I also don't like the fact that it forces you to upgrade everything at once if you want to go to a newer version, but maybe that's not a big deal?

I dunno, tradeoffs tradeoffs tradeoffs...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand, this value changes quite rarely - so "2022/02" may be better than "null".
The same reason for single value per instance, instead of value per each subscription.

If some one needs, for ex., to update one set of subscriptions with "2022/02", other with "2022/01" - it's possible to do some "dirty hacks" like - create own tasks mySubscription01, mySubscription02 - copy jsons from somewhere into ./play/subscriptions, set play.subscriptionsRegionsVersion, run publishSubscription 🤦‍♀️

Copy link
Collaborator

Choose a reason for hiding this comment

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

so "2022/02" may be better than "null"

Well but then there's no point in having that play {} option because if we updated the default from "2022/02" to 2024/whatever, then all the people that forgot to set the play {} are silently broken.

So our options are between having a play {} option that errors if unspecified when using subscriptions stuff or building the version into the file name. That means the decision comes down to whether or not we want to allow subscriptions to use independent region versions. With that in mind, do you still have a preference? Otherwise I prefer the file name approach for its flexibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. But may be it's better to put versions in seprate files, because values contains "/", like:
product_id.metadata.json:
{ "regionsVersion": "2022/02" }

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or replace with a dash. But yeah, maybe a metadata file is just better. Are you ok with doing that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool!

@@ -265,6 +269,12 @@ internal interface BootstrapOptions {
field = value
}

override var downloadSubscriptions = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should downloadProducts (line 266 above) default to false? Like does it even make sense to have both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. We should have both "products" and "subscriptions" to true. "Products" (inappproducts.) still used for in-app purchases (consumable and non-consumable), and "subscriptions" (monetization.subscriptions.) used only for subscriptions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good, thanks for the explanation!

@SUPERCILEX SUPERCILEX merged commit 5746197 into Triple-T:master May 18, 2024
4 checks passed
@SUPERCILEX
Copy link
Collaborator

This is great, thanks for the PR!

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.

Missing support for Subscriptions API
2 participants