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

18023 incorrect name in breadcrumb #725

Conversation

jamespaologarcia
Copy link
Contributor

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).

src/App.vue Outdated Show resolved Hide resolved
src/App.vue Outdated Show resolved Hide resolved
@janisrogers
Copy link

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
Copy link
Collaborator

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
Copy link

janisrogers commented Oct 5, 2023

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
Copy link
Collaborator

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
Copy link

Yes, please proceed with that

@bcgov bcgov deleted a comment from bcregistry-sre Oct 5, 2023
@bcgov bcgov deleted a comment from JazzarKarim Oct 5, 2023
@severinbeauvais
Copy link
Collaborator

severinbeauvais commented Oct 5, 2023

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
Copy link
Collaborator

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

@bcgov bcgov deleted a comment from bcregistry-sre Oct 6, 2023
@severinbeauvais
Copy link
Collaborator

/gcbrun

Copy link
Collaborator

@severinbeauvais severinbeauvais left a comment

Choose a reason for hiding this comment

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

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

@severinbeauvais severinbeauvais merged commit 7126066 into bcgov:main Oct 6, 2023
5 checks passed
@bcregistry-sre
Copy link
Collaborator

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

@severinbeauvais
Copy link
Collaborator

severinbeauvais commented Oct 6, 2023

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.

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