Subscriptions: Intercept login action #15362
Conversation
prateekbh
commented
May 16, 2018
prateekbh
commented
- intercepts login action and delegates based on platform selection.
prateekbh
changed the title
| platformScores.sort(function(platform1, platform2) { | ||
| return platform2.weight - platform1.weight; | ||
| }); | ||
| return platformScores[0].platform; |
There was a problem hiding this comment.
If none of the platforms matches viewer - would this guarantee that local wins then?
There was a problem hiding this comment.
Do we have a reason to do this?
There was a problem hiding this comment.
Yes. With all equal "local" should win.
| * @return {!Promise<!./subscription-platform.SubscriptionPlatform>} | ||
| */ | ||
| scoreBasedLogin() { | ||
| return this.platformStore_.getAllPlatforms().then(platforms => { |
There was a problem hiding this comment.
We are taking some risk here getting into popup blocker situation. What if we went synchronous here with strictly already loaded platforms? Local is always there, so we always have one option. In other words, maybe getAvailablePlatforms() method instead? Otherwise "login" is only enhanced - there's no significant change in behavior. If we fail to enhance something - it's still functional.
| if (element.hasAttribute('subscriptions-service')) { | ||
| if (element.getAttribute('subscriptions-service') === 'local') { | ||
| this.executeAction(action); | ||
| } else if (!element.hasAttribute('subscriptions-service') |
There was a problem hiding this comment.
Would it be just easier to do if ((element.getAttribute('subscriptions-service') || 'auto') == 'auto')?
| * | ||
| * @return {!Promise} | ||
| */ | ||
| login() { |
There was a problem hiding this comment.
Let's rename to represent a bit more precisely what it does. E.g. "selectPlatformForLogin" or some such.
| /** | ||
| * Returns login platform based on platform selection | ||
| * | ||
| * @return {!Promise} |
There was a problem hiding this comment.
fixed, was an intermittent thing.
|
|
||
| it('should delegate service selection to scoreBasedLogin if no service ' | ||
| + 'name is specified', () => { | ||
| element.removeAttribute('subscriptions-service'); |
There was a problem hiding this comment.
Let's also test "auto" value.
