Skip to content

fix: decouple UI layout from admin routes #961

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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

jescalada
Copy link
Contributor

@jescalada jescalada commented Mar 28, 2025

Fixes #918, along with #917.

This PR aims to make route access more intuitive. All /admin routes have been replaced with /dashboard, and relevant files have been renamed. Actual admin-only routes, such as the list of users, are displayed with the /admin/ prefix. For example: /dashboard/admin/user.

Unit tests execute the same way as before, and E2E tests have been expanded and amended to work with the new setup.

I also changed the login flow to redirect to the repo list (which seems more intuitive than one's profile), and the username update flow to redirect to one's own profile instead of the "user view" admin route (which would error out for non-admins).

Finally, I added a config entry to define what routes have a specific auth requirement. The rules are shown by default but not enabled.

"uiRouteAuth": {
    "enabled": false,
    "rules": [
      {
        "pattern": "/dashboard/*",
        "adminOnly": false,
        "loginRequired": true
      },
      {
        "pattern": "/admin/*",
        "adminOnly": true,
        "loginRequired": true
      }
    ]
  }

Changelog

  • Add optional route redirection (admin-only, login required)
    • Can be configured through proxy.config.json
    • Configuration allows multiple regex rules
  • Refactor "admin" layout/routes into "dashboard"
  • Fix error handling on repo page
    • Catches GitHub repo retrieval error and makes it more descriptive
  • Add new E2E test for basic login flow (with admin/admin account) and redirect
  • Fix login Cypress command
    • Existing repo E2E test now logs in first

@github-actions github-actions bot added the fix label Mar 28, 2025
Copy link

netlify bot commented Mar 28, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit 351d67d
🔍 Latest deploy log https://app.netlify.com/sites/endearing-brigadeiros-63f9d0/deploys/681491fd6ffdff0008d2a66e

Copy link

codecov bot commented Apr 2, 2025

Codecov Report

Attention: Patch coverage is 44.44444% with 5 lines in your changes missing coverage. Please review.

Project coverage is 49.62%. Comparing base (9ad2d07) to head (351d67d).

Files with missing lines Patch % Lines
src/config/index.ts 40.00% 3 Missing ⚠️
src/service/routes/auth.js 50.00% 1 Missing ⚠️
src/service/routes/config.js 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #961      +/-   ##
==========================================
- Coverage   49.65%   49.62%   -0.03%     
==========================================
  Files          48       48              
  Lines        1718     1725       +7     
  Branches      175      176       +1     
==========================================
+ Hits          853      856       +3     
- Misses        841      845       +4     
  Partials       24       24              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JamieSlome
Copy link
Member

@jescalada - can you resolve conflicts and we will get this merged? 👏

@jescalada
Copy link
Contributor Author

Sure! As you mentioned in #917, I'll implement the optional check first, then it should be ready to review. 😃

@jescalada
Copy link
Contributor Author

Hi @JamieSlome, it should be good to review now! I added the config entry, new RouteGuard logic, and updated the PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add UI route authorization and decouple admin routes
2 participants