Skip to content
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

Improve default child mechanism #610

Merged
merged 21 commits into from
Jul 4, 2019

Conversation

maxmarkus
Copy link
Contributor

You can now click on Browser back button to go to the last node, even if you were redirected by default child mechanism.

@maxmarkus maxmarkus added the enhancement New feature or request label Jun 25, 2019
@maxmarkus maxmarkus added this to the Sprint_1 milestone Jun 25, 2019
…lt-child-mechanism

# Conflicts:
#	core/test/services/routing.spec.js
@jesusreal jesusreal self-assigned this Jul 1, 2019
@maxmarkus maxmarkus removed this from the Sprint_1 milestone Jul 1, 2019
@JohannesDoberer JohannesDoberer self-assigned this Jul 1, 2019
Copy link
Contributor

@jesusreal jesusreal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done! I just left some small comments regarding the unit tests.

@@ -467,6 +471,7 @@ describe('Routing', () => {

assert.equal(singleStateWithPath.path, expectedRoute);
assert.equal(pushStateCallsNum + 1, expectedPushStateCallsNum);
assert.equal(replaceStateCallsNum, 0, 'replaceState not been called');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to use here notCalled as above and then we don't need the extra variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

core/test/services/routing.spec.js Outdated Show resolved Hide resolved
core/test/utilities/helpers/navigation-helpers.spec.js Outdated Show resolved Hide resolved
assert.isFalse(NavigationHelpers.isNodeAccessPermitted(checkNode));
});

it('logged out, exclusive anonymousAccess', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the mocks for this tests, as they are the same as in the test above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, we need to check if the exclusive anonymous access implementation is valid, since it also calls the permission checker function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please bring this topic to the next Scrum meeting ;)

@maxmarkus maxmarkus requested a review from jesusreal July 2, 2019 14:30
Copy link
Contributor

@jesusreal jesusreal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment

assert.isTrue(NavigationHelpers.isNodeAccessPermitted(checkNode));
});

it('logged out, exclusive anonymousAccess, with permissionCheckerFn', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is confusing IMHO since the mock of the function call returns false value, which is explicitly being returned above. It is also a little bit too much testing from my point of view. I would replace it with a test for the part where we are actually returning false, most precisely for the 2nd condition of the if block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense. done

@maxmarkus maxmarkus requested a review from jesusreal July 3, 2019 09:28
@maxmarkus maxmarkus merged commit 9e9e6e2 into SAP:master Jul 4, 2019
@maxmarkus maxmarkus deleted the 216-improve-default-child-mechanism branch July 4, 2019 12:28
stanleychh pushed a commit to stanleychh/luigi that referenced this pull request Dec 30, 2021
* improved default child mechanism and added e2e tets for it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants