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

fix: add dynamic routes for editting storage pool and project configs [WD-8467] #627

Merged
merged 1 commit into from
Feb 1, 2024
Merged

fix: add dynamic routes for editting storage pool and project configs [WD-8467] #627

merged 1 commit into from
Feb 1, 2024

Conversation

mas-who
Copy link
Contributor

@mas-who mas-who commented Jan 30, 2024 •
edited
Loading

Done

  • Added dynamic url routing for editing storage pool and project configurations.
  • Adjusted storage volume route url pattern as it conflicts with the added storage pool config url pattern.
  • Automatically expand the Restrictions drop down if project restrictions are enabled. This is done for both edit and create project forms to keep behaviour consistent.

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 @lorumic or @edlerd for access.
    • With a local copy of this branch, run as described here.
  2. Perform the following QA steps:
    • Visit an existing ceph storage pool, navigate to the configurations tab and click on all side menu items within the edit form. Make sure can update configurations and the url changes depending on the active form menu item.
    • Go to project configurations, check all side menu items within the edit form i.e. Project details, Resource limits, Clusters, Instances, Device usage, Networks. Make sure configurations can be updated and url changes depending on the active form menu item.
    • Visit all storage volume paths i.e. storage volume overview, storage volume configuration, storage volume snapshots. Make sure those pages are accessible and all form menu items are accessible.
    • Visit create or edit project configuration forms. Enable project restriction, ensure that the Restrictions drop down opens automatically. Revisit an existing project (with restrictions enabled) configuration form, make sure that the Restrictions drop down is expanded.

@webteam-app
Copy link

Demo starting at https://lxd-ui-627.demos.haus

@mas-who
Copy link
Contributor Author

mas-who commented Jan 30, 2024

Checked again across the app and couldn't find any additional edit form sections that does not have a dynamic route mapping.

@mas-who mas-who changed the title fix: add dynamic routes for editting storage pool and project configs [WD-8467]] fix: add dynamic routes for editting storage pool and project configs [WD-8467] Jan 30, 2024
edlerd
edlerd reviewed Jan 30, 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.

When navigating to storage > volumes > any volume detail page, the overview tab is not active i.e. on this page: https://lxd-ui-627.demos.haus/ui/project/default/storage/detail/pool-dir/custom/abcdd

When reading project configuration, we should expand the "restrictions" submenu (if it is not disabled like in the default project on the demo server), i.e. for this page: https://lxd-ui-627.demos.haus/ui/project/A-very-very-very-very-very-very-very-very-very-very-long-name/configuration/clusters

@mas-who
Copy link
Contributor Author

mas-who commented Jan 30, 2024

When navigating to storage > volumes > any volume detail page, the overview tab is not active i.e. on this page: https://lxd-ui-627.demos.haus/ui/project/default/storage/detail/pool-dir/custom/abcdd

When reading project configuration, we should expand the "restrictions" submenu (if it is not disabled like in the default project on the demo server), i.e. for this page: https://lxd-ui-627.demos.haus/ui/project/A-very-very-very-very-very-very-very-very-very-very-long-name/configuration/clusters

Yeah sorry, I picked up that I missed a few dependencies from some tests failing, going to do a scan through then will request review again.

@mas-who mas-who force-pushed the form-menu-items-urls branch 3 times, most recently from 47d3492 to a2bd389 Compare January 31, 2024 08:56
@mas-who mas-who requested a review from edlerd January 31, 2024 16:45
@edlerd
Copy link
Collaborator

edlerd commented Jan 31, 2024 •
edited
Loading

When navigating to storage > volumes > any volume detail page, the overview tab is not active i.e. on this page: https://lxd-ui-627.demos.haus/ui/project/default/storage/detail/pool-dir/custom/abcdd

This doesn't work reliably yet, while on the 307 demo it does. Can you have a look?

@mas-who
Copy link
Contributor Author

mas-who commented Jan 31, 2024

When navigating to storage > volumes > any volume detail page, the overview tab is not active i.e. on this page: https://lxd-ui-627.demos.haus/ui/project/default/storage/detail/pool-dir/custom/abcdd

This doesn't work reliably yet, while on the 307 demo it does. Can you have a look?

Yes I found the issue, I missed that you can navigate to storage volumes from the storage pool detail overview page. I've created an issue to add a test for this scenario.

edlerd
edlerd approved these changes Feb 1, 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 LGTM, just a tiny nitpick on naming

src/pages/projects/EditProject.tsx Outdated
const { isRestricted } = useAuth();
const notify = useNotify();
const queryClient = useQueryClient();
const [section, setSection] = useState(PROJECT_DETAILS);
const { activeSection: section } = useParams<{ activeSection?: string }>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should rename the parameter activeSection to section for all the routes and its usages. Seems simpler to avoid the renaming in components, and we can streamline naming across the app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a good improvement, will implement that 👍

@mas-who
fix: add dynamic routes for editting storage pool and project configs
c6585f2 
Signed-off-by: Mason Hu <mason.hu@canonical.com>
@mas-who mas-who merged commit 3d00c0a into canonical:main Feb 1, 2024
6 checks passed
github-actions bot pushed a commit that referenced this pull request Feb 1, 2024
workflow: remove all reports for PR #627
f1cd118
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

Assignees
No one assigned
Labels
Review: Code +1 Review: QA +1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mas-who @webteam-app @edlerd