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

feat: introduce temporary admin mode for site config #1033

Merged
merged 4 commits into from
Feb 3, 2025

Conversation

dcshzj
Copy link
Contributor

@dcshzj dcshzj commented Jan 24, 2025

Problem

Ops load on engineers is quite high for editing the site config, navbar and footer.

Solution

Breaking Changes

  • Yes - this PR contains breaking changes
  • No - this PR is backwards compatible

Features:

  • Add a new /admin route for Isomer Admins to directly modify the site's config, theme, navbar and footer items.
    • Access to this feature is gated for just Isomer Admins, all other users will not know of its existence and even if they do, they will not have the permission to access the form to edit.

Before & After Screenshots

View for Isomer Admins:

image

View for unauthorised users:

image

Tests

  • Log in to Studio as a whitelisted Isomer Admin (your email should be whitelisted inside Growthbook).
  • Go to any site.
  • Verify that a star icon appears on the navigation bar on the left side.
  • Click on the star icon, verify that you are able to see the form to change the site configurations.
  • Change any configuration and save, verify that you are able to save successfully.
  • Navigate to any page and verify that those changes have persisted (or change another configuration that is more visible, such as the navigation bar or footer).
  • Log in to Studio as a non-whitelisted user.
  • Go to any site, verify that there are only 2 icons on the navigation bar on the left side.
  • Append /admin to the URL and navigate to the page.
  • Verify that the page shows that you do not have permission to access the page.

@dcshzj dcshzj requested a review from a team as a code owner January 24, 2025 02:31
@datadog-opengovsg
Copy link

datadog-opengovsg bot commented Jan 24, 2025

Datadog Report

Branch report: feat/site-config-admin
Commit report: 13c7e02
Test service: isomer-studio

✅ 0 Failed, 266 Passed, 36 Skipped, 48.73s Total Time
➡️ Test Sessions change in coverage: 1 no change

seaerchin
seaerchin previously approved these changes Jan 24, 2025
@seaerchin seaerchin dismissed their stale review January 24, 2025 09:13

missed something oopsie

Copy link
Contributor

@seaerchin seaerchin left a comment

Choose a reason for hiding this comment

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

minor issue - not too sure what the useEffect is doing! i think because this affects end user sites (and the site wide config), we might want to sanitise the json object to make sure it's valid (cos we're just parsing to make sure it's valid json, but not that the jsonobject matches what we expect - eg: {siteNavItems: [...]} vs {siteNavItem: [...]}

but no actionable re: the second point of matching - onlyi want to make sure the useEffect isn't sussy

Copy link
Contributor

@seaerchin seaerchin left a comment

Choose a reason for hiding this comment

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

mostly lgtm - i think blind spots for me are the useEffect, which is actually pretty confusing

the router method can be updated but not a real blocker, given that it's narrowly used and easy to infer from context

@dcshzj dcshzj merged commit 978680a into main Feb 3, 2025
16 of 17 checks passed
@dcshzj dcshzj deleted the feat/site-config-admin branch February 3, 2025 03:20
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.

2 participants