Details
I tested and found that this fix the new account registration flow on master when using a multi-keyserver workflow. Prior to this, I was experiencing the issue details in ENG-7366
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
In the issue you said:
However, reduceServicesAccessToken does not have code to set the CSAT on identityRegisterActionTypes.success. The end result is that the CSAT is cleared on this action, after initially being set by AccessTokenHandler via setAccessTokenActionType (triggered by getCommServicesAuthMetadataEmitter that comes before identityRegisterActionTypes.success).
This is going to be the case for login as well once ENG-6598 is implemented. This makes me wonder if AccessTokenHandler is needed at all. If we will be setting CSAT on login and register actions, is there a need for it? I guess the CSAT can change while being logged in, but then how do we handle that on web, and why is native different?
On web we handle register and login in a special reducer - D11238. This was done following the discussion in D11022. This diff should then remove handling identityRegisterActionTypes.success from reduceServicesAccessToken in web/redux/services-access-token-reducer.js
But I still don't know how we handle / plan to handle CSAT changes while the user is logged in (which I think are possible if identity invalidates the session? cc. @varun )
This is going to be the case for login as well once ENG-6598 is implemented. This makes me wonder if AccessTokenHandler is needed at all. If we will be setting CSAT on login and register actions, is there a need for it? I guess the CSAT can change while being logged in, but then how do we handle that on web, and why is native different?
The point of AccessTokenHandler is to avoid inconsistency between native and JS layers. When we introduced it, I think our thinking was that on native, changes to the CSAT would all be initiated from the native layer, and then be propagated to the JS layer.
I am skeptical about getting rid of it, because it seems likely that some inconsistency would end up being introduced.
I think we can keep the principle that the CSAT is owned by native. But in that case, we should not be resetting the CSAT in Redux directly via resetUserSpecificState. Instead we should reset it in native first (eg. via clearSensitiveData), and then we would expect that the change will trickle into native.
I'm going to land this for now, but @inka can you create a task to discuss how we want to handle this long-term? Make sure to subscribe myself, @marcin, and @tomek.