-
Notifications
You must be signed in to change notification settings - Fork 56
fix: content gate metadata methods and params #4306
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
Conversation
miguelpeixe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests well!
I left a couple of non-blocking comments; feel free to merge after addressing them.
|
|
||
| namespace Newspack; | ||
|
|
||
| use Newspack\Content_Gate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given we're already in Newspack namespace, this and the one below are superfluous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, I didn't check this and just followed the existing pattern 😄
|
|
||
| use Newspack\Memberships; | ||
| use Newspack\Memberships\Metering; | ||
| use Newspack\Metering; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the previous comment, these 3 lines are superfluous
leogermani
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't able to confirm the GA properties.. I still didn't see them. But approving because the PHP fatal is more important
|
🎉 This PR is included in version 6.24.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
All Submissions:
Changes proposed in this Pull Request:
Fixes a fatal error due to referencing a method that moved from the
Newspack\Membershipsclass toNewspack\Content_Gate. Also ensures that GA4 custom events receive thegate_post_idandnewspack_popup_idparams throughout auth and modal checkout flows by normalizing the param name frommemberships_content_gatetogate_post_id(Automattic/newspack-blocks#2121 attempted to do this with Automattic/newspack-blocks@0c5301b#diff-f371eb641807afd155089ce67e2add277c8d7a9dfe029518e977527c7ca015c3R144-R148 but some flows still expected thememberships_content_gateparam).How to test the changes in this Pull Request:
releasefor this repo and Newspack Blocks, complete registration, login, and modal checkout flows from a content gate and a Campaigns prompt. Observe that:gate_post_idandnewspack_popupparamsgate_post_idandnewspack_popup_idparams are appended to all GA4 events from registration, login, and modal checkout flows originating from a content gate and Campaigns prompt, respectively.Other information: