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(storage) add warning for project snapshot restriction [WD-7885] #577

Merged
merged 1 commit into from
Jan 5, 2024

Conversation

mas-who
Copy link
Contributor

@mas-who mas-who commented Dec 11, 2023

Done

  • If the project has snapshot restrictions, then a warning is shown for instance and custom storage volume snapshot settings page.
  • Removed a duplicated component StorageVolueSnapshotsForm.tsx. There is a pre-existing component StorageVolumeFormSnapshots.tsx which serves the exact same purpose.

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:
    • Navigate to the side panel Configuration tab, set Allow custom restrictions on a project level to true.
    • Navigate to a instance's detail page, under the Configuration tab, click on the Snapshots menu item and you should see a warning indicating that snapshot creation is blocked for the current project.
    • Navigate to the Snapshots tab within the same instance detail page, click on the See configuration button. You should see the same warning in the pop up modal.
    • Navigate to a custom storage volume's detail page, under the Configuration tab, click on the Snapshots menu item and you should see a warning indicating that snapshot creation is blocked for the current project.
    • Navigate to the Snapshots tab within the same custom storage volume detail page, click on the See configuration button. You should see the same warning in the pop up modal.

@webteam-app
Copy link

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

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 and QA looks good to me, maybe find a better or shorter copy as suggested below.

src/pages/storage/forms/StorageVolumeFormSnapshots.tsx Outdated Show resolved Hide resolved
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.

All good, thanks for adjusting the notification copy

@mas-who
Copy link
Contributor Author

mas-who commented Dec 13, 2023

All good, thanks for adjusting the notification copy

Should we still wait for piper to do a design review on this?

@piperdeck
Copy link

I think this copy might be a little clearer:

(Snapshots tab)
Snapshot creation has been disabled for instances in the the project {project name}.

(Warning message)
Snapshot creation has been disabled for instances in the project {project name}.
If you have administrator privileges, you can change this setting [here].

In the last message, would it be possible to include a link to the configuration page where the setting can be changed back?

@mas-who
Copy link
Contributor Author

mas-who commented Jan 3, 2024

I think this copy might be a little clearer:

(Snapshots tab) Snapshot creation has been disabled for instances in the the project {project name}.

(Warning message) Snapshot creation has been disabled for instances in the project {project name}. If you have administrator privileges, you can change this setting [here].

In the last message, would it be possible to include a link to the configuration page where the setting can be changed back?

@piperdeck the snapshots tab warning message makes sense. I think we can take it one step further for the configuration warning message. We can maybe display a different message for restricted users since they will not be allowed to edit project configurations. See below screenshots, WDYT? Also, this would be done for both instance and custom storage volume snapshot configurations.

For non-restricted users
Screenshot from 2024-01-03 10-28-29

For restricted users
Screenshot from 2024-01-03 10-29-30

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.

Ideas to simplify and reduce code below.

src/pages/storage/forms/StorageVolumeFormSnapshots.tsx Outdated Show resolved Hide resolved
@mas-who mas-who force-pushed the snapshot-restriction-warning branch from 269bff0 to 54e55c0 Compare January 5, 2024 07:23
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.

Thanks for the changes, tiny nitpick about copy. Links should always be descriptive of the target document, it is discouraged to use "here" as a link text.

src/components/SnapshotDiabledWarningLink.tsx Outdated Show resolved Hide resolved
Signed-off-by: Mason Hu <mason.hu@canonical.com>
@mas-who mas-who force-pushed the snapshot-restriction-warning branch from 54e55c0 to fcdd641 Compare January 5, 2024 09:54
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.

All good, thanks for applying the changes 👍

@edlerd edlerd merged commit 1c5ed3b into main Jan 5, 2024
6 checks passed
@edlerd edlerd deleted the snapshot-restriction-warning branch January 5, 2024 10:17
github-actions bot pushed a commit that referenced this pull request Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants