Don't accidentally unpause the story when amp-access resolves. #17560
Conversation
| @@ -147,6 +147,11 @@ const actions = (state, action, data) => { | |||
| switch (action) { | |||
| // Triggers the amp-acess paywall. | |||
| case Action.TOGGLE_ACCESS: | |||
| // Don't change the PAUSED_STATE if ACCESS_STATE is not changed. | |||
There was a problem hiding this comment.
Is this the right fix? If I actually do sign in via another window and change my auth status, will this get triggered? Seems like it should be "don't unpause if we're still waiting for consent.". Should we have multiple states like "paused due to consent", "paused due to access", etc, then a single PAUSED_STATE that is true if any of the others are true?
There was a problem hiding this comment.
"Unfortunately" you're right... This fix addresses this very problem, but that's it :(
So far we're safe because we use the paused state for consent, share menu, paywall, bookend, and these can never happen at the same time.
Because the access authorizations could resolve at any time, it's the first time we could have a conflict, if it resolves why the consent|share menu|bookend is displayed.
We could do:
- What you suggest. It's a bit complex but it's very likely the best and safest solution
- Define a rule where we pay attention that actions are a noop if the main property stays unchanged. Ie:
TOGGLE_ACCESS, falseis noop ifACCESS_STATEwas alreadyfalse(this PR). As long as consent, share menu, ..., can't happen at the same time, we should be safe.
But we might have to do (1) eventually...
What do you think? Do you want to implement (1) right now?
There was a problem hiding this comment.
Per our conversation earlier, let's go ahead as-is until we prove out how difficult option 2 is.
| @@ -147,6 +147,11 @@ const actions = (state, action, data) => { | |||
| switch (action) { | |||
| // Triggers the amp-acess paywall. | |||
| case Action.TOGGLE_ACCESS: | |||
| // Don't change the PAUSED_STATE if ACCESS_STATE is not changed. | |||
There was a problem hiding this comment.
Per our conversation earlier, let's go ahead as-is until we prove out how difficult option 2 is.
It's hard to reproduce but the issue was:
amp-story-consentis displayedamp-accessresolves, and if the user is authorized, callsdispatch(Action.TOGGLE_ACCESS, false)PAUSED_STATEtofalseand accidentally unpause the story(See test case)