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.

Sign up for GitHub

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

Jump to bottom

Add back IntroductoryPrice.introPricePeriodUnit string #319

Merged
merged 12 commits into from
Feb 15, 2022
Merged

Add back IntroductoryPrice.introPricePeriodUnit string #319

merged 12 commits into from
Feb 15, 2022

Conversation

beylmk
Copy link
Contributor

@beylmk beylmk commented Feb 11, 2022 •
edited
Loading

In https://github.com/RevenueCat/purchases-flutter/pull/270/files#diff-31ad10b1074cdc6e6c82c3cc2ab3221a117afbaaee017193b6b189d7f34bc769R40 we changed periodUnit from a string to a PeriodUnit enum. An issue was opened indicating we released this change without reflecting it in the changelog and did a minor instead of a major.

This fix adds back the string type and deprecates it. We can leave the PeriodUnit enum type since the properties have different names.

Also:

  • added unit tests to confirm the IntroductoryPrice values are set properly from the original JSON
  • added a little "api test" method to our current sample app which simply accesses each property, ensuring the types are correct. This issue highlights our need for a full API tester in purchases-flutter, but this is a quicker api test
  • created a follow-up ticket for eventually removing introPricePeriodUnit, since some of the changes required aren't obviously attributed to the property

We will release a 3.9.4 with this fix.

[sc-13246]

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #13246: [Flutter] Release 3.9.4 bringing string version of priceIntroductoryPrice.periodUnit.

beylmk
beylmk commented Feb 12, 2022
View reviewed changes
lib/models/introductory_price.dart
@@ -19,6 +15,8 @@ enum PeriodUnit {

/// Contains all the introductory information associated with a [Product]
class IntroductoryPrice with _$IntroductoryPrice {
const IntroductoryPrice._();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

needed to add this to enable the custom getter of the PeriodUnit type. see https://github.com/rrousselGit/freezed#custom-getters-and-methods

beylmk
beylmk commented Feb 12, 2022
View reviewed changes
revenuecat_examples/purchase_tester/lib/main.dart
@@ -127,6 +132,21 @@ class _UpsellScreenState extends State<UpsellScreen> {
),
);
}

void apiTestIntroductoryPrice(IntroductoryPrice introPrice) {
Copy link
Contributor Author

@beylmk beylmk Feb 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

i had this here so i could test with a real API response, but maybe it makes more sense for this to be in the unit tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i ran the API tester method added here on 3.8.0 and noticed that there was also a name change on the introPrice properties. the properties themselves were prefixed with "introPrice" (i.e. introPriceNumberOfPeriodUnits became numberOfPeriodUnits, see attached screenshot for the names in 3.8.0). i didn't see any callout in our releases since 3.8.0 for this change, though perhaps it was encompassed in some of the downstream changes. i'll do more follow-up on monday, but figured i'd post here in case someone has a quick answer...
Screen Shot 2022-02-11 at 5 52 57 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to add all of them back since they are breaking changes right?

Copy link
Contributor Author

@beylmk beylmk Feb 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

yeah i think so. was just confirming i didn't somehow miss these changes somewhere, like in links in changelogs

@beylmk beylmk requested a review from a team February 12, 2022 02:01
@beylmk beylmk marked this pull request as ready for review February 14, 2022 18:21
beylmk
beylmk commented Feb 14, 2022
View reviewed changes
revenuecat_examples/purchase_tester/lib/main.dart
@@ -127,6 +132,21 @@ class _UpsellScreenState extends State<UpsellScreen> {
),
);
}

void apiTestIntroductoryPrice(IntroductoryPrice introPrice) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i ran the API tester method added here on 3.8.0 and noticed that there was also a name change on the introPrice properties. the properties themselves were prefixed with "introPrice" (i.e. introPriceNumberOfPeriodUnits became numberOfPeriodUnits, see attached screenshot for the names in 3.8.0). i didn't see any callout in our releases since 3.8.0 for this change, though perhaps it was encompassed in some of the downstream changes. i'll do more follow-up on monday, but figured i'd post here in case someone has a quick answer...
Screen Shot 2022-02-11 at 5 52 57 PM

lib/models/introductory_price.dart
/// String representation of unit for the billing period of the introductory
/// price, can be DAY, WEEK, MONTH or YEAR.
@Deprecated('Use periodUnit property of type PeriodUnit instead.')
@JsonKey(name: 'periodUnit') String introPricePeriodUnit,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

couldn't have two properties with same JsonKey, so i pulled the enum out

@beylmk
Add back other deprecated properties and add test ensuring they're eq…
7acbd7a 
…ual to the new ones. update apitest method with deprecated props
@beylmk beylmk force-pushed the add-back-intro-price-string branch 2 times, most recently from f256086 to 5f269a3 Compare February 15, 2022 21:34
@beylmk
Copy link
Contributor Author

beylmk commented Feb 15, 2022

@vegaro ready for re-review

vegaro
vegaro approved these changes Feb 15, 2022
View reviewed changes
lib/models/introductory_price.dart Outdated

/// Formatted introductory price of a subscription, including
/// its currency sign, such as €3.99.
/// @Deprecated('Use priceString instead.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// @Deprecated('Use priceString instead.')
@Deprecated('Use priceString instead.')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

@beylmk beylmk merged commit 03f4e18 into main Feb 15, 2022
@beylmk beylmk deleted the add-back-intro-price-string branch February 15, 2022 23:01
@vegaro vegaro mentioned this pull request Jun 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vegaro vegaro vegaro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@beylmk @vegaro