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

Recurring Payments: AMP + analytics #14819

Merged
merged 8 commits into from
Mar 11, 2020
Merged

Recurring Payments: AMP + analytics #14819

merged 8 commits into from
Mar 11, 2020

Conversation

artpi
Copy link
Contributor

@artpi artpi commented Feb 26, 2020

There are no functional changes, this PR just reshuffles things in order to properly behave in AMP environment

Changes proposed in this Pull Request:

  • Introduce URL fallback, so the block works in AMP
  • pass the redirect URL, so the checkout flow from AMP redirects back to the page where it started
  • pass the pid parameter, which is the source post id. We want to start gathering this data, so we can provide analytics on which posts are most effective so our users can do proper testing of different approaches.

Testing instructions:

  • Add a recurring payments block to a page, set up a payment button
  • Open the page in the front end
  • See that it opens the checkout window as before (no regressions)
  • Install AMP plugin , activate
  • Go to the page where you set up recurring payments, append /amp/ at the end
  • Checkout button should work, will just redirect you to the checkout window instead of opening the modal

Proposed changelog entry for your changes:

  • Recurring Payments now supports Accelerated Mobile Pages

Yes, checkout window is still WIP, but even in the current state it is better than the broken experience we have right now.

@artpi artpi requested a review from a team as a code owner February 26, 2020 19:33
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello artpi! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D39464-code before merging this PR. Thank you!

@artpi artpi requested review from pablinos and a team February 26, 2020 19:33
@artpi artpi added [Status] Needs Review To request a review from Crew. Label will be renamed soon. AMP [Block] Payment Button aka Recurring Payments labels Feb 26, 2020
@jetpackbot
Copy link

jetpackbot commented Feb 26, 2020 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: April 7, 2020.
Scheduled code freeze: March 31, 2020

Generated by 🚫 dangerJS against 72e22e1

@jeherve jeherve added this to the 8.4 milestone Feb 27, 2020
eoigal
eoigal approved these changes Feb 27, 2020
View reviewed changes
Copy link
Contributor

@eoigal eoigal left a comment

Choose a reason for hiding this comment

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

Wfm

eoigal
eoigal requested changes Feb 27, 2020
View reviewed changes
Copy link
Contributor

@eoigal eoigal left a comment

Choose a reason for hiding this comment

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

I found an issue on Longreads with this patch. Don't merge until we figure out what's the cause.
The issue is that the popup modal that allows a user to subscribe isn't being displayed in a NON-AMP page - the user is being redirected to a the subscribe.wordpress.com instead.

Example - http://longreads.com/2020/01/15/whatever-happened-to-______/

@artpi
Copy link
Contributor Author

artpi commented Feb 27, 2020

The issue @eoigal found is related to how Wordpress.com VIP handles Google Analytics.
More details: p1HpG7-8Db-p2

@matticbot
Copy link
Contributor

artpi, Your synced wpcom patch D39464-code has been updated.

@eoigal
Copy link
Contributor

eoigal commented Mar 3, 2020

As I understand the issue with needing to add the target property is because Longreads are using VIP google analytics plugin that is hijacking the links. Since the behaviour of adding the target="_blank" causes a new tab to open and then any redirects will be handled inside that tab, the behaviour isn't ideal. Just wondering have considered passing in some flag in the $attrs for \Jetpack_Memberships::render_button that would include the target="_blank" property - this way we can update Longreads to set this attribute when the VIP GA plugin is active - and if not then we can let the link work within the current tab.

@matticbot
Copy link
Contributor

artpi, Your synced wpcom patch D39464-code has been updated.

@artpi
Copy link
Contributor Author

artpi commented Mar 3, 2020

@eoigal you are right, I changed that. D39464-code will also require a change in themes/a8c/longreads/functions.php to test properly.

@matticbot
Copy link
Contributor

artpi, Your synced wpcom patch D39464-code has been updated.

1 similar comment
@matticbot
Copy link
Contributor

artpi, Your synced wpcom patch D39464-code has been updated.

@artpi
Copy link
Contributor Author

artpi commented Mar 5, 2020

change in themes/a8c/longreads/functions.php is shipped - this should work good on Longreads.com

eoigal
eoigal previously approved these changes Mar 5, 2020
View reviewed changes
Copy link
Contributor

@eoigal eoigal left a comment

