Early implementation of amp-access-laterpay by trodrigues · Pull Request #5920 · ampproject/amphtml

trodrigues · 2016-10-31T18:38:36Z

This is the first attempt at an implementation for amp-access-laterpay.

(For anyone looking at this out of context, this comes as a follow up to some communication with @dvoytenko out of this repository and is based on some preparation work he did recently to allow for this implementation effort to occur)

This first implementation is mostly trying to get the basic workflow down, so no proper tests/docs/type hints yet (and could probably do with a WIP label 😄 )

Also, while I had quite a bit of a look at other modules, and some of the spec docs to get a better idea of the overall codebase patterns and existing methods and utilities I'm sure this still needs a bit of work in conforming to those. I'm adding some details below.

TODO

Add tests
Add documentation
Add docs and closure type hints
Improve code around creating the layout (see below)
Figure out details around opening login window (see below)
Understand what happens in regards to the login process in cached pages and make changes if necessary (see below)
Publish the mock service (and add some documentation on the expected service responses)

Layout creation

The current attempt at an implementation merely injects some DOM elements into an amp-access-laterpay-list element. Looking at the guidance here we could possibly ask the publisher to predefine a width/height for this to avoid reflows.

In such a case I feel like having an overlay rather than an in page list would be more appropriate, but we don't have a strong need here for one particular case or the other (in page or overlay), we can go with what works best for performance/ux reasons.

I'd also like to better understand if I need to use any specific pattern when rendering the list into the DOM. I've noticed the use of the vsync module in multiple places but don't entirely understand if I should be making use of it in this case or not.

Opening the login window

At the moment the code which opens the login window is unfinished. The current login method in the accessService takes as an argument a type which refers to the login type defined in the login configuration. Given that in this case we're getting the login urls from the service, I'd like to just pass that URL directly to the login method, given that I also don't wish to reimplement everything else the login method is doing, given that that is useful for our use case as well, and for handing back control to amp-access.

My proposal would be to make some changes to the login method to take an URL instead and have the URL lookup happen before that method gets called (or as another code path inside of the method, but the previous solution sounds cleaner to me).

What happens in cached pages?

At the moment, the configuration for our (LaterPay) service is defined in the module. While this shouldn't change in the future, in the case that it does it would probably mean that we'd have to publish a new version of this extension with a new configuration. In such a case we'd probably also advise our customers to make that update and follow the guidelines at https://developers.google.com/amp/cache/overview but given that we don't control the publisher page we can't ever be entirely sure if/when changes are properly made.

I was wondering if we should concern ourselves in any way with this potential situation for the case of this implementation and have any sort of fallback mechanism in case a user gets an out of date page trying to talk to a non existing authorization access point.

googlebot · 2016-10-31T18:38:38Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

dvoytenko · 2016-10-31T22:15:35Z

@trodrigues Thanks a lot! I set "WIP" label for now.

I'll review the initial code and send my comments along.

Opening the login window

I (or you :)) will expose the needed functionality via AccessService.openLoginDialog(url) method. The code in the login() is very close. The only difference, as you mention, is that we will pass url in directly. So, the existing login() method will call the new openLoginDialog method.

What happens in cached pages?

This depends on whether the configuration changes are in the documents or in your JS module. If it's your JS module - then it should be all handled by caches automatically - they update JS versions and force new version downloads very regularly. You will be looking at most 1wk deployment latency.

dvoytenko · 2016-11-02T19:45:19Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

 */

 import {accessServiceFor} from '../../../src/access-service';
+import {xhrFor} from '../../../src/xhr';


Please sort.

dvoytenko · 2016-11-02T19:45:33Z

examples/article-access-laterpay.amp.html

      "vendor": "laterpay",
