-
-
Notifications
You must be signed in to change notification settings - Fork 874
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
web: clean up UserInterface in prep for OAuth and Silo Projects #8278
Conversation
… to the UserInterface triggered the "harder" eslint pass, which said "UserInterface exceeds permitted complexity (9)." I couldn't disagree; it had lots of conditionals. This commit: - Changes no functionality; it's just cleanup. - Breaks UserInterface into business and presentation layers - The presentation layer: - Further breaks the presentation layer into a frame and conditional components. Each conditional is now a simple guard condition. - Taps into the event listener set-up for toggles, eliminating their local scope/window duplication - Extracts in-line complex expressions into isolated and scope functions to name them and make them easier to find and read. - Extracts the custom CSS into its own named variable, again, making it easier to find and read. - The business layer: - Builds the window-level event listener at connection, and disconnects them correctly, allowing this whole interface to be used in a SPA. - Asserts a reliable contract at the presentation layer; there should be no question "Session" and "UIConfig" are available before rendering. - Renames `firstUpdated` to `fetchConfigurationDetails`, and calls it in the constructor. There ought to be no circumstances where this object is constructed outside a working environment; no sense in waiting until you've done a `render() { nothing }` pass to fetch details. Oddities: There are a pair of `<!-- -->` HTML comments in the framing `render()`; those are there just to stop prettier from slamming a string of conditional renders all into one line, making them harder to read.
✅ Deploy Preview for authentik-storybook ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the guided tour.
@property({ type: Object }) | ||
brand!: CurrentBrand; | ||
|
||
get userDisplayName() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns this into a switch expression, so Typescript can type it and assert conditional exhaustion correctly!
</div>` | ||
: html``} | ||
${this.renderApiDrawer()} | ||
<!-- --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are here just to space things out; otherwise, prettier
wants to string all these together into a single line, which was harder to read.
return nothing; | ||
} | ||
|
||
const onClick = (ev: Event) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current version has two handlers for EVENT_API_DRAWER_TOGGLE, one attached to UserInterface
, another attached to window
. No sense in keeping both; this eliminates one of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would probably rename this to renderApiDrawerTrigger
since it doesn't render the actual drawer
this.ws = new WebsocketClient(); | ||
this.fetchConfigurationDetails(); | ||
configureSentry(true); | ||
this.toggleNotificationDrawer = this.toggleNotificationDrawer.bind(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this binding is annoying, but necessary; it allows us to connect and disconnect these handlers from window
at need. Which would also allow us, if we wanted, to switch between "User" and "Admin" apps without actually leaving the page or reloading the environment, since we're now cleaning up after ourselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching between all these interfaces without a full reload would definitely be very nice to have
if (!this.isFullyConfigured) { | ||
return nothing; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this because it makes it very clear what the presentation responds to: here are the state variables that will trigger a re-render.
<button | ||
class="pf-c-button pf-m-plain" | ||
type="button" | ||
@click=${() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This duplicates the functionality of the window.EVENT_API_DRAWER_TOGGLE
functionality above, so replacing this with "send the EVENTS_API_DRAWER_TOGGLE
event to window
" eliminates that duplication.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8278 +/- ##
===========================================
+ Coverage 46.62% 92.20% +45.58%
===========================================
Files 626 626
Lines 30996 30922 -74
===========================================
+ Hits 14451 28511 +14060
+ Misses 16545 2411 -14134
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
* main: (30 commits) web: clear out selecteds list after an API event to ensure a fresh copy of the policies-to-delete list (#8125) web: provide dual-list multiselect with pagination (#8004) web: provide a context for checking the status of the enterprise license (#8153) core: compile backend translations (#8311) translate: Updates for file web/xliff/en.xlf in zh-Hans (#8304) translate: Updates for file web/xliff/en.xlf in zh_CN (#8305) translate: Updates for file locale/en/LC_MESSAGES/django.po in zh_CN (#8300) translate: Updates for file locale/en/LC_MESSAGES/django.po in zh-Hans (#8301) events: fix missing labels on prometheus metrics (#8309) core: bump goauthentik.io/api/v3 from 3.2023106.4 to 3.2023106.5 (#8302) web: bump the wdio group in /tests/wdio with 4 updates (#8303) web: restore test anchor tag (#8298) translate: Updates for file web/xliff/en.xlf in fr (#8296) translate: Updates for file locale/en/LC_MESSAGES/django.po in fr (#8295) website: update wording (#8290) enterrpise: exclude inactive users from license (#8294) web: bump API Client version (#8292) core: compile backend translations (#8291) events: migrate SystemTasks to DB (#8159) web/admin: fix footer links not being parsed on settings page (#8289) ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
return nothing; | ||
} | ||
|
||
const onClick = (ev: Event) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would probably rename this to renderApiDrawerTrigger
since it doesn't render the actual drawer
web/src/user/UserInterface.ts
Outdated
</div>`; | ||
} | ||
|
||
renderNotificationDrawer() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
web/src/user/UserInterface.ts
Outdated
</div>`; | ||
} | ||
|
||
renderAccessAdmin() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renderAccessAdmin() { | |
renderAdminInterfaceLink() { |
this.ws = new WebsocketClient(); | ||
this.fetchConfigurationDetails(); | ||
configureSentry(true); | ||
this.toggleNotificationDrawer = this.toggleNotificationDrawer.bind(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching between all these interfaces without a full reload would definitely be very nice to have
* main: (331 commits) website/docs: installation: kubernetes: fix values (#8783) web: bump the wdio group in /tests/wdio with 4 updates (#8789) core: bump github.com/stretchr/testify from 1.8.4 to 1.9.0 (#8790) core: bump twisted from 23.10.0 to 24.3.0 (#8788) translate: Updates for file locale/en/LC_MESSAGES/django.po in zh_CN (#8778) translate: Updates for file locale/en/LC_MESSAGES/django.po in zh-Hans (#8779) root: ensure consistent install_id (#8775) web: bump the sentry group in /web with 1 update (#8762) web: bump style-mod from 4.1.1 to 4.1.2 in /web (#8763) website: bump @types/react from 18.2.60 to 18.2.61 in /website (#8764) core: bump goauthentik.io/api/v3 from 3.2024021.2 to 3.2024021.3 (#8765) core: bump ruff from 0.2.2 to 0.3.0 (#8766) core: bump twilio from 8.13.0 to 9.0.0 (#8767) translate: Updates for file locale/en/LC_MESSAGES/django.po in fr (#8774) core, web: update translations (#8759) web/admin: don't mark LDAP group property mappings as required (#8772) website/docs: move Applications docs up a level, other edits (#8712) web/admin: don't mark property mappings as required anywhere (#8752) website: redirect root to /docs (#8754) web: bump API Client version (#8753) ...
✅ Deploy Preview for authentik-docs canceled.
|
* main: web: clean up UserInterface in prep for OAuth and Silo Projects (#8278) website/docs: installation: kubernetes: fix values (#8783) web: bump the wdio group in /tests/wdio with 4 updates (#8789) core: bump github.com/stretchr/testify from 1.8.4 to 1.9.0 (#8790) core: bump twisted from 23.10.0 to 24.3.0 (#8788) translate: Updates for file locale/en/LC_MESSAGES/django.po in zh_CN (#8778) translate: Updates for file locale/en/LC_MESSAGES/django.po in zh-Hans (#8779) root: ensure consistent install_id (#8775) web: bump the sentry group in /web with 1 update (#8762) web: bump style-mod from 4.1.1 to 4.1.2 in /web (#8763) website: bump @types/react from 18.2.60 to 18.2.61 in /website (#8764) core: bump goauthentik.io/api/v3 from 3.2024021.2 to 3.2024021.3 (#8765) core: bump ruff from 0.2.2 to 0.3.0 (#8766) core: bump twilio from 8.13.0 to 9.0.0 (#8767)
* main: website: fix missing compose file (#8809) core: bump django from 5.0.2 to 5.0.3 (#8808) core: bump github.com/go-openapi/strfmt from 0.22.1 to 0.22.2 (#8801) core, web: update translations (#8800) core: bump goauthentik.io/api/v3 from 3.2024021.3 to 3.2024022.1 (#8802) core: bump golang.org/x/oauth2 from 0.17.0 to 0.18.0 (#8803) core: bump github.com/go-openapi/runtime from 0.27.1 to 0.27.2 (#8804) website: bump @types/react from 18.2.61 to 18.2.62 in /website (#8805) web: bump the eslint group in /tests/wdio with 2 updates (#8806) web: bump the eslint group in /web with 2 updates (#8807) website/integrations: fix typo in proxmox docs (#8791) web: bump API Client version (#8797) release: 2024.2.2 website/docs: prepare 2024.2.2 release notes (#8782) flows: fix mismatched redirect behaviour for invalid and valid flows (#8794) providers/oauth2: fix validation ordering (#8793) web: clean up UserInterface in prep for OAuth and Silo Projects (#8278)
* web/replace-rollup-with-esbuild: website: fix missing compose file (#8809) core: bump django from 5.0.2 to 5.0.3 (#8808) core: bump github.com/go-openapi/strfmt from 0.22.1 to 0.22.2 (#8801) core, web: update translations (#8800) core: bump goauthentik.io/api/v3 from 3.2024021.3 to 3.2024022.1 (#8802) core: bump golang.org/x/oauth2 from 0.17.0 to 0.18.0 (#8803) core: bump github.com/go-openapi/runtime from 0.27.1 to 0.27.2 (#8804) website: bump @types/react from 18.2.61 to 18.2.62 in /website (#8805) web: bump the eslint group in /tests/wdio with 2 updates (#8806) web: bump the eslint group in /web with 2 updates (#8807) website/integrations: fix typo in proxmox docs (#8791) web: bump API Client version (#8797) release: 2024.2.2 website/docs: prepare 2024.2.2 release notes (#8782) flows: fix mismatched redirect behaviour for invalid and valid flows (#8794) providers/oauth2: fix validation ordering (#8793) web: clean up UserInterface in prep for OAuth and Silo Projects (#8278)
While looking through @BeryJu's Oauth-for-authentik stuff, changes to
UserInterface
triggered the "harder" eslint pass, which said "UserInterface exceeds permitted complexity (9)." I couldn't disagree; it had lots of conditionals.This commit:
firstUpdated
tofetchConfigurationDetails
, and calls it in the constructor. There ought to be no circumstances where this object is constructed outside a working environment; no sense in waiting until you've done arender() { nothing }
pass to fetch details.Oddities: There are a pair of
<!-- -->
HTML comments in the framingrender()
; those are there just to stop prettier from slamming a string of conditional renders all into one line, making them harder to read.Checklist
ak test authentik/
)make lint-fix
)If an API change has been made
make gen-build
)If changes to the frontend have been made
make web
)make i18n-extract
)If applicable
make website
)