Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Partner Compatibility: Adding a unique connection screen for customers who receive a coupon from a Jetpack partner #21813
Partner Compatibility: Adding a unique connection screen for customers who receive a coupon from a Jetpack partner #21813
Changes from 5 commits
ade160a
6ddbc18
c2037ef
2922ac2
81b8709
c25ba93
1166843
455d685
53fcb6b
306db06
d00fb48
817c71c
e14072d
4dea4da
66dd804
23b647e
768c956
0e1818f
ec8a2b6
8cfd50c
404a3ba
d891cc3
1edcf3d
72cc873
7326a80
560b064
8d91fa4
aa1ddb6
26ef703
a279dd8
3c4e1bc
c4df089
60380e2
31c3e5a
77ece71
fb9ea3f
82414c4
76c922f
10191d3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should the site connection take priority, so if the site is connected, we do not display the banner even if you use the query string?
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.
It's on purpose that we've added a query string override. It's so we can link directly to the coupon component in "remote JITMs" where we're going to do some coupon validation on the server-side (e.g.: has the coupon already been redeemed?).
It might be, that you have a use-case in mind that we've missed but it should be OK 😄
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.
Since we're introducing a completely new class, it would be nice to have tests for all of this.
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.
You bet! Here's a link to the new
Partner_Coupon_Test
class.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.
If we move the lib to the Partner package, we'll need to move away from the dependency on the
Jetpack
class here as well, and instead build the admin URL ourselves.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.
My implementation isn't suuuuper clean, but we need allow plugins to register:
I've created an anonymous function so we can accept dynamic argument values:
jetpack/projects/packages/partner/src/class-partner-coupon.php
Lines 88 to 97 in 26ef703
This means that the plugin that wish to register the admin hooks and tell us which URL we should redirect to, can just add:
Jetpack_Partner_Coupon::register_coupon_admin_hooks( 'jetpack', Jetpack::admin_url() );
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 see that this will be more difficult to replace, and that may make it harder to move the whole class to the Partner package. :(
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 feel like the "compromise" fits well with the spirit of WordPress. I just moved it to a filter instead.
The most awesome experience here would be an API request to fetch products etc, but because we have a scenario where the user haven't connected their site yet (pre-connection), we're not allowed to make external requests yet (source: Jesse Friedman, based on JITM experience).