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

feat: Use RoutesBuilder API #29

Merged
merged 14 commits into from
Apr 16, 2024
Merged

feat: Use RoutesBuilder API #29

merged 14 commits into from
Apr 16, 2024

Conversation

mshabarov
Copy link
Contributor

No description provided.

@mshabarov mshabarov force-pushed the use-routes-builder branch from 44166a8 to 164f94f Compare March 28, 2024 04:58
@mshabarov mshabarov marked this pull request as ready for review April 11, 2024 07:31
Copy link

github-actions bot commented Apr 12, 2024

Unit Test Results

10 tests   10 ✔️  25s ⏱️
  2 suites    0 💤
  2 files      0

Results for commit f0ad36b.

♻️ This comment has been updated with latest results.

Copy link
Member

@tltv tltv left a comment

Choose a reason for hiding this comment

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

Suggesting import format to be same everywhere with a spaces around inside import { ... }.

src/main/frontend/routes.tsx Outdated Show resolved Hide resolved
src/main/frontend/routes.tsx Outdated Show resolved Hide resolved
src/main/frontend/views/about.tsx Outdated Show resolved Hide resolved
src/main/frontend/views/hilla.tsx Outdated Show resolved Hide resolved
src/main/frontend/views/hilla.tsx Outdated Show resolved Hide resolved
Copy link

@caalador caalador left a comment

Choose a reason for hiding this comment

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

Looks fine, but it works a bit strange.
It had all the routes for the different users, also the ones that are not applicable for the user.
So the Flow view is there for user, which leads to a Reason: Access is denied by annotations on the view..
And Hilla User is there for admin which leads to the login view, even though you are already logged in.

@mshabarov
Copy link
Contributor Author

It had all the routes for the different users, also the ones that are not applicable for the user.
So the Flow view is there for user, which leads to a Reason: Access is denied by annotations on the view..
And Hilla User is there for admin which leads to the login view, even though you are already logged in.

Yeah, I'll try to change that by editing the main layout impl.

@mshabarov
Copy link
Contributor Author

Unfortunately, I couldn't manage to make it work with the createMenuItems method, because:

  1. Any item returned by createMenuItems seems to not have rolesAllowed, nor loginRequired properties, I don't know how could I get the authorities with this API
  2. Authorities for server-side views aren't sent to the client and basically they aren't filtered by server (this feature haven't been implemented yet).

Thus, I'm not sure how can I filter the menu items by access (in role and is logged in).

I pushed an ugly checking codes that works, but we have to replace it with a proper way.

@mshabarov
Copy link
Contributor Author

UPD:
createMenuItems function now checks the access control automatically, but this feature hasn't been released in any alpha yet. Should be available in Hilla 24.4.0.alpha22, and then I can change my main layout impl to use:

<nav>
    {
         createMenuItems().map(({ to, icon, title }) => (
                <NavLink className={navLinkClasses} to={to} key={to}>
                      {title}
                </NavLink>
              ))
         },
         { flowIsInRole ? (
                <NavLink to={'/flow'} className={navLinkClasses}>
                      Flow Admin
                </NavLink>
         ) : null}
</nav>

Later on, when we implement this feature in Flow, we will be able to even remove that part

         { flowIsInRole ? (
                <NavLink to={'/flow'} className={navLinkClasses}>
                      Flow Admin
                </NavLink>
         ) : null}

as server-side routes will be checked agains access control and added into createMenuItems output list, if have access.

So we should wait for Vaadin 24.4.0.alpha22 release, remove my workaround for Hilla view, then wait for the above feature in Flow and just leave the following codes:

<nav>
    {
         createMenuItems().map(({ to, icon, title }) => (
                <NavLink className={navLinkClasses} to={to} key={to}>
                      {title}
                </NavLink>
              ))
         }
</nav>

FYI @caalador @tltv

@mshabarov mshabarov merged commit 5ac9643 into v24.4 Apr 16, 2024
3 checks passed
@mshabarov mshabarov deleted the use-routes-builder branch April 16, 2024 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants