PWA-875: Implement dynamic redirects #244
Conversation
fkloes
commented
Sep 5, 2018
fkloes
commented
- updated the router subscriptions to enable processing of dynamic redirects
- removed some outdated webckeckout related code
- fixed a product selector bug that caused endless loading state for descriptions
- extended tests
- it doesn't return NULL anymore, if the description is an empty string
- introduced the "historyReplace" action to handle redirect situations - replaced existing logic with the new action - when protector routes are about to be replaced by legacy or in-app-browser routes, they are now popped from the history stack
- added router subscriptions coverage for redirect handlers - added tests for the historyRedirect action
- added redirect data to the listener within the user subscriptions - removed logic from the web checkout since it duplicated the logic from the user subscriptions
- subscribers now receive the expected parameters
…o PWA-875-implement-dynamic-redirects
…o PWA-875-implement-dynamic-redirects # Conflicts: # themes/gmd
| */ | ||
| export function historyRedirect(params = {}) { | ||
| return (dispatch) => { | ||
| const { location, state = {} } = params; |
There was a problem hiding this comment.
Please don't make the state an empty object. The history... actions will pass it through. This means if state is not set, it will be treated as undefined. Thas the way to go.
| return (dispatch) => { | ||
| const { location, state = {} } = params; | ||
|
|
||
| if (location) { |
There was a problem hiding this comment.
Please rewrite this to be a guard clause:
if (!location) {
dispatch(historyPop());
return;
}
...
This is more readable.
| @@ -3,6 +3,8 @@ export * from './historyPop'; | |||
| export * from './historyReplace'; | |||
| export * from './historyReset'; | |||
|
|
|||
There was a problem hiding this comment.
Please remove unnecessary empty lines.
| parsed.query.sgcloud_callback_data = JSON.stringify({ redirectTo }); | ||
|
|
||
| // Explicitly check if we are about to be redirected to the checkout. | ||
| if (redirect && redirect.startsWith(CHECKOUT_PATH)) { |
There was a problem hiding this comment.
The first check doesn't make sense here. If the redirect param is maybe not set, it falls back to an empty string. Please don't give it a default, if you check it either way. Or don't check it again. One of both is redundant.
There was a problem hiding this comment.
i removed the check but kept the default value.
| @@ -0,0 +1,32 @@ | |||
| import queryString from 'query-string'; | |||
| import { | |||
| CHECKOUT_PATH, | |||
There was a problem hiding this comment.
Please make this single lined. Its only two variables. Makes it a bit cleaner.
|
|
||
| if (redirect) { | ||
| // Check if a redirect handler was assigned and execute it. |
There was a problem hiding this comment.
This comment is not needed. It is obvious what's happening.
| dispatch(unsetViewLoading(pathname)); | ||
|
|
||
| if (!redirect) { | ||
| // Stop processing when no redirect was created. |
There was a problem hiding this comment.
Also obvious. The code speaks for itself.
| testExpectedCall(conductor.push, { | ||
| ...params, | ||
| pathname: params.pathname.slice(0, -1), | ||
| }); | ||
| }); | ||
|
|
||
| it('should redirect to a protector route when the user is not logged in', () => { | ||
| it('should redirect to a protector route when the user is not logged in', async () => { |
There was a problem hiding this comment.
protector is correct since the pushed route is protected. so the navigate$ subscription redirects the user the the protector route e.g. login.
| @@ -198,7 +215,7 @@ describe('Router subscriptions', () => { | |||
| })); | |||
| }); | |||
|
|
|||
| it('should not redirect to a protector route when the user is logged in', () => { | |||
| it('should not redirect to a protector route when the user is logged in', async () => { | |||
There was a problem hiding this comment.
same like above. it's about a redirect to the route which protects a protected route. this is called protector.
| /** | ||
| * @return {Promise} | ||
| */ | ||
| const redirectHandler = () => Promise.reject(error); |
There was a problem hiding this comment.
No need to store this into a variable. Please put that inline. (reduces the amount of code)
There was a problem hiding this comment.
i did it because it makes the test more readable from my point of view. a function which returns a rejected promise as the 2nd argument of the set method would be hard to understand if someone would have to understand or extend this test.
so i'd be happy to keep it like it is.