Choose a reason for hiding this comment

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

Looks good

jeherve
jeherve requested changes Mar 6, 2020
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This seems to test well for me. I'd have a few notes below.

modules/memberships/class-jetpack-memberships.php Outdated Show resolved Hide resolved
modules/memberships/class-jetpack-memberships.php Outdated Show resolved Hide resolved
modules/memberships/class-jetpack-memberships.php Outdated Show resolved Hide resolved
modules/memberships/class-jetpack-memberships.php Show resolved Hide resolved
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Mar 6, 2020
@jeherve jeherve mentioned this pull request Mar 6, 2020
Open
30 tasks
@artpi artpi added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Mar 9, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2020

Howdy! The Jetpack team has disappeared for a few days to a secret island lair to concoct new ways to make Jetpack one hundred billion percent better. As a result, your Pull Request may not be reviewed right away. Do not worry, we will be back next week to look at your work! Thank you for your understanding.

@eoigal
Copy link
Contributor

eoigal commented Mar 9, 2020

One issue I found was when viewing a post in AMP and already subscribed. The close modal in the screen below doesn't have the redirect from what I can tell and the user can't do much other that to go back in the browser or view their current subscriptions.

no-redirect-close-modal

jeherve
jeherve previously approved these changes Mar 10, 2020
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This works for me. It should be good to merge!

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Mar 10, 2020
@matticbot
Copy link
Contributor

artpi, Your synced wpcom patch D39464-code has been updated.

@artpi artpi dismissed stale reviews from jeherve and eoigal via 72e22e1 March 11, 2020 12:54
@matticbot
Copy link
Contributor

artpi, Your synced wpcom patch D39464-code has been updated.

@artpi artpi merged commit 15a2259 into master Mar 11, 2020
@artpi artpi deleted the recurring-payments/amp branch March 11, 2020 14:08
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 11, 2020
jeherve added a commit that referenced this pull request Mar 20, 2020
@jeherve
Changelog: add #14819
1f3b27b
jeherve added a commit that referenced this pull request Mar 31, 2020
@jeherve
8.4 Changelog (#15075)
0eb0b35 
* Initial changelog entry

* Changelog: add #14904

* Changelog: add #14910

* Changelog: add #14913

* Changelog: add #14916

* Changelog: add #14922

* Changelog: add #14924

* Changelog: add #14925

* Changelog: add #14928

* Changelog: add #14840

* Changelog: add #14841

* Changelog: add #14842

* Changelog: add #14826

* Changelog: add #14835

* Changelog: add #14859

* Changelog: add #14884

* Changelog: add #14888

* Changelog: add #14817

* Changelog: add #14814

* Changelog: add #14819

* Changelog;: add #14797

* Changelog: add #14798

* Changelog: add #14802

* Changelog: add #13676

* Changelog: add #13744

* Changelog: add #13777

* Changelog: add #14446

* Changelog: add #14739

* Changelog: add #14770

* Changelog: add #14784

* Changelog: add #14897

* Changelog: add #14898

* Changelog: add #14968

* Changelog: add #14985

* Changelog: add #15044

* Changelog: add #15052

* Update to remove Podcast since it remains in Beta

* Changelog: add #14803

* Changelog: add #15028

* Changelog: add #15065

* Changelog:add #14886

* Changelog: add #15118

* Changelog: add #14990

* Changelog: add #14528

* Changelog: add #15120

* Changelog: add #15126

* Changelog: add #15049

* Chanegelog: add #14852

* Changelog: add #15090

* Changelog: add #15138

* Changelog: add #15124

* Changelog:add #15055

* Changelog: add #15017

* Changelog: add #15109

* Changelog: add #15145

* Changelog:add #15096

* Changelog:add #15153

* Changelog: add #15133

* Changelog: add #14960

* Changelog: add #15127

* Changelog: add #15056

* Copy current changelog to changelog archive.

* Clarify changelog description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeherve jeherve jeherve left review comments

@kraftbj kraftbj kraftbj approved these changes

@eoigal eoigal eoigal left review comments

@pablinos pablinos Awaiting requested review from pablinos

Assignees

@artpi artpi

Labels
AMP [Block] Payment Button aka Recurring Payments Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
8.4
Development

Successfully merging this pull request may close these issues.
6 participants
@artpi @matticbot @jetpackbot @eoigal @kraftbj @jeherve