Adding @PageAuthorize page state.

[BenDol]

This gives an opportunity for "gatekeeping" pages asynchronously.

@PageAuthorize
void onAuthorize(NavigationControl control) {
    myService.call(hasAccess -> {
        if(hasAccess) {
            // We have access, proceed with navigation.
            control.proceed();
        } else {
            // Interrupt navigation and redirect.
            control.interrupt();
            redirectPageTo.go();
        }
    }).hasAccess(modelId, userId);
}

[kie-ci]

Can one of the admins verify this PR? Comment with 'ok to test' to start the build.

[mbarkley]

Hi @BenDol,

I had been meaning to respond to your forum post, but clearly you were too quick for me ;)

First let me say that I like this feature. Your changes are easy to understand, and thank you for adding javadoc and a license header to the new file you created!

That being said, there are some things I think we should change before merging this. Here are some of my concerns:

1. Calling NavigationControl.interrupt() appears to leave navigation in a state were we are not on any page. Perhaps interrupt could take a class for a @Page or UniquePageRole to navigate to instead? Maybe calling interrupt without an argument navigates back to the previous page? One way or another, we should end up on some page after navigation has been interrupted.

2. It seems like we could integrate this behaviour into @PageShowing. In the same way that @PageHiding can optionally have a NavigationControl parameter, we could make the same enhancement to @PageShowing. This is preferable because it keeps the API symmetric and removes the need for an additional lifecycle method.

3. As far as I can tell, the NavigationControl.interrupt method will cause an NPE if invoked from a @PageHiding method. If someone calls interrupt from a @PageHiding method without an argument, it would make sense for nothing to happen. Certainly if we are exposing this API, it shouldn't blow up.

4. There are no tests or docs for this feature. We'll need to add at least 3 tests to NavigationTest: one where interrupt is called, one where proceed is called, and one where interrupt is called from a @PageHiding method to verify that this doesn't cause errors. The project documentation can be found here, where you would need to update the "Page Lifecycle" section.

Cheers.

[BenDol]

Hi @mbarkley, thanks for the response. I will address the points made here:

1. That is probably a good idea to give interrupt some form of default behavior, but the idea is that interrupt is called when you take control of the navigation process, perhaps interrupt requires a Page transition class to be passed in as a parameter? That way they will always need to go to a specific page upon interrupting.

2. The reason I kept this functionality separate is because in my own project I have a lot of other functionality in @PageShowing. However perhaps doing it as you have suggested is a better approach.

3. Won't blow up, I think https://github.com/errai/errai/pull/215/files#diff-7a86ee145a10ddfa8fe57b68cdfd86c4R62

4. I will look into writing these tests once we have agree'd on the best approach.

Cheers.

[BenDol]

@mbarkley Thoughts?

[mbarkley]

Hi @BenDol,

Let me respond to your previous comment point by point.

1. Taking a page as a parameter for this method that you redirect to would be great. In fact, at that point I think it makes sense to rename interrupt to redirect. Then usage would look like this: control.redirect(HomePage.class).

2. I understand why you made this a separate life-cycle method, but I think the best choice for the framework is to add this to the @PageShowing so that we don't increase the size of the navigation life-cycle API and maintain symmetry between showing and hiding pages.

3. Yeah, looks like I was wrong about that one :) Of course, if we change interrupt to be redirect(Page.class) we'll have to make sure that calling redirecting also works from @PageHiding methods, but that should be easy enough.

If you agree with my suggestions, then I can review the PR again once you've had time to update it.

[BenDol]

Continued here #224