-      "laterpay": {},
+      "laterpay": {
+        "access": true


Could you explain this please? Seems like this is not needed.

Yes, this is not needed. I'll remove it.

dvoytenko · 2016-11-02T19:48:36Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

    this.accessService_ = accessService;
-    // TODO: implement
+    this.win_ = this.accessService_.win;
+    this.laterpayConfig_ = this.accessService_.adapter_.getConfig();


We'll need to expose this as a public method on AccessService, e.g. getConfig(). This is because all private vars are obfuscated.

dvoytenko · 2016-11-02T19:49:01Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+    this.win_ = this.accessService_.win;
+    this.laterpayConfig_ = this.accessService_.adapter_.getConfig();
+    this.purchaseConfig_ = null;
+    this.purchaseOptionListeners_ = [];


Could you please add jsdoc for private vars?

dvoytenko · 2016-11-02T19:50:11Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+  }
+
+  getInitialPurchaseConfig_() {
+    const urlPromise = this.accessService_.buildUrl_(CONFIG_URL,


Let's keep for now. I'll find a way to pass context with config and buildUrl for you in a separate PR.

Has there been any progress on this one?

Feel free to expose buildUrl as a public method on the AccessService. But please keep the private buildUrl_ intact to avoid refactoring for now.

dvoytenko · 2016-11-02T19:52:05Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+      'amp-access-laterpay-list')[0];
+    const listContainer = document.createElement('ul');
+    // TODO set these up somewhere else and make them configurable
+    this.purchaseConfig_.premiumcontent.title = 'Buy this article';


A bigger question I have is how you'd make this translated?

The possibility I had thought about was to have it be supplied via the in page configuration parameters, which means the merchant could just server render it for whatever language they wanted. This is similar to what we do with our existing client side integration and meta tags: https://connectormwi.laterpay.net/docs/inpage_configuration/translations.html

Ok. This sounds good. But what's the default? English?

I mean in your current API...

Yeah, the default is English.

dvoytenko · 2016-11-02T19:52:29Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+      listContainer.appendChild(this.createPurchaseOption_(timepass));
+    });
+    const purchaseButton = document.createElement('button');
+    purchaseButton.innerHTML = 'Confirm your selection';


textContent = ....

oops, I keep forgetting about textContent

dvoytenko · 2016-11-02T19:52:44Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+    this.purchaseConfig_.timepasses.forEach(timepass => {
+      listContainer.appendChild(this.createPurchaseOption_(timepass));
+    });
+    const purchaseButton = document.createElement('button');


Ditto on this.win_.document.createElement

dvoytenko · 2016-11-02T19:53:27Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+    purchaseButton.innerHTML = 'Confirm your selection';
+    purchaseButton.disabled = true;
+    this.purchaseButton_ = purchaseButton;
+    listenOnce(purchaseButton, 'click', this.handlePurchase_.bind(this));


Is this really listenOnce? Is it possible for a user to cancel and select another option and try "purchase" again?

They should get redirected to our domain for the purchase flow when performing this action, so they would only be able to do that by coming back to the page.

But that's not what would happen with the "login dialog" when it runs in numerous different scenarios. E.g. it can be launched as a popup dialog. In this case, the page is still there. We will call for the re-authorization if anything changes and it's certainly possible to re-render this dialog if that's what you want. But this is why I was asking..

Ah yes, that makes sense.

I also just had a better look at the code of the login method in AccessService.

So after the loginPromise is resolved, should I keep the dialog around and only remove it on reauthorization if the authorization is successful, or should I remove it immediately and then re-render? I'm thinking the first approach is probably the better one.

Either option is up to you. But generally, I'd recommend to rerender the dialog from scratch since you'll get the best and freshest data again.

