Subscriptions: Better platform selection #14004

prateekbh · 2018-03-14T22:37:28Z

better platform selection with weights
default platform selection is always local
refactored entitlementStore to PlatformStore

…pingback

…ullEntitlements

dvoytenko · 2018-03-14T23:15:34Z

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

   */
  pingback() {}
+
+  /** override */


@override

dvoytenko · 2018-03-14T23:19:04Z

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

+      if (this.viewTrackerPromise_) {
+        this.viewTrackerPromise_.then(() => {
+          const localPlatform = /** @type {!LocalSubscriptionPlatform} */ (
+            user().assert(this.platformStore_.getLocalPlatform(),


I think the user().assert() should just go to getLocalPlatform implementation itself.

Ah yes, it already is, should just remove from here

dvoytenko · 2018-03-14T23:19:37Z

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

@@ -327,7 +321,7 @@ export class SubscriptionService {
   */
  delegateActionToLocal(action) {
    const localPlatform = /** @type {LocalSubscriptionPlatform} */ (
-      dev().assert(this.subscriptionPlatforms_['local'],
+      dev().assert(this.platformStore_.getLocalPlatform(),


Then this goes away as well.

Ah yes, it already is, should just remove from here

dvoytenko · 2018-03-14T23:19:46Z

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

@@ -218,6 +218,11 @@ export class LocalSubscriptionPlatform {
      });
    });
  }
+
+  /** override */


@override

dvoytenko · 2018-03-14T23:21:35Z

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

+   * @param {string} servideId
+   * @returns {!./subscription-platform.SubscriptionPlatform}
+   */
+  getPlatform(servideId) {


This method seems a bit dangerous. It looks like it's fine for local platform, but not for others. I recommend you make it private. Eventually if we need something like this for non-local platforms, we'll add a new method with promise.

dvoytenko · 2018-03-14T23:23:30Z

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

+  getAllResolvedPlatforms() {
+    const platforms = [];
+    for (const platformKey in this.subscriptionPlatforms_) {
+      if (this.subscriptionPlatforms_.hasOwnProperty(platformKey)) {


No need for hasOwnProperty here. But if you'd like, you should use hasOwn from object.js utils to avoid compatibility issues.

dvoytenko · 2018-03-14T23:35:43Z

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

+   * Returns all the platforms;
+   * @returns {!Array<!./subscription-platform.SubscriptionPlatform>}
+   */
+  getAllResolvedPlatforms() {


Does it need to be public? The name is also confused - this is a registered platform, not resolved since "resolved" further used to mean "entitlements are known".

Nups, and yes bad name as well. Changed

dvoytenko · 2018-03-14T23:37:27Z

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

+
+      // If supports the current viewer, gains weight 9
+      if (platform.supportsCurrentViewer()) {
+        platformWeights[platform.getServiceId()] =


Calculate via local var and assign to the map in the end when the score is ready.

I dont really understand this one completely but, I guess i have flattened the algo a bit

dvoytenko · 2018-03-14T23:42:08Z

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

+    const localPlatform = this.getLocalPlatform();
+
+    /** @type {!Object<string, number>} */
+    const platformWeights = {};


Why not just build weightSorted array right away? Instead of building a temp map and then copying it over? Further, you keep looking up by serviceId, but you already have platform instances. You can do this:

/** @type {!Array<{platform: Platform, weight: number}>} */ const platformWeights = []; forEach(platform => { const weight = calculayeWeight(platform, entitlement); platformWeights.push({platform, weight}); }); platformWeights.sort((p1, p2) { return p2.weight - p1.weight; }); // the rest as you have

dvoytenko · 2018-03-14T23:43:40Z

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

+
+    this.getAllResolvedPlatforms().forEach(platform => {
+      const entitlement =
+          this.getResolvedEntitlementFor(platform.getServiceId());


You are iterating here over getAllResolvedPlatforms(), while calling getResolvedEntitlemntFor. But they are not resolved in tandem (one simply depends on subscriptionplatforms_ var, the other on entitlements_ var). Thus you are spilling too much asynchronous state via synchronous and, it looks to me, bugs are very likely.

Also, we need to minimize public methods returning synchronous/resolved stuff - there shouldn't be any need for them. It's ok to have some minimal API for local platform, but everything else should be async.

To fix this particular issue, however, it looks like you can simply need to ignore a platform that doesn't have entitlements yet. Or you can wait until all platforms have been resolved, but that's not what getAllResolvedPlatforms does know as far as I can tell.

getAllResolvedPlatforms() is just wrongly named and is more like an iteration over the resolved ones.

Also the entire thing starts after getAllPlatformsEntitlements_ so it should be safe to assume, we might not run into race condition thingy

dvoytenko

LGTM with couple comments.

dvoytenko · 2018-03-15T18:40:35Z

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

+   *
+   * In the end candidate with max weight is selected.
+   * However if candidate's weight is equal to local platform, then local platform is selected.
+   * @returns {!./subscription-platform.SubscriptionPlatform}


Add @private

dvoytenko · 2018-03-15T18:42:10Z

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

+
+  /** @override */
+  supportsCurrentViewer() {
+    return false;


Please return this.isGoogleViewer_ here since that PR is merged now.

…etterPlatformSelection

* adding pingback * adding test * adding viewer tracker * adding tests for viewer tracker * adding more tests * fixing tests * fixing tests * fixing tests * adding undefined for entitlements * fixing all subscriptions denied use case * Building platformStore * adding parameters over platform selection * adding tests for select platform * fixing comments * type fixes * adding assert in select platform * fixing scope * fixing types

prateekbh added 15 commits March 12, 2018 13:37

adding pingback

ade809b

adding test

fb798a9

adding viewer tracker

826fd65

adding tests for viewer tracker

cfbd34f

adding more tests

6220ec1

fixing tests

840b254

fixing tests

1944434

Merge branch 'pingback' of https://github.com/prateekbh/amphtml into …

bf1c1a3

…pingback

fixing tests

1e7b55d

adding undefined for entitlements

37bee9d

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

76bb809

…ullEntitlements

fixing all subscriptions denied use case

720b380

Building platformStore

5d7dc40

adding parameters over platform selection

80fa1e4

adding tests for select platform

5f23c82

prateekbh requested a review from dvoytenko March 14, 2018 22:37

googlebot added the cla: yes label Mar 14, 2018

dvoytenko suggested changes Mar 14, 2018

View reviewed changes

prateekbh added 3 commits March 14, 2018 22:18

fixing comments

fdfcc74

type fixes

13522df

adding assert in select platform

e160984

dvoytenko approved these changes Mar 15, 2018

View reviewed changes

prateekbh added 3 commits March 15, 2018 12:08

fixing scope

78d9251

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

9222ef9

…etterPlatformSelection

fixing types

ef77191

prateekbh merged commit e738264 into ampproject:master Mar 15, 2018

prateekbh deleted the betterPlatformSelection branch March 15, 2018 20:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subscriptions: Better platform selection #14004

Subscriptions: Better platform selection #14004

prateekbh commented Mar 14, 2018

dvoytenko Mar 14, 2018

prateekbh Mar 14, 2018

dvoytenko Mar 14, 2018

prateekbh Mar 15, 2018

dvoytenko Mar 14, 2018

prateekbh Mar 15, 2018

dvoytenko Mar 14, 2018

prateekbh Mar 15, 2018

dvoytenko Mar 14, 2018

prateekbh Mar 15, 2018

dvoytenko Mar 14, 2018

prateekbh Mar 15, 2018

dvoytenko Mar 14, 2018

prateekbh Mar 15, 2018

dvoytenko Mar 14, 2018

prateekbh Mar 15, 2018

dvoytenko Mar 14, 2018

prateekbh Mar 15, 2018

dvoytenko Mar 14, 2018

prateekbh Mar 15, 2018

dvoytenko left a comment

dvoytenko Mar 15, 2018

dvoytenko Mar 15, 2018

Subscriptions: Better platform selection #14004

Subscriptions: Better platform selection #14004

Conversation

prateekbh commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment