Skip to content

Recurring Payments: AMP + analytics#14819

Merged
artpi merged 8 commits intomasterfrom
recurring-payments/amp
Mar 11, 2020
Merged

Recurring Payments: AMP + analytics#14819
artpi merged 8 commits intomasterfrom
recurring-payments/amp

Conversation

@artpi
Copy link
Copy Markdown
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 February 26, 2020 19:33
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Feb 26, 2020

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 a team and pablinos February 26, 2020 19:33
@artpi artpi added [Status] Needs Review This PR is ready for review. AMP [Block] Payment Button aka Recurring Payments labels Feb 26, 2020
@jetpackbot
Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

matticbot commented Mar 2, 2020

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

@eoigal
Copy link
Copy Markdown
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
Copy Markdown
Contributor

matticbot commented Mar 3, 2020

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

@artpi
Copy link
Copy Markdown
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.

@artpi artpi force-pushed the recurring-payments/amp branch from ce987cc to 096529a Compare March 5, 2020 09:38
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Mar 5, 2020

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

1 similar comment
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Mar 5, 2020

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

@artpi
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. Enhancement Changes to an existing feature — removing, adding, or changing parts of it and removed [Status] Needs Review This PR is ready for review. labels Mar 6, 2020
@jeherve jeherve mentioned this pull request Mar 6, 2020
Open
30 tasks
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Mar 9, 2020

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

eoigal
eoigal previously approved these changes Mar 9, 2020
View reviewed changes
Copy link
Copy Markdown
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.

LGTM

@artpi artpi added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 9, 2020
@github-actions
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 This PR is ready for review. labels Mar 10, 2020
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Mar 11, 2020

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
@artpi artpi force-pushed the recurring-payments/amp branch from 156c6a8 to 72e22e1 Compare March 11, 2020 12:54
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Mar 11, 2020

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 Enhancement Changes to an existing feature — removing, adding, or changing parts of it Touches WP.com Files

Projects

None yet

Milestone

8.4

Development

Successfully merging this pull request may close these issues.

6 participants

@artpi @matticbot @jetpackbot @eoigal @kraftbj @jeherve