dvoytenko · 2016-11-02T19:53:41Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+    });
+  }
+
+  renderPurchaseOverlay_() {


trodrigues · 2016-11-10T14:01:35Z

Btw, just dropping a note to mention that I've been sidetracked by a few other things but I expect to resume work on this next week.

trodrigues · 2016-11-28T16:46:28Z

@dvoytenko In case you don't find them immediately, I added a couple of replies in comments that became outdated after my last push.

dvoytenko · 2016-11-29T18:18:58Z

@trodrigues Thanks! Replied. Let me know when it's ready for a review again. And apologies for the delay. Indeed GitHub makes it hard to know when the overall scope is ready for response and people tend to reply bit-by-bit before sending an overall response. If not too hard, could you please send a note "PTAL again" once ready?

trodrigues · 2016-12-06T17:34:44Z

@dvoytenko still need to add tests for amp-access-laterpay and update the amp access service tests, but I've implemented those changes to the login methods, and did most of the remaining work on our implementation.

I still have a couple of known bugs to fix and one more feature to add (already purchased link) and also need to do some cleanup on the mock service I want to publish separately to test this with.

In any case, I'd appreciate a review of these new changes, specially on the changes made to the other modules outside of amp-access-laterpay.

dvoytenko · 2016-12-08T17:05:12Z

examples/article-access-laterpay.amp.html

-      "laterpay": {},
-      "authorizationFallbackResponse": {"error": true}
+      "authorizationFallbackResponse": {"error": true},
+      "noPingback": true,


Why noPingback = true? I don't think you want this. The access system will delegate this call to the pingback method in your service and you can process it as you like, but it's better not advise it in the sample docs.

Oh, right, I can override that call. I'll do that then.

dvoytenko · 2016-12-08T17:08:24Z

examples/article-access-laterpay.amp.html

-
        <section amp-access="NOT error AND NOT access" amp-access-hide class="login-section">
-          <a on="tap:amp-access.login">Login to read more!</a>
+          <amp-access-laterpay-list></amp-access-laterpay-list>


So, I looked through this in more detail, and I have two points/questions:

Would the name amp-access-laterpay-dialog be better for this?

Do you have a default action that you can take if a publisher didn't specify this tag? E.g. is it possible to append the dialog to the bottom of the page? Or something like this?

The way custom elements are supported in AMP, this maybe better to switch to use an element with id, e.g. <div id="amp-access-laterpay-dialog">. WDYT? If you prefer a tag name - I'll update our validator to allow it and will ensure that AMP runtime does not try to "manage" this tag, it's all possible, but I wanted to discuss.

Given that people can potentially customize the look and feel of it, some might prefer displaying it as a dialog while others might prefer displaying it as just a section below an article. But in any case, I don't have any strong preference for one name or the other.

Right now, no, and we'd like to give the users some measure of control over where this shows up, rather than just having a dialog pop up over all of their content. What we do in our existing client side integration is to have them specify a selector for an element over which our dialog will be placed. So we also don't have a "default" action there, it's always something that requires that little bit of configuration. AFAIK so far this hasn't been a major issue specially because the publishers we've worked with so far seem to like to have that measure of control.

Either would be fine, also don't have a special preference on this. Just assumed this would be the best way for doing this in AMP.

Ok. Let's start with id="amp-access-laterpay-dialog" format. And we'll adjust as needed.

dvoytenko · 2016-12-08T17:09:25Z

extensions/amp-access/0.1/amp-access-vendor.js

+    this.vendorConfig_ = configJson[this.vendorName_];
+
+    /** @const @private {boolean} */
+    this.isPingbackEnabled_ = !configJson['noPingback'];


Asked a question about it above. If you don't need pingback at this time (though I think you probably do need it) you can just do a no-op in your service.

Ping on this question.

I ended up changing that on the service, but I thought it should still make sense to return the configuration value here.

True. It does. Let's keep.

dvoytenko · 2016-12-08T17:10:50Z

extensions/amp-access/0.1/amp-access.js

+  /**
+   * Runs the login flow using one of the predefined urls in the amp-access config
+   *
+   * @param {string} type Type of login defined in the config


Please also add @private

dvoytenko · 2016-12-08T17:13:03Z

extensions/amp-access/0.1/amp-access.js

+  loginWithType_(type) {
+    // Login URL should always be available at this time.
+    const loginUrl = user().assert(this.loginUrlMap_[type],
+        'Login URL is not ready: %s', type);


Could you change the order? The loginConfig_ assert should go before loginUrlMap_: the record absent in the loginConfig_ is a misconfiguration and it will always mean that loginUrlMap_ is also missing. The reverse is not true, however.

Ah right, I somehow reordered this when moving it from the other method, not sure why.

dvoytenko · 2016-12-08T17:20:27Z

extensions/amp-access/0.1/amp-access.js

+   * @param {string} url
+   * @return {!Promise}
+   */
+  loginWithUrl(url) {


We can't use url as an analytics label. Two options we have:

No label at all (eventLabel == empty string). This will result in analytics events that look like this: access-login-success.

A label by the name of you source, e.g. "laterpay". The analytics event would look like access-login-laterpay-success. You can pass "laterpay" as an argument to this method.

Overall, I'd say, if you only supply "laterpay" as an event type, there's no a lot of point in having it at all and we can just go with ''. However, of you'll have a couple of different login types, it might be a good idea to provide them. E.g. "laterpay-login1", "laterpay-login2" - this sort of thing.

Ah, we do have different purchase types, pay per use or single item sale (time passes), which we essentially use as the different login urls, so that would probably be the correct thing to pass along here.

dvoytenko · 2016-12-08T17:21:15Z

extensions/amp-access/0.1/amp-access.js

-    // Login URL should always be available at this time.
-    const loginUrl = user().assert(this.loginUrlMap_[type],
-        'Login URL is not ready: %s', type);
+    dev().fine(TAG, 'Start login: ', eventLabel);


Let's do fine(TAG, 'Start login: ', loginUrl, eventLabel)

dvoytenko · 2016-12-08T17:22:47Z

src/service/xhr-impl.js

    if (response.status < 200 || response.status >= 300) {
      /** @const {!Error} */
      const err = user().createError(`HTTP error ${response.status}`);
+      err.response = response;


Do you use it anywhere? It's an interesting idea, but if there's some specific need - it might be better to just parse it out and fill it in here specifically rather leave it up to the client to dig in the XHR response?

I am, here.

That makes me ask: did you see the diff of the amp-access-laterpay.js file? I saw someone complain about this on twitter the other day, but apparently when diffs are too big (I guess) it just does this:

I was looking at the Files Changed tab and thinking "where did all of that code go? I wrote more code than this" and that's when I noticed this collapsed diff.

Maybe that's what it was. I see it now. Thanks!

dvoytenko · 2016-12-08T17:24:43Z

@trodrigues This looks great! I made a few comments, but nothing major. The changes to the login API look exactly how I imagined them. It looks like you didn't get yet to exposing public buildUrl on the service, so I'll re-check it later. But otherwise it looks good.

trodrigues · 2016-12-09T13:35:54Z

Ah right, I didn't change the buildUrl because of your previous comment that you'd change that. I'll do something on that then.

dvoytenko · 2016-12-09T21:29:29Z

@trodrigues I made couple of responses, but I think we are all good, provided we address buildUrl. Is there anything else you wanted to address in this PR? Let me know and I'll merge.

One more thing though - this might be a good time to add some content into amp-access-laterpay.md?

trodrigues · 2016-12-13T17:52:17Z

I've fixed the issues you mentioned, as well as the broken tests for amp-access and amp-access-vendor.

Biggest outstanding issues:

finish writing the tests for amp-access-laterpay
writing up the readme

I'm going to be away on leave until the end of year so this will wait until then.

In the meanwhile, I also wanted to ask a question about something we noticed while using a demo for this.

After logging in, when you get back to the page the scroll position is always maintained. Depending on where the dialog is placed (and if the publisher makes some content available previously or not) this can be a bit confusing or weird. Would it be ok if we included a window.scrollTo(0, 0) on handling a successful authorization (ideally behind a configuration parameter which would make it optional) ?

dvoytenko · 2016-12-14T21:02:26Z

@trodrigues Action items sound good.

Would it be ok if we included a window.scrollTo(0, 0) on handling a successful authorization (ideally behind a configuration parameter which would make it optional) ?

Is this in popup case or redirect case? Generally, I'd say it's fine if it's behind the configuration flag. You'd need to use viewport.setScrollTop.

dvoytenko · 2017-01-09T21:42:25Z

@trodrigues (Happy New Year!) I saw the commit. Should I re-review?

trodrigues · 2017-01-10T17:52:10Z

Hi there! thanks :) Yes, you can have another look now. I was just finishing up a couple of other things.

So now there's tests and documentation I've also added the feature we discussed about the scroll to top, and an optional header/footer where publishers can potentially set a title and a footer where they can add some further information (like the example you had once pointed out from SPIEGEL where they add some extra explanation about our service).

Other than all this, I'm still waiting for our server endpoint to be finished up and deployed, so I'd leave this marked as experimental until that's ready.

trodrigues · 2017-01-10T17:56:00Z

Oh, forgot something!

I noticed other extended components have a validation section in the readme with a link to some validation rules. Do we also need to add this? Can you give me a bit more info on how to go about that if so?

Also, I'll see about adding also something to ampbyexample.com

dvoytenko · 2017-01-10T23:27:06Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.css

@@ -0,0 +1,55 @@
+.amp-access-laterpay {
+  padding: 16px;
+  width: 420px;


What would happen in, e.g., iPhone6 with 375px width? E.g. do we need max-width: 100% here or something like that?

dvoytenko · 2017-01-10T23:29:31Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+    this.laterpayConfig_ = this.accessService_.getAdapterConfig();
+
+    /** @const @private {!PurchaseConfigDef} */
+    this.purchaseConfig_ = null;


Doesn't match type since it's defined as non-nullable (!). Also, I think it's not a @const.

Oh right. weirdly enough I didn't get any warnings/errors when building it. Should that have happened?

I turned off type compilation on your extension to avoid distracting you too much. So I only comment on the structural things. When we are ready, I'll turn it on and do few minor fixes in one PR.

dvoytenko · 2017-01-10T23:29:40Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+    this.purchaseConfig_ = null;
+
+    /** @const @private {?Function} */
+    this.purchaseButtonListener_ = null;


Ditto, is this really a @const? And few below...

dvoytenko · 2017-01-10T23:31:14Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+    this.purchaseConfigBaseUrl_ = configUrl + CONFIG_BASE_PATH;
+    const articleId = this.laterpayConfig_.articleId;
+    if (articleId) {
+      this.purchaseConfigBaseUrl_ += '&article_id=' + articleId;


Please use encodeURIComponent(articleId)

Ah thanks, I think I fixed it somewhere else but forgot it here.

dvoytenko · 2017-01-10T23:35:34Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+  getArticleTitle_() {
+    const title = this.doc_.querySelector(
+      this.laterpayConfig_.articleTitleSelector);
+    dev().assert(


This should be user().assert() since the publisher has to see this error. Also, is this config property strictly required? Any reasonable default such as document.title?

Right, I actually was unsure of that and forgot to ask.

I believe it is strictly required because we show it in the payment dialog (so the user knows what they're paying for) but I'll look into what we do in our existing frontend integration.

Strict requirement: agree. But if we can try to default, e.g. to this.doc_.title - I see it as a good thing - let's duplicative configuration when they are exactly the same. But if in your experience they are almost never the same - let's keep the way it is.

Ok, so I was thinking, use this as a default yes, but log a warning message when in dev mode.

dvoytenko · 2017-01-10T23:40:21Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js


  /**
-   * @return {!Promise}
+   * @returns {boolean}


@return {boolean}. Though, the interface defines it as Promise?

dvoytenko · 2017-01-10T23:41:20Z

extensions/amp-access-laterpay/amp-access-laterpay.md

@@ -0,0 +1,241 @@
+<!---
+Copyright 2015 The AMP HTML Authors. All Rights Reserved.


dvoytenko · 2017-01-10T23:42:26Z

extensions/amp-access/0.1/amp-access-vendor.js

+    this.vendorConfig_ = configJson[this.vendorName_];
+
+    /** @const @private {boolean} */
+    this.isPingbackEnabled_ = !configJson['noPingback'];


Ping on this question.

dvoytenko · 2017-01-10T23:44:07Z

extensions/amp-access/0.1/amp-access.js

   */
-  login(type) {
+  login_(loginUrl, eventLabel) {
    const now = Date.now();


Please add @private in the jsdoc.

dvoytenko · 2017-01-10T23:45:32Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+    }, err => {
+      const status = err && err.response && err.response.status;
+      if (status === 402) {
+        this.purchaseConfig_ = err.responseJson;


I'm not sure I'm seeing responseJson actually set anywhere. Could you please confirm?

This is set by the xhr service, in the callback used in the fetchJson method: https://github.com/ampproject/amphtml/blob/master/src/service/xhr-impl.js#L436

Is that what you meant?

I see. Sorry, I thought it was you change. LG.

dvoytenko · 2017-01-10T23:48:31Z

Thanks, @trodrigues! I left few more comments, but nothing major. I'll ask someone to add the validator config for you, so don't worry about that.

dvoytenko · 2017-01-12T21:54:17Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+
+  /**
+   * @param {!string} href
+   * @returns {!Node}


Ditto: @return

dvoytenko · 2017-01-12T21:54:23Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+
+  /**
+   * @private
+   * @returns {!Promise}


It's @return (no "s" at the end).

Bad habits from using some jsdoc flavours that allow both 😁

dvoytenko · 2017-01-12T21:54:34Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js


  /**
-   * @return {!Promise}
+   * @returns {!Promise}


Ditto: @return

dvoytenko · 2017-01-12T21:55:27Z

extensions/amp-access/0.1/amp-access-vendor.js

+    this.vendorConfig_ = configJson[this.vendorName_];
+
+    /** @const @private {boolean} */
+    this.isPingbackEnabled_ = !configJson['noPingback'];


True. It does. Let's keep.

dvoytenko · 2017-01-12T21:57:59Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+  getArticleTitle_() {
+    const title = this.doc_.querySelector(
+      this.laterpayConfig_.articleTitleSelector);
+    dev().assert(


Strict requirement: agree. But if we can try to default, e.g. to this.doc_.title - I see it as a good thing - let's duplicative configuration when they are exactly the same. But if in your experience they are almost never the same - let's keep the way it is.

dvoytenko · 2017-01-12T22:00:20Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+    this.purchaseConfigBaseUrl_ = configUrl + CONFIG_BASE_PATH;
+    const articleId = this.laterpayConfig_.articleId;
+    if (articleId) {
+      this.purchaseConfigBaseUrl_ += '&article_id=' + encodeURIComponent(articleId);


Linter complains that this line is too long.

dvoytenko · 2017-01-12T22:01:45Z

@trodrigues Great! Just a few small comments and I'll be able to merge. At the same time, I'm almost done with validator changes, so it should be all ready on AMP's side.

jridgewell · 2017-01-12T23:41:20Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+    /** @private {?Function} */
+    this.alreadyPurchasedListener_ = null;
+
+    /** @const @private {!Array<Event>} */


{!Array<function(!Event)>}?

jridgewell · 2017-01-12T23:42:16Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+    this.currentLocale_ = this.laterpayConfig_.locale || 'en';
+
+    /** @private {Object} */
+    this.i18n_ = Object.assign(DEFAULT_MESSAGES,


This will mutate DEFAULT_MESSAGES. You can use Object.assign({}, DEFAULT_MESSAGES, ...) to avoid.

Ah yes, thanks. Shouldn't be the biggest of issues because DEFAULT_MESSAGES wouldn't ever really get reused but it makes sense.

jridgewell · 2017-01-12T23:43:50Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+    /** @const @private {!Xhr} */
+    this.xhr_ = xhrFor(this.win_);
+
+    installStyles(this.win_.document, CSS, () => {}, false, TAG);


Should this be done in the constructor? Can we ever have multiple instances?

There shouldn't be multiple instances of this extension, no, so I'd rather add a check for that.

Given that the instance of this class is already created at the end of this file when the vendor is registered, I'm wondering it it's already the case?

jridgewell · 2017-01-12T23:49:19Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+      } else {
+        throw err;
+      }
+      return {access: err.responseJson.access};


err.responseJson is not garaunteed since buildUrl or timeoutPromise could cause the error.

Ah right. That value should always be false in this case anyway, so there's really no need to get it from the response anyway.

jridgewell · 2017-01-12T23:50:08Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+          AUTHORIZATION_TIMEOUT,
+          this.xhr_.fetchJson(url, {
+            credentials: 'include',
+            requireAmpResponseSourceOrigin: true,


This has been deprecated, right @lannka?

jridgewell · 2017-01-12T23:51:39Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+  getContainer_() {
+    const id = TAG + '-dialog';
+    const dialogContainer = this.doc_.getElementById(id);
+    user().assert(


Nit: We can return user().assertElement(), which will allow us to return a {!Element}.

Oh, yes, that's right. I saw that pattern somewhere in existing code but forgot about it. Thanks!

jridgewell · 2017-01-12T23:52:26Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+      this.purchaseButtonListener_();
+    }
+    if (this.alreadyPurchasedListener_) {
+      this.alreadyPurchasedListener_();


We should null this out afterwards.

jridgewell · 2017-01-12T23:52:38Z

extensions/amp-access-laterpay/0.1/amp-access-laterpay.js

+      unlistener();
+    }
+    if (this.purchaseButtonListener_) {
+      this.purchaseButtonListener_();


We should null this out afterwards.

dvoytenko · 2017-01-17T17:08:33Z

@trodrigues @jridgewell I'm all good to merge on my side. Let me know if anything still outstanding.

I see that Travis is red with this error:

src/service/xhr-impl.js:431: ERROR - actual parameter 1 of isRetriable$$module$src$service$xhr_impl does not match formal parameter
found   : number
required: string
      if (isRetriable(response.status)) {

Sounds like there's some typing issue. PTAL.

I can merge as soon as Travis is green.

- add private tag - ensure assert comes first

Also rename i18n keys for clarity.

trodrigues · 2017-01-17T17:58:51Z

Hah, this was a weird one. The typechecker output is:

found   : number
required: string

But the values actually used inside of the method are numbers.

I did change some stuff in this method but on first thought, nothing that would affect this value. Then looking at my changes, I remembered I fixed a typo on the type hints for the existing code:
https://github.com/ampproject/amphtml/pull/5920/files#diff-eccb89b08543ad578ace4060bef469d5L419

Changing the actual expected type in the isRetriable method fixes it properly.

dvoytenko · 2017-01-17T18:25:07Z

@trodrigues Yes, clearly isRetriable is incorrect. Could you please update it to number and push?

- add license to css file - rename property to avoid scrollTop check

trodrigues · 2017-01-17T18:46:42Z

Had already done so but that exposed a couple of other issues which I also just pushed a fix for.

dvoytenko · 2017-01-17T19:08:41Z

Merged! Thanks a lot!

trodrigues · 2017-01-18T14:55:16Z

Thank you too :) When our server endpoints are ready and we try everything together I'll open a PR to remove the experimental flag. Anything else I should consider when doing that?

dvoytenko · 2017-01-18T20:59:23Z

@trodrigues Whenever ready, yes, send me the launch PR where experiment is removed and indicate if you want an immediate launch or wait for normal deployment cycle. Our normal cycle takes the changes 1 week to canary and the following week to PROD, but I can launch experiments immediately if desired.

* Early implementation of amp-access-laterpay * Remove unused amp-access-laterpay config params * Code cleanup issues from PR * Expose response in error object for assertSuccess * Further work on amp-access-laterpay - Remove unnecessary pingback skeleton - Add jsdoc annotations for existing code - Laterpay config retrieval based on draft spec - Improve rendering of the purchase overlay - Price formatting and localization * Expose adapter config on AccessService * Fix pingback detection in amp-access-vendor * Allow amp-access login flow with url only Create new methods loginWithType_ (similar to old login method) and loginWithUrl as entry points to old login method (now private). * Fix exposure of amp access vendor config * Add "already purchased" link * Add CSS for amp-access-laterpay * PR feedback fixes: - add private tag - ensure assert comes first * Add event label to loginWithUrl calls * Also check if we're on localdev mode * Fix amp-access login tests * Fix amp-access-vendor tests * Use id instead of custom element for amp-access-laterpay * Override pingback in amp-access-laterpay * Expose buildUrl in amp-access * Add tests for amp-access-laterpay * Simplify example page * Apply labels properly based on purchase type Also rename i18n keys for clarity. * Add readme for amp-access-laterpay * Typos, cleanup, linting * Add option for scrolling to top after auth * Add header/footer to amp-access-laterpay * Fix some default styling issues * Address PR feedback and update LaterPay description * Address more PR feedback fixes * Add validator info to amp-access-laterpay readme * Add types for missing config options * Add sandbox flag to LaterPay config options * Fix some linting and rules issues - add license to css file - rename property to avoid scrollTop check

dvoytenko added the WIP label Oct 31, 2016

dvoytenko self-assigned this Oct 31, 2016

dvoytenko suggested changes Nov 2, 2016

View reviewed changes

dvoytenko suggested changes Dec 8, 2016

View reviewed changes

dvoytenko suggested changes Jan 10, 2017

View reviewed changes

honeybadgerdontcare mentioned this pull request Jan 11, 2017

Laterpay validator config #6974

Merged

dvoytenko reviewed Jan 12, 2017

View reviewed changes

dvoytenko suggested changes Jan 12, 2017

View reviewed changes

jridgewell reviewed Jan 12, 2017

View reviewed changes

dvoytenko approved these changes Jan 17, 2017

View reviewed changes

Tiago Rodrigues added 3 commits January 17, 2017 18:51

Early implementation of amp-access-laterpay

37902fd

Remove unused amp-access-laterpay config params

86f3c1d

Code cleanup issues from PR

3dae066

Tiago Rodrigues added 20 commits January 17, 2017 18:51

PR feedback fixes:

0b71883

- add private tag - ensure assert comes first

Add event label to loginWithUrl calls

de01043

Also check if we're on localdev mode

6fa13bb

Fix amp-access login tests

fca007a

Fix amp-access-vendor tests

2f6bfe2

Override pingback in amp-access-laterpay

54d84ab

Expose buildUrl in amp-access

56134a6

Add tests for amp-access-laterpay

a4b542d

Simplify example page

cb9b28e

Apply labels properly based on purchase type

cb77f09

Also rename i18n keys for clarity.

Add readme for amp-access-laterpay

7fd4627

Typos, cleanup, linting

7e871e5

Add option for scrolling to top after auth

1b11532

Add header/footer to amp-access-laterpay

6d0dddf

Fix some default styling issues

637dfc5

Address PR feedback and update LaterPay description

35113c2

Address more PR feedback fixes

52e28a0

Add validator info to amp-access-laterpay readme

b67a360

Add types for missing config options

98c229e

Add sandbox flag to LaterPay config options

2599d34

Fix some linting and rules issues

8df91aa

- add license to css file - rename property to avoid scrollTop check

dvoytenko merged commit 1972fac into ampproject:master Jan 17, 2017

		@@ -0,0 +1,241 @@
		<!---
		Copyright 2015 The AMP HTML Authors. All Rights Reserved.

Conversation

trodrigues commented Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Layout creation

Opening the login window

What happens in cached pages?

Uh oh!

googlebot commented Oct 31, 2016

Uh oh!

dvoytenko commented Oct 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trodrigues Nov 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trodrigues commented Nov 10, 2016

Uh oh!

trodrigues commented Nov 28, 2016

Uh oh!

dvoytenko commented Nov 29, 2016

Uh oh!

trodrigues commented Dec 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

trodrigues commented Oct 31, 2016 •

edited

Loading

trodrigues Nov 28, 2016 •

edited

Loading