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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 Fixed 'comped' flag behavior in Member API #15030

Merged
merged 2 commits into from Jul 15, 2022

Conversation

naz
Copy link
Member

@naz naz commented Jul 14, 2022

closes https://github.com/TryGhost/Team/issues/1674
refs TryGhost/Members#407

  • The comped flag in Members API unintentionally stopped working when v3 API was dropped with the release of Ghost v5. The flag is deprecated but should be back-compatible for now - we don't want to break integrations like Zapier.
  • To properly deprecate the flag we need to plan it's removal and start signalling about it through the version headers

@allouis @SimonBackx marking you for the review as this change has triggered quite a few rewrites in the member test snapshots. The most unexpected one is the format change in the MemberPaidSubscriptionEvent event test. The rest seems like an expected change. It's the territory I have not touched in a long while, so would be great to get 2x:eyes: over the change :pray:

closes TryGhost/Product#1674

- The comped flag in Members API unintentionally stopped working when v3 API was dropped with the release of Ghost v5. The flag is deprecated but should be back-compatible for now - we don't want to break integratons like Zapier.
- To properly deprecate the flag we need to plan it's removal and start signalling about it through the version headers
@naz naz added bug [triage] something behaving unexpectedly affects:api Affects the Ghost API p2:major [triage] Bugs which we'll fix soon labels Jul 14, 2022
@naz naz requested review from allouis and SimonBackx July 14, 2022 15:31
@naz naz self-assigned this Jul 14, 2022
@codecov
Copy link

codecov bot commented Jul 14, 2022

Codecov Report

Merging #15030 (9d103fb) into main (de23ddb) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #15030      +/-   ##
==========================================
+ Coverage   61.07%   61.09%   +0.02%     
==========================================
  Files         593      596       +3     
  Lines       47557    47649      +92     
  Branches     4271     4290      +19     
==========================================
+ Hits        29044    29112      +68     
- Misses      18464    18488      +24     
  Partials       49       49              
Impacted Files Coverage 螖
...er/api/endpoints/utils/serializers/output/index.js 58.77% <0.00%> (-0.49%) 猬囷笍
core/server/web/members/app.js 0.00% <0.00%> (酶)
core/frontend/web/middleware/cors.js 0.00% <0.00%> (酶)
core/server/web/api/middleware/cors.js 100.00% <0.00%> (酶)
...ndpoints/utils/serializers/output/mappers/index.js 100.00% <0.00%> (酶)
...r/api/endpoints/utils/serializers/output/offers.js
core/frontend/helpers/total_paid_members.js 62.50% <0.00%> (酶)
core/frontend/utils/member-count.js 88.00% <0.00%> (酶)
core/frontend/helpers/total_members.js 64.70% <0.00%> (酶)
...dpoints/utils/serializers/output/mappers/offers.js 14.28% <0.00%> (酶)

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update de23ddb...9d103fb. Read the comment docs.

Copy link
Contributor

@SimonBackx SimonBackx left a comment

Choose a reason for hiding this comment

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

The issue with MemberPaidSubscriptionEvent was that we had two Stripe Subscriptions with the same ID. This wasn't an issue in the past, because the subscription was ignored while the comped flag was removed. Now that started to cause some issues internally in the second test (because the subscription already existed and didn't change, so it didn't need to create the MemberPaidSubscriptionEvent). By using a new subscription id, it now creates a new subscription internally and also a new MemberPaidSubscriptionEvent.

+ I also bumped package.json to test this locally

Copy link
Contributor

@allouis allouis left a comment

Choose a reason for hiding this comment

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

Thanks @SimonBackx for sorting the tests!

@allouis allouis merged commit 6901c3c into TryGhost:main Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects:api Affects the Ghost API bug [triage] something behaving unexpectedly p2:major [triage] Bugs which we'll fix soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants