web/user: refactor LibraryPage for testing, add CTA by kensternberg-authentik · Pull Request #5665 · goauthentik/authentik

kensternberg-authentik · 2023-05-17T17:47:41Z

Details

Resolves UX: add an actionable button to "No Applications" page #5171

Changes

web: revise LibraryPage, add CTA

Summary

TL;DR:

Separated LibraryPage into a bunch of different, independent parts, none of which require Authentik running to be testable or viewable.
This made adding the “Add an Application” CTA easier.
This sets the stage for unit and view testing of the UI

Description

Refactored

This commit revises the LibraryPage, devolving it into a couple of independent components that have to asynchronous dependencies, with a single asynchronous master:

LibraryPage: Loads the UIConfig, UserConfig, and CoreApi, and once those are loaded, launches the LibraryPageImpl.
LibraryPageImpl: the ListView of applications available, and updates the ListView according to search criteria it receives via an event listener.
- LibraryPageImpl.css: The stylesheet. Put here because it’s visual clutter.
- LibraryPageImpl.utils: defines static functions used to filter the view. Here because, again, it would otherwise be visual clutter of the LibraryPageImpl.
ApplicationEmptyState: Shows the “You have no applications” and, if the user is a superuser, the “Add an application” button.
ApplicationSearch: Contains the Fuse implementation and, as the search result is updated, sends the selected and filtered app list to the LibraryPage via an event. Also controls the “Choose an application by pressing Enter” event.
ApplicationList: Displays the list of applications.

All of these components are responsive to changes in the Apps collection via the LibraryPage itself, but none of them invoke the Apps collection, UIConfig, and CoreApi directly, so it should be possible to create Storybook implementations that view the LibraryPageImpl itself without having to have an instance of Authentik running.

Extension and Bugfix

If the user is a superuser, the “You have no applications” panel now shows the “Add an Application” button and a link to the documentation on how to add an application.

New Features

Adds a "Add an Application" to the LibraryView if there are no applications and the user is an administrator.

Breaking Changes

No breaking changes.

Checklist

Local tests pass (ak test authentik/)
The code has been formatted (make lint-fix)

If an API change has been made

No API Changes were made.

If changes to the frontend have been made

The code has been formatted (make web)
The translation files have been updated (make i18n-extract)

No documentation changes were made.

github-actions · 2023-05-17T17:48:01Z

authentik translations instructions

Thanks for your pull request!

authentik translations are handled using Transifex. Please edit translations over there and they'll be included automatically.

codecov · 2023-05-17T17:56:08Z

Codecov Report

Patch coverage has no change and project coverage change: +0.13 🎉

Comparison is base (0d0bb1a) 92.49% compared to head (d0415c1) 92.62%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5665      +/-   ##
==========================================
+ Coverage   92.49%   92.62%   +0.13%     
==========================================
  Files         547      547              
  Lines       26227    26227              
==========================================
+ Hits        24256    24289      +33     
+ Misses       1971     1938      -33

Flag	Coverage Δ
e2e	`51.90% <ø> (+0.22%)`	⬆️
integration	`26.39% <ø> (ø)`
unit	`89.49% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

see 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

github-actions · 2023-05-17T18:12:30Z

authentik PR Installation instructions

Instructions for docker-compose

Add the following block to your .env file:

AUTHENTIK_IMAGE=ghcr.io/goauthentik/dev-server
AUTHENTIK_TAG=gh-BUG-5171-revised-package-json-1684790489-d0415c1
AUTHENTIK_OUTPOSTS__CONTAINER_IMAGE_BASE=ghcr.io/goauthentik/dev-%(type)s:gh-%(build_hash)s

For arm64, use these values:

AUTHENTIK_IMAGE=ghcr.io/goauthentik/dev-server
AUTHENTIK_TAG=gh-BUG-5171-revised-package-json-1684790489-d0415c1-arm64
AUTHENTIK_OUTPOSTS__CONTAINER_IMAGE_BASE=ghcr.io/goauthentik/dev-%(type)s:gh-%(build_hash)s

Afterwards, run the upgrade commands from the latest release notes.

Instructions for Kubernetes

Add the following block to your values.yml file:

authentik:
    outposts:
        container_image_base: ghcr.io/goauthentik/dev-%(type)s:gh-%(build_hash)s
image:
    repository: ghcr.io/goauthentik/dev-server
    tag: gh-BUG-5171-revised-package-json-1684790489-d0415c1

For arm64, use these values:

authentik:
    outposts:
        container_image_base: ghcr.io/goauthentik/dev-%(type)s:gh-%(build_hash)s
image:
    repository: ghcr.io/goauthentik/dev-server
    tag: gh-BUG-5171-revised-package-json-1684790489-d0415c1-arm64

Afterwards, run the upgrade commands from the latest release notes.

BeryJu

A bunch of nitpicks and a bunch of opinions/questions, already looking very good and should be a lot easier to test

web/src/user/LibraryPage/ApplicationEmptyState.ts

BeryJu · 2023-05-17T18:51:31Z

web/src/user/LibraryPage/ApplicationEmptyState.ts

+        `,
+    ];
+
+    @state() isSuperUser = false;


I'm personally more of a fan of having it like

@state() foo = "bar";

but that's also very much a nitpick

But a good one. Consistency is important.

BeryJu · 2023-05-17T18:53:42Z

web/src/user/LibraryPage/constants.ts

@@ -0,0 +1,2 @@
+export const SEARCH_UPDATED = "authentik.search-updated";


should maybe be in web/src/common/constants.ts? especially if we want to re-use the same event for different places

It's very local to this, and the parent object that listens for it does a stopPropagation, so the name could be re-used in other places without confusion. I'll think on it much more, but my main goal is not polluting a global namespace if I don't have to.

BeryJu · 2023-05-17T18:56:01Z

web/src/user/LibraryPage/types.ts

+export type AppGroupEntry = [string, Application[]];
+export type AppGroupList = AppGroupEntry[];
+
+export type PageUIConfig = {


do we want to have another type for this? Any reason that UIConfig in web/src/common/ui/config.ts doesn't work?

It's not that it doesn't work, it's that it's too much. There are only three settings we care about and I want to document those. I'm caught between the "Don't pass to child components info they don't need" vs "Don't calculate what is already stored," but I think the duplication here reduces confusion about what styles this component uses. Down in the components that use these I would have to invoke the Demeter-defying strings of dereferences anyway; putting them here centralizes that effort, makes them easy to find, and gives them names.

Yeah thats a fair point, allthough maybe it would be better to go the other way around and compose UIConfig of smaller interfaces, and we don't have to add things in two places, but that can be a later PR if we want to do that

web/src/user/LibraryPage/LibraryPageImpl.ts

netlify · 2023-05-17T21:36:12Z

✅ Deploy Preview for authentik ready!

Name	Link
🔨 Latest commit	`83807f2`
🔍 Latest deploy log	https://app.netlify.com/sites/authentik/deploys/646bc879501cd00008981a41
😎 Deploy Preview	https://deploy-preview-5665--authentik.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

I'm pretty impressed that that worked. Good on Jens for having that in the prettier rules.

Removed the migration and web/README.md file. The former should not have been included; the latter is currently unprofessional in tone.

TL;DR: - Separated LibraryPage into a bunch of different, independent parts, none of which require Authentik running to be testable or viewable. - This made adding the "Add an Application" CTA easier. - This sets the stage for unit and view testing of the UI This commit revises the LibraryPage, devolving it into a couple of independent components that have to asynchronous dependencies, with a single asynchronous master: - LibraryPage: Loads the UIConfig, UserConfig, and CoreApi, and once those are loaded, launches the LibraryPageImpl. - LibraryPageImpl: the ListView of applications available, and updates the ListView according to search criteria it receives via an event listener. - LibraryPageImpl.css: The stylesheet. Put here because it's visual clutter. - LibraryPageImpl.utils: defines static functions used to filter the view. Here because, again, it would otherwise be visual clutter of the LibraryPageImpl. - ApplicationEmptyState: Shows the "You have no applications" and, if the user is a superuser, the "Add an application" button. - ApplicationSearch: Contains the Fuse implementation and, as the search result is updated, sends the selected and filtered app list to the LibraryPage via an event. Also controls the "Choose an application by pressing Enter" event. - ApplicationList: Displays the list of applications. All of these components are _responsive_ to changes in the Apps collection via the LibraryPage itself, but none of them invoke the Apps collection, UIConfig, and CoreApi directly, so it should be possible to create Storybook implementations that view the LibraryPageImpl itself without having to have an instance of Authentik running. If the user is a superuser, the "You have no applications" panel now shows the "Add an Application" button and a link to the documentation on how to add an application.

\#\# Details - Resolves #5171 \#\# Changes This just updates the prettier and eslint passes.

- Resolves #5171 \#\# Changes Removed unused declarations.

- web: refactor LibraryPage, resolves #5171 \#\# Changes Some changes found in code review, including an embarassing failure to both remove the old internal accessor and propagate the new one for "isAdmin". A pattern is emerging that a LitComponent class should consist of: - styles - properties - states - queries - other object fields - constructor() - connectedCallBack() - disconnectedCallBack() - event listeners - callback helpers - render helpers - render() ... in that order.

Signed-off-by: Jens Langhammer <jens@goauthentik.io>

kensternberg-authentik · 2023-05-17T17:49:59Z

web/src/user/LibraryPage/ApplicationEmptyState.ts

+        PFEmptyState,
+        PFContent,
+        PFSpacing,
+        css`


This is one of the few things that really bothers me about this change. I could not figure out how to make the "button" (it's really an anchor) look like the mock-up without hacking my own CSS, and I don't think I should have had to do that.

Also, this is a bit inconsistent with my usual "Define the styles outside of the class because they're static" principle. I may revisit this in the future, if the opportunity presents itself.

web/src/user/LibraryPage/ApplicationEmptyState.ts

kensternberg-authentik · 2023-05-17T20:43:31Z

web/src/user/LibraryPage/ApplicationEmptyState.ts

+        `,
+    ];
+
+    @state() isSuperUser = false;


But a good one. Consistency is important.

web/src/user/LibraryPage/ApplicationEmptyState.ts

kensternberg-authentik · 2023-05-17T20:44:43Z

web/src/user/LibraryPage/constants.ts

@@ -0,0 +1,2 @@
+export const SEARCH_UPDATED = "authentik.search-updated";


It's very local to this, and the parent object that listens for it does a stopPropagation, so the name could be re-used in other places without confusion. I'll think on it much more, but my main goal is not polluting a global namespace if I don't have to.

kensternberg-authentik · 2023-05-17T20:47:27Z

web/src/user/LibraryPage/types.ts

+export type AppGroupEntry = [string, Application[]];
+export type AppGroupList = AppGroupEntry[];
+
+export type PageUIConfig = {


It's not that it doesn't work, it's that it's too much. There are only three settings we care about and I want to document those. I'm caught between the "Don't pass to child components info they don't need" vs "Don't calculate what is already stored," but I think the duplication here reduces confusion about what styles this component uses. Down in the components that use these I would have to invoke the Demeter-defying strings of dereferences anyway; putting them here centralizes that effort, makes them easy to find, and gives them names.

web/src/user/LibraryPage/LibraryPageImpl.ts

tanberry · 2023-05-22T20:35:10Z

Thanks @kensternberg-authentik for adding CTA here! One request: can you change the wording about the Docs to "Refer to documentation"?

@tana

As requested by @tana.

@tana

* 5171: Fixed README to comply with Prettier rules. I'm pretty impressed that that worked. Good on Jens for having that in the prettier rules. * web: revised package.json Removed the migration and web/README.md file. The former should not have been included; the latter is currently unprofessional in tone. * web: revise LibraryPage, add CTA TL;DR: - Separated LibraryPage into a bunch of different, independent parts, none of which require Authentik running to be testable or viewable. - This made adding the "Add an Application" CTA easier. - This sets the stage for unit and view testing of the UI This commit revises the LibraryPage, devolving it into a couple of independent components that have to asynchronous dependencies, with a single asynchronous master: - LibraryPage: Loads the UIConfig, UserConfig, and CoreApi, and once those are loaded, launches the LibraryPageImpl. - LibraryPageImpl: the ListView of applications available, and updates the ListView according to search criteria it receives via an event listener. - LibraryPageImpl.css: The stylesheet. Put here because it's visual clutter. - LibraryPageImpl.utils: defines static functions used to filter the view. Here because, again, it would otherwise be visual clutter of the LibraryPageImpl. - ApplicationEmptyState: Shows the "You have no applications" and, if the user is a superuser, the "Add an application" button. - ApplicationSearch: Contains the Fuse implementation and, as the search result is updated, sends the selected and filtered app list to the LibraryPage via an event. Also controls the "Choose an application by pressing Enter" event. - ApplicationList: Displays the list of applications. All of these components are _responsive_ to changes in the Apps collection via the LibraryPage itself, but none of them invoke the Apps collection, UIConfig, and CoreApi directly, so it should be possible to create Storybook implementations that view the LibraryPageImpl itself without having to have an instance of Authentik running. If the user is a superuser, the "You have no applications" panel now shows the "Add an Application" button and a link to the documentation on how to add an application. * web: lint and prettier updates \#\# Details - Resolves goauthentik#5171 \#\# Changes This just updates the prettier and eslint passes. * \#\# Details - Resolves goauthentik#5171 \#\# Changes Removed unused declarations. * \#\# Details - web: refactor LibraryPage, resolves goauthentik#5171 \#\# Changes Some changes found in code review, including an embarassing failure to both remove the old internal accessor and propagate the new one for "isAdmin". A pattern is emerging that a LitComponent class should consist of: - styles - properties - states - queries - other object fields - constructor() - connectedCallBack() - disconnectedCallBack() - event listeners - callback helpers - render helpers - render() ... in that order. * actually remove LibraryPage that got re-added in the rebase Signed-off-by: Jens Langhammer <jens@goauthentik.io> * fix router import Signed-off-by: Jens Langhammer <jens@goauthentik.io> * use pf-c-button for CTA Signed-off-by: Jens Langhammer <jens@goauthentik.io> * fix different alignment compared to old version Signed-off-by: Jens Langhammer <jens@goauthentik.io> * use docLink() for documentation link Signed-off-by: Jens Langhammer <jens@goauthentik.io> * also open docs in new tab Signed-off-by: Jens Langhammer <jens@goauthentik.io> * web: minor language changes As requested by @tana. --------- Signed-off-by: Jens Langhammer <jens@goauthentik.io> Co-authored-by: Jens Langhammer <jens@goauthentik.io>

kensternberg-authentik requested a review from a team as a code owner May 17, 2023 17:47

BeryJu reviewed May 17, 2023

View reviewed changes

kensternberg-authentik added 6 commits May 22, 2023 20:26

5171: Fixed README to comply with Prettier rules.

b766419

I'm pretty impressed that that worked. Good on Jens for having that in the prettier rules.

web: revised package.json

f9a1c1b

Removed the migration and web/README.md file. The former should not have been included; the latter is currently unprofessional in tone.

web: lint and prettier updates

da0f396

\#\# Details - Resolves #5171 \#\# Changes This just updates the prettier and eslint passes.

\#\# Details

b62e0ee

- Resolves #5171 \#\# Changes Removed unused declarations.

BeryJu force-pushed the BUG-5171-revised-package-json branch from 854531e to 83807f2 Compare May 22, 2023 19:54

BeryJu added 6 commits May 22, 2023 22:08

actually remove LibraryPage that got re-added in the rebase

8b5691c

Signed-off-by: Jens Langhammer <jens@goauthentik.io>

fix router import

d6bc432

Signed-off-by: Jens Langhammer <jens@goauthentik.io>

use pf-c-button for CTA

64d495e

Signed-off-by: Jens Langhammer <jens@goauthentik.io>

fix different alignment compared to old version

57a3ad3

Signed-off-by: Jens Langhammer <jens@goauthentik.io>

use docLink() for documentation link

6bc4197

Signed-off-by: Jens Langhammer <jens@goauthentik.io>

also open docs in new tab

6a92c1e

Signed-off-by: Jens Langhammer <jens@goauthentik.io>

kensternberg-authentik commented May 22, 2023

View reviewed changes

web: minor language changes

d0415c1

As requested by @tana.

BeryJu approved these changes May 22, 2023

View reviewed changes

BeryJu changed the title ~~web: refactor LibraryPage for testing, add CTA, fix 5171~~ web/user: refactor LibraryPage for testing, add CTA, fix 5171 May 22, 2023

BeryJu changed the title ~~web/user: refactor LibraryPage for testing, add CTA, fix 5171~~ web/user: refactor LibraryPage for testing, add CTA May 22, 2023

BeryJu merged commit 7c7957f into main May 22, 2023

BeryJu deleted the BUG-5171-revised-package-json branch May 22, 2023 21:35

		@@ -0,0 +1,2 @@
		export const SEARCH_UPDATED = "authentik.search-updated";

Uh oh!

Conversation

kensternberg-authentik commented May 17, 2023

Details

Changes

Summary

Description

Refactored

Extension and Bugfix

New Features

Breaking Changes

Checklist

Uh oh!

github-actions bot commented May 17, 2023

authentik translations instructions

Uh oh!

codecov bot commented May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BeryJu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

netlify bot commented May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for authentik ready!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tanberry commented May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented May 17, 2023 •

edited

Loading

github-actions bot commented May 17, 2023 •

edited

Loading

BeryJu left a comment •

edited

Loading

netlify bot commented May 17, 2023 •

edited

Loading

tanberry commented May 22, 2023 •

edited

Loading