Subscriptions: New entitlement shape by prateekbh · Pull Request #15059 · ampproject/amphtml

prateekbh · 2018-05-03T20:13:56Z

Rewriting the entitlement object

…ew-entitlement

prateekbh · 2018-05-03T21:15:39Z

extensions/amp-subscriptions/0.1/entitlement.js

-  constructor({source, raw = '', service, products = [],
-    subscriptionToken = '', loggedIn = false, metering = null}) {
+  constructor({source, raw = '', service, granted = false,
+    grantReason = '', data}) {


data here throws

The type information available for this expression is too loose to ensure conformance.

I don't know a workaround

For data or for grantReason? B/c I could see why with grantReason...

Do you also want to default data to null?

If this doesn't work - you may have to define a @typedef.

dvoytenko · 2018-05-04T01:21:28Z

extensions/amp-subscriptions/0.1/amp-subscriptions.js

 const SERVICE_TIMEOUT = 3000;

-/** @typedef {{loggedIn: boolean, subscribed: boolean, granted: boolean, entitlement: !JsonObject, metered: boolean}} */
+/** @typedef {{granted: boolean, subscribed: boolean, entitlement: !JsonObject, }} */


Can we remove this object completely? In general isn't it now more or less 100% equal to Entitlement object? I'd really like to get rid of this object, tbh.

If not, can we then add grantReason here?

Yep, removed RenderState

dvoytenko · 2018-05-04T01:23:21Z

extensions/amp-subscriptions/0.1/entitlement.js


-/** @typedef {{left: number, total: number, resetTime: number, durationUnit: string, token: string}} */
-export let MeteringData;
+export const GrantReasons = {


Let's add @enum {string} and remove quotes for names. I.e.:

/** @enum {string} */ export const GrantReason = { SUBSCRIBER: 'SUBSCRIBER', METERING: 'METERING', }

Also, I like "SUBSCRIBER" a bit better than "SUBSCRIBED". Feel free to argue your point though - i don't feel very strongly about it.

One thing for sure though. GrantReason must be singular, not plural.

I am not sure what do you mean by singular?

Plus yeah no strong reason for SUBSCRIBED as well so i can change if you want

Singular I mean "GrantReason" vs "GrantReasons". And, after some thinking, yeah, let's use "subscriber"

dvoytenko · 2018-05-04T01:26:08Z

extensions/amp-subscriptions/0.1/entitlement.js

-  constructor({source, raw = '', service, products = [],
-    subscriptionToken = '', loggedIn = false, metering = null}) {
+  constructor({source, raw = '', service, granted = false,
+    grantReason = '', data}) {


For data or for grantReason? B/c I could see why with grantReason...

Do you also want to default data to null?

If this doesn't work - you may have to define a @typedef.

dvoytenko · 2018-05-04T01:26:53Z

extensions/amp-subscriptions/0.1/entitlement.js

@@ -79,10 +72,9 @@ export class Entitlement {
      'raw': this.raw,


I think we actually should remove raw from json() form. In pingback it's ok since it serves a pretty different purpose.

dvoytenko · 2018-05-04T01:28:14Z

extensions/amp-subscriptions/0.1/entitlement.js

      'raw': this.raw,
      'source': this.source,
-      'grantState': this.enablesThis(),
+      'grantState': this.granted,


I think we should apply the same form with granted and grantReason. Probably data too.

Made this raw + json()

dvoytenko · 2018-05-04T01:34:24Z

extensions/amp-subscriptions/0.1/platform-store.js

    this.grantStatusEntitlementPromise_ = new Promise(resolve => {
-      if ((this.grantStatusEntitlement_
-          && this.grantStatusEntitlement_.subscriptionToken)
+      if ((this.grantStatusEntitlement_ && this.grantStatusEntitlement_.granted)


Hmm. I think the old code actually meant what it said with .subscriptionToken. I.e. we need to be checking .isSubscribed() here. I believe the idea was to only do early resolution of promise for a subscriber and wait until later for other types of grant. This also means a good comment is in order :)

oh yeah may bad, should be checking .isSubscribed

dvoytenko · 2018-05-04T01:34:43Z

extensions/amp-subscriptions/0.1/platform-store.js


      // Subscriber wins immediatly.
-      if (!!entitlement.subscriptionToken) {
+      if (!!entitlement.isSubscribed()) {


It's strictly boolean so let's remove !!

dvoytenko · 2018-05-04T03:05:30Z

extensions/amp-subscriptions/0.1/amp-subscriptions.js

        entitlement: selectedEntitlement.json(),
-        loggedIn: selectedEntitlement.loggedIn,
-        subscribed: !!selectedEntitlement.subscriptionToken,
+        subscribed: !!selectedEntitlement.isSubscribed(),


Typo. Removed.

dvoytenko · 2018-05-04T03:08:26Z

extensions/amp-subscriptions/0.1/viewer-subscription-platform.js

-              Entitlement.parseFromJson(entitlements[index], token);
-          if (entitlementObject.enables(currentProductId)) {
-            entitlement = entitlementObject;
+          if (entitlements[index].products.indexOf(currentProductId) !== -1) {


So now you're accessing a json object, right? In that case you need to reference the fields with quotes.

dvoytenko · 2018-05-04T03:11:54Z

extensions/amp-subscriptions/0.1/entitlement.js

-   * @param {?string} [input.subscriptionToken]
-   * @param {boolean} [input.loggedIn]
-   * @param {?MeteringData} [input.metering]
+   * @param {boolean} [input.granted]


In this case we should also update amp-subscriptions-google platform in this PR, right?

dvoytenko

Almost there. Couple more clarifications.

dvoytenko · 2018-05-05T03:09:08Z

extensions/amp-subscriptions/0.1/amp-subscriptions.js

-
-      selectedPlatform.activate(renderState);
+
+      selectedPlatform.activate(selectedEntitlement.json());


Let's just pass the entitlement itself. The platform can always call json() on it itself if it wants to :)

dvoytenko · 2018-05-05T03:10:21Z

extensions/amp-subscriptions-google/0.1/amp-subscriptions-google.js

    // Offers or abbreviated offers may need to be shown depending on
    // whether the access has been granted and whether user is a subscriber.
-    if (!renderState.granted) {
+    if (!renderState['granted']) {


Ditto: let's pass the actual Entitlement object. Then it'd be a cleaner:

if (!entitlement.granted) {} .. if (!entitlement.isSubscriber()) {} ...

ghost · 2018-05-07T22:31:23Z

This pull request introduces 1 alert when merging 0cb9b43 into 9beaf37 - view on lgtm.com

new alerts:

1 for Property access on null or undefined

Comment posted by lgtm.com

…ew-entitlement

dvoytenko · 2018-05-10T16:06:29Z

extensions/amp-subscriptions-google/0.1/amp-subscriptions-google.js

+    if (!entitlement.granted) {
      this.runtime_.showOffers({list: 'amp'});
-    } else if (!renderState.subscribed) {
+    } else if (entitlement.grantReason !== GrantReason.SUBSCRIBER) {


Why not just use entitlement.isSubscriber()?

dvoytenko · 2018-05-10T16:13:49Z

extensions/amp-subscriptions/amp-subscriptions.md

-    "products": ["norcal_tribune.com:basic",...],
-    "subscriptionToken": "subscription-token",
-    "loggedIn": true/false,
+    "granted": true


…ew-entitlement

prateekbh added 6 commits May 1, 2018 09:48

WIP new entitlement

ffe9caf

new entitlement shape

237c818

fixing tests

0ad6583

Merge branch 'master' of https://github.com/ampproject/amphtml into n…

113424d

…ew-entitlement

fixing tests

24e6d12

fixing more tests

5ffbc33

prateekbh requested a review from dvoytenko May 3, 2018 20:13

googlebot added the cla: yes label May 3, 2018

fixing tests

bda3109

prateekbh commented May 3, 2018

View reviewed changes

dvoytenko suggested changes May 4, 2018

View reviewed changes

fixing comments

2d75a7d

dvoytenko suggested changes May 5, 2018

View reviewed changes

prateekbh added 3 commits May 7, 2018 10:02

fixing grant reasons

9fc9520

fixing viewer subscriptions

37e8688

new entitlements shape

0cb9b43

prateekbh mentioned this pull request May 8, 2018

Subscriptions: fixing examples #15129

Merged

prateekbh added 3 commits May 9, 2018 13:17

bug fix

868e09b

fixing type

9d96a5a

Merge branch 'master' of https://github.com/ampproject/amphtml into n…

1618ffe

…ew-entitlement

dvoytenko approved these changes May 10, 2018

View reviewed changes

prateekbh added 6 commits May 10, 2018 14:19

fixing tests

7491700

fixing comments

0be84ec

fixing tests

27e3dc6

fixing lint

2f0003b

Merge branch 'master' of https://github.com/ampproject/amphtml into n…

dbbd66c

…ew-entitlement

Merge branch 'master' of https://github.com/ampproject/amphtml into n…

264991d

…ew-entitlement

prateekbh merged commit 04c5b80 into ampproject:master May 12, 2018

prateekbh deleted the new-entitlement branch May 12, 2018 02:22

		@@ -79,10 +72,9 @@ export class Entitlement {
		'raw': this.raw,


		selectedPlatform.activate(renderState);

		selectedPlatform.activate(selectedEntitlement.json());

Conversation

prateekbh commented May 3, 2018

Uh oh!

prateekbh May 3, 2018 • 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!

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!

dvoytenko left a comment

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!

ghost commented May 7, 2018

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

prateekbh May 3, 2018 •

edited

Loading