search

bcgov / namerequest

Public Front End for the Name Request System
1 stars 42 forks source link

18023 incorrect name in breadcrumb #725

Closed jamespaologarcia closed 11 months ago

jamespaologarcia commented 11 months ago

Issue #: /bcgov/entity#18023

Description of changes: Created a separate breadcrumb for logged in users

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the namerequest license (Apache 2.0).

janisrogers commented 11 months ago

The changes requested in the ticket look great. I did however notice that the non-logged in user is taken to the login page, instead of the BCROS home page (https://dev.bcregistry.gov.bc.ca/) Can that be updated?

severinbeauvais commented 11 months ago

The changes requested in the ticket look great. I did however notice that the non-logged in user is taken to the login page, instead of the BCROS home page (https://dev.bcregistry.gov.bc.ca/) Can that be updated?

@janisrogers Is it correct to tell the user in the breadcrumb that they will go to the login page? After login, they go to the BCROS dashboard (same as logged in).

janisrogers commented 11 months ago

There is a non logged in version of the BCROS ~dashboard~ Main page. This is where the breadcrumb should take them. I don't think we should direct the user to the login page if they haven't chosen to login.

severinbeauvais commented 11 months ago

There is a non logged in version of the BCROS ~dashboard~ Main page. This is where the breadcrumb should take them. I don't think we should direct the user to the login page if they haven't chosen to login.

That's a good point / idea. Janis, let us know whether to proceed with that.

James, when Janis has approved this requirement, the non-logged-in breadcrumb should go to registryHomeUrl and its text should be "BC Registries and Online Services". The logged-in breadcrumb should go to ${registryHomeUrl}dashboard/${getParams()} and its text should be "BC Registries Dashboard".

janisrogers commented 11 months ago

Yes, please proceed with that

severinbeauvais commented 11 months ago

James, there might be a problem with how the code determines whether or not the user is authenticated.

I tested this change in the preview build above (temp URL), and it looks fine for logged out, but after logging in, the breadcrumb is incorrect until I refresh the page again. Try it yourself. (I have an idea how to fix it.)

severinbeauvais commented 11 months ago

I have an idea how to fix it.

I think the error is that isAuthenticated checks for the presence of the keycloak token before it's populated. Because it's a getter, its value is cached until something it depends on changes (ie, responsive), but it doesn't recognize changes to the session storage so it keeps its original False value.

Ref: https://stackoverflow.com/questions/69789213/any-way-to-make-sessionstorage-reactive-in-vue3

severinbeauvais commented 11 months ago

/gcbrun

bcregistry-sre commented 11 months ago

Temporary Url for review: https://namerequest-dev--pr-725-eeac624z.web.app

severinbeauvais commented 11 months ago

This is good enough for now but I might pick away at the code a bit tomorrow to clean something up.

There was actually a bug in this PR (wrong getter), but I want to keep THIS change separate from the login detection change (*), so I fixed the bug by using isAuthenticated (in another commit) and created a new ticket for the login detection.

(*) I want us to fix the login detection properly, which is out of scope for THIS ticket.