New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Subscriptions: Intercept login action #15362
Changes from 10 commits
02de65f
7bee025
6e87079
0cdc0d8
6706c77
42b4965
e4f0bd0
b0184d6
a8ffef8
551703b
88cb7d0
01abc05
51dc07d
d4f16a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -153,8 +153,7 @@ export class SubscriptionService { | |
* service. | ||
* | ||
* @param {string} serviceId | ||
* @param {function(!JsonObject, !ServiceAdapter):!SubscriptionPlatform} | ||
* subscriptionPlatformFactory | ||
* @param {function(!JsonObject, !ServiceAdapter):!SubscriptionPlatform} subscriptionPlatformFactory | ||
*/ | ||
registerPlatform(serviceId, subscriptionPlatformFactory) { | ||
return this.initialize_().then(() => { | ||
|
@@ -271,7 +270,7 @@ export class SubscriptionService { | |
this.initializeLocalPlatforms_(service); | ||
}); | ||
|
||
this.platformStore_.getAllRegisteredPlatforms().forEach( | ||
this.platformStore_.getAvailablePlatforms().forEach( | ||
subscriptionPlatform => { | ||
this.fetchEntitlements_(subscriptionPlatform); | ||
} | ||
|
@@ -457,6 +456,31 @@ export class SubscriptionService { | |
platform.decorateUI(element, action, options); | ||
}); | ||
} | ||
|
||
/** | ||
* Evaluates platforms and select the one to be selected for login. | ||
* @return {!Promise<!./subscription-platform.SubscriptionPlatform>} | ||
*/ | ||
scoreBasedLogin() { | ||
return this.platformStore_.getAllPlatforms().then(platforms => { | ||
const platformScores = []; | ||
platforms.forEach(platform => { | ||
let score = 0; | ||
if (platform.supportsCurrentViewer()) { | ||
score += 1000; | ||
} | ||
platformScores.push({ | ||
platform, | ||
score, | ||
}); | ||
}); | ||
|
||
platformScores.sort(function(platform1, platform2) { | ||
return platform2.weight - platform1.weight; | ||
}); | ||
return platformScores[0].platform; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If none of the platforms matches viewer - would this guarantee that local wins then? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have a reason to do this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. With all equal "local" should win. |
||
}); | ||
} | ||
} | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -151,11 +151,18 @@ export class LocalSubscriptionPlatform { | |
handleClick_(element) { | ||
if (element) { | ||
const action = element.getAttribute('subscriptions-action'); | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be just easier to do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|| element.getAttribute('subscriptions-service') === 'auto' | ||
|| element.getAttribute('subscriptions-service') === '') { | ||
this.serviceAdapter_.login().then(platform => { | ||
const serviceId = platform.getServiceId(); | ||
this.serviceAdapter_.delegateActionToService(action, serviceId); | ||
}); | ||
} else if (element.getAttribute('subscriptions-service')) { | ||
const serviceId = element.getAttribute('subscriptions-service'); | ||
this.serviceAdapter_.delegateActionToService(action, serviceId); | ||
} else { | ||
this.executeAction(action); | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,6 +79,15 @@ export class ServiceAdapter { | |
getDialog() { | ||
return this.subscriptionService_.getDialog(); | ||
} | ||
|
||
/** | ||
* Returns login platform based on platform selection | ||
* | ||
* @return {!Promise} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed, was an intermittent thing. |
||
*/ | ||
login() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's rename to represent a bit more precisely what it does. E.g. "selectPlatformForLogin" or some such. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
return this.subscriptionService_.scoreBasedLogin(); | ||
} | ||
} | ||
|
||
/** @package @VisibleForTesting */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -136,6 +136,7 @@ describes.fakeWin('LocalSubscriptionsPlatform', {amp: true}, env => { | |
beforeEach(() => { | ||
element = document.createElement('div'); | ||
element.setAttribute('subscriptions-action', 'subscribe'); | ||
element.setAttribute('subscriptions-service', 'local'); | ||
}); | ||
|
||
it('should call executeAction with subscriptions-action value', () => { | ||
|
@@ -159,6 +160,30 @@ describes.fakeWin('LocalSubscriptionsPlatform', {amp: true}, env => { | |
expect(executeStub).to.not.be.called; | ||
expect(delegateStub).to.be.called; | ||
}); | ||
|
||
it('should delegate service selection to scoreBasedLogin if no service ' | ||
+ 'name is specified', () => { | ||
element.removeAttribute('subscriptions-service'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's also test "auto" value. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
const platform = {}; | ||
const serviceId = 'serviceId'; | ||
platform.getServiceId = sandbox.stub().callsFake(() => serviceId); | ||
const loginStub = sandbox.stub( | ||
localSubscriptionPlatform.serviceAdapter_, | ||
'login' | ||
).callsFake(() => Promise.resolve(platform)); | ||
const delegateStub = sandbox.stub( | ||
localSubscriptionPlatform.serviceAdapter_, | ||
'delegateActionToService' | ||
); | ||
localSubscriptionPlatform.handleClick_(element); | ||
expect(loginStub).to.be.called; | ||
return loginStub().then(() => { | ||
expect(delegateStub).to.be.calledWith( | ||
element.getAttribute('subscriptions-action'), | ||
serviceId, | ||
); | ||
}); | ||
}); | ||
}); | ||
|
||
describe('executeAction', () => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done