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.

Sign up for GitHub

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

Jump to bottom

[WD-10833] - Indicate logged-in-user-email #779

Merged
merged 1 commit into from
Jun 4, 2024
Merged

[WD-10833] - Indicate logged-in-user-email #779

merged 1 commit into from
Jun 4, 2024

Conversation

Kxiru
Copy link
Contributor

@Kxiru Kxiru commented May 23, 2024 •
edited
Loading

Done

  • Created a custom react hook, useLoggedInUser, which returns the logged in username.
  • Created a custom react hook, useIdentities to return details about the identities associated with LXD. Also used React QueryKeys and useQuery.
  • Created a new tab in the navigation bar that conditionally renders text based on the connection type.

Pending Design Draft

QA

  1. Run the LXD-UI:
    • On the demo server via the link posted by @webteam-app below. This is only available for PRs created by collaborators of the repo. Ask @mas-who or @edlerd for access.
    • With a local copy of this branch, run as described here.
  2. Perform the following QA steps:
    • Upon running LXD through a TLS connection, you should see "lxd-ui.crt" next to a lock icon, in the navigation bar.
    • Upon running LXD through an OIDC connection, you should see your username in the navigation bar. If you do not have an associated username, you should see your email address, which will likely be truncated depending on length.

Screenshots (updated)

  • Authenticated using a Certificate:
    image

  • Authenticated using OIDC:
    image

  • Authenticated using OIDC- Inspect element used to replace username with email address (For users with no username) and show truncation:
    image

@Kxiru Kxiru requested review from edlerd and mas-who May 23, 2024 15:01
@webteam-app
Copy link

Demo

Jenkins

demos.haus

@Kxiru Kxiru marked this pull request as draft May 23, 2024 15:02
@Kxiru Kxiru force-pushed the indicate-logged-in-user-email branch 2 times, most recently from 1dc4bd0 to a3e3234 Compare May 23, 2024 15:33
edlerd
edlerd reviewed May 23, 2024
View reviewed changes
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

Nice, this works :)
Some simplification ideas below.

src/context/useLoggedInUser.tsx Outdated Show resolved Hide resolved
src/context/useLoggedInUser.tsx Outdated Show resolved Hide resolved
@Kxiru Kxiru force-pushed the indicate-logged-in-user-email branch 4 times, most recently from d6daa7d to 8c20079 Compare May 24, 2024 14:39
@Kxiru Kxiru force-pushed the indicate-logged-in-user-email branch 2 times, most recently from 92bbb0e to 5540d43 Compare May 29, 2024 11:59
edlerd
edlerd requested changes May 29, 2024
View reviewed changes
Copy link
Collaborator

@edlerd edlerd left a comment •
edited
Loading

Choose a reason for hiding this comment

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

1

The new navigation item should be hidden on the login screens. Like in the screen below, there is the icon for the username partially visible, where it shouldn't be.
Screenshot from 2024-05-29 14-10-00

2

Design question also for @piperdeck : Shall we add truncation for the overflow case and a bit of right margin?
image

3

In the design, the "Log out" button was moved to the end of the navigation to be placed directly below "Report a bug".

src/components/Navigation.tsx Outdated Show resolved Hide resolved
src/components/Navigation.tsx Outdated Show resolved Hide resolved
@Kxiru Kxiru force-pushed the indicate-logged-in-user-email branch 6 times, most recently from fdff7e2 to 22f66b1 Compare May 29, 2024 13:44
edlerd
edlerd approved these changes May 29, 2024
View reviewed changes
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

Code and QA looks good to me.

@Kxiru Kxiru marked this pull request as ready for review May 29, 2024 13:57
@Kxiru Kxiru force-pushed the indicate-logged-in-user-email branch from 22f66b1 to 502ec1d Compare May 30, 2024 11:03
@Kxiru
Indicate logged-in-user-email in Navigation bar.
3dd7873 
- Conditional rendering of username in Navigation.tsx.
- New custom react hook, useLoggedInUser.
- Passed through additional properties such as loggedInUserName and loggedInUserID

Signed-off-by: Nkeiruka <[email protected]>
@Kxiru Kxiru force-pushed the indicate-logged-in-user-email branch from 502ec1d to 3dd7873 Compare June 3, 2024 09:11
@piperdeck
Copy link

Hi @Kxiru, could you please post screenshots of the QA steps? I wasn't able to stand up a testing lxd instance in order to test the OIDC behaviour, but I'd really like to review this for you.

@Kxiru
Copy link
Contributor Author

Kxiru commented Jun 4, 2024

Hi @Kxiru, could you please post screenshots of the QA steps? I wasn't able to stand up a testing lxd instance in order to test the OIDC behaviour, but I'd really like to review this for you.

I've added updated screenshots to the PR description for you.

@piperdeck
Copy link

LGTM

@Kxiru Kxiru merged commit 5198029 into canonical:main Jun 4, 2024
12 checks passed
github-actions bot pushed a commit that referenced this pull request Jun 4, 2024
workflow: remove all reports for PR #779
4429e3c
@Kxiru Kxiru deleted the indicate-logged-in-user-email branch June 17, 2024 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edlerd edlerd edlerd approved these changes

@mas-who mas-who mas-who left review comments

Assignees

@piperdeck piperdeck

@Kxiru Kxiru

Labels
Review: Code +1 Review: Design +1 Review: QA +1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Kxiru @webteam-app @piperdeck @edlerd @mas-who