Fix link to removed volumes being shown in info card and list view #8833

winterhazel · 2024-03-26T11:52:12Z

Description

The volume snapshots and the snapshot details page show a link to the snapshotted volume. However, if the volume has been removed, it is not possible to access its page, resulting in a 404 when a user clicks the link.

This PR adds a validation in the UI in order to avoid showing this link when the volume has been removed.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Bug Severity

Screenshots (if appropriate):

When the volume exists, a link is shown.

If the volume has been removed, the link does not get shown.

How Has This Been Tested?

I created a volume snapshot and verified that the volume snapshots and the snapshot details page showed a link to the volume;
I removed the volume and verified that the link was not shown anymore.

codecov · 2024-03-26T12:00:07Z

Codecov Report

Attention: Patch coverage is 21.52918% with 390 lines in your changes missing coverage. Please review.

Project coverage is 15.42%. Comparing base (53faf0f) to head (e558ca6).
Report is 18 commits behind head on 4.19.

❗ Current head e558ca6 differs from pull request most recent head 62e405d

Please upload reports for the commit 62e405d to get more accurate results.

Files	Patch %	Lines
agent/src/main/java/com/cloud/agent/Agent.java	0.00%	62 Missing ⚠️
...ommand/admin/network/CreateNetworkOfferingCmd.java	0.00%	50 Missing ⚠️
...ck/api/command/admin/vpc/CreateVPCOfferingCmd.java	11.62%	35 Missing and 3 partials ⚠️
...nt/resource/consoleproxy/ConsoleProxyResource.java	0.00%	36 Missing ⚠️
...in/java/com/cloud/agent/api/storage/OVFHelper.java	10.71%	24 Missing and 1 partial ⚠️
...gent/src/main/java/com/cloud/agent/AgentShell.java	9.09%	20 Missing ⚠️
api/src/main/java/com/cloud/storage/Storage.java	68.33%	16 Missing and 3 partials ⚠️
...pi/src/main/java/com/cloud/agent/api/to/NicTO.java	0.00%	12 Missing ⚠️
.../main/java/com/cloud/deploy/DeploymentPlanner.java	7.69%	12 Missing ⚠️
...nternallb/ListInternalLoadBalancerElementsCmd.java	0.00%	11 Missing ⚠️
... and 49 more

Additional details and impacted files

@@             Coverage Diff              @@
##               4.19    #8833      +/-   ##
============================================
+ Coverage     14.97%   15.42%   +0.45%     
- Complexity    11049    11818     +769     
============================================
  Files          5390     5475      +85     
  Lines        470964   478624    +7660     
  Branches      59210    60820    +1610     
============================================
+ Hits          70528    73834    +3306     
- Misses       392599   396657    +4058     
- Partials       7837     8133     +296

Flag	Coverage Δ
uitests	`4.21% <ø> (-0.07%)`	⬇️
unittests	`16.18% <21.52%> (+0.49%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

sureshanaparti · 2024-03-27T09:23:06Z

@blueorangutan ui

blueorangutan · 2024-03-27T09:24:03Z

@sureshanaparti a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan · 2024-03-27T09:39:20Z

UI build: ✔️
Live QA URL: https://qa.cloudstack.cloud/simulator/pr/8833 (QA-JID-309)

DaanHoogland

clgtm

DaanHoogland · 2024-03-29T14:19:40Z

qa does not seem to be responsive, not tested!

lucas-a-martins

LGTM

I manually tested by creating a volume snapshot and then deleting the volume.

Before the proposed changes

The snapshot details page shows a link to the volume even after the volume has been removed.

Clicking the link redirected me to a 404 page.

After the proposed changes

The snapshot details page does not show a link to the removed volume.

I'm not sure if this is within the scope of this PR, but on the Volume Snapshots page, where it displays the list of volume snapshots, a link to the removed volume is still being shown and redirects to a 404 page.

DaanHoogland · 2024-04-24T06:40:24Z

@winterhazel , I marked this for 4.20 now as it is targeted for main. Do you want this in 4.19?

cc @lucas-a-martins

winterhazel · 2024-04-24T15:35:24Z

Thanks for testing @lucas-a-martins. At first, I think the issue regarding invalid links in the list view should be tackled in another PR, but I'll investigate how it could be implemented to see if it makes sense to unify how the validation is performed with this one.

@winterhazel , I marked this for 4.20 now as it is targeted for main. Do you want this in 4.19?

@DaanHoogland sure, I'll target 4.19.

winterhazel · 2024-05-28T17:30:00Z

Hey @lucas-a-martins @DaanHoogland, sorry for the delay.

Looking back at this PR, I think it would be better to implement the link validation in both pages by including the volume's state in the main response and verifying it in the UI, as the approach here (calling another API to obtain a single volume's data) would become unfeasible when we need to validate links for a lot of resources (such as in the volume snapshots page). I suppose this would also be the ideal approach for validating most links to resources.

I will make some changes in this PR to adjust this, and to include the validation in the volume snapshots page as well.

winterhazel · 2024-06-27T14:01:26Z

@lucas-a-martins @DaanHoogland, I changed how the link validation is performed, and included it in the list view as well. Could you guys take another look at this PR?

github-actions · 2024-06-28T11:47:29Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

DaanHoogland

clgtm

lucas-a-martins

LGTM

I tested the current iteration of this PR and everything is working as intended, including the changes to the volume snapshot list view.

Volume snapshot view

Before deleting the volume:

After deleting the volume:

Snapshot details view

Before deleting the volume:

After deleting the volume:

DaanHoogland · 2024-07-23T08:28:20Z

@blueorangutan package

blueorangutan · 2024-07-23T08:30:03Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-07-23T09:34:03Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10444

DaanHoogland · 2024-07-23T12:00:53Z

@blueorangutan test

blueorangutan · 2024-07-23T12:02:03Z

@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2024-07-23T12:23:44Z

[SF] Trillian Build Failed (tid-10948)

blueorangutan · 2024-07-24T02:38:57Z

[SF] Trillian test result (tid-10950)
Environment: kvm-alma9 (x2), Advanced Networking with Mgmt server a9
Total time taken: 46197 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8833-t10950-kvm-alma9.zip
Smoke tests completed. 132 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

JoaoJandre · 2024-07-24T12:08:59Z

Merging based on approvals, CI result and manual tests by @lucas-a-martins

…pache#8833) * Framework for validating links in the front-end * Rename valid links map in the list view

boring-cyborg bot added the component:UI label Mar 26, 2024

DaanHoogland approved these changes Mar 29, 2024

View reviewed changes

DaanHoogland added the status:needs-testing label Mar 29, 2024

lucas-a-martins approved these changes Apr 23, 2024

View reviewed changes

DaanHoogland removed the status:needs-testing label Apr 24, 2024

DaanHoogland added this to the 4.20.0.0 milestone Apr 24, 2024

boring-cyborg bot added the component:api label Jun 27, 2024

winterhazel requested review from DaanHoogland and lucas-a-martins June 27, 2024 14:01

winterhazel added 2 commits June 28, 2024 08:45

Framework for validating links in the front-end

893773c

Rename valid links map in the list view

62e405d

winterhazel force-pushed the fix-404-in-info-card-volume branch from e558ca6 to 62e405d Compare June 28, 2024 11:47

boring-cyborg bot added component:agent component:backup component:compute component:kvm component:orchestration component:storage component:vmware labels Jun 28, 2024

winterhazel changed the base branch from main to 4.19 June 28, 2024 11:47

github-actions bot added the status:has-conflicts label Jun 28, 2024

winterhazel removed component:kvm component:vmware component:agent component:compute component:orchestration component:backup component:storage status:has-conflicts labels Jun 28, 2024

winterhazel changed the title ~~Fix link to removed volumes being shown in info card~~ Fix link to removed volumes being shown in info card and list view Jun 28, 2024

rohityadavcloud modified the milestones: 4.20.0.0, 4.19.2.0 Jun 29, 2024

DaanHoogland approved these changes Jul 1, 2024

View reviewed changes

DaanHoogland added the status:needs-testing label Jul 1, 2024

lucas-a-martins approved these changes Jul 22, 2024

View reviewed changes

DaanHoogland removed the status:needs-testing label Jul 23, 2024

JoaoJandre merged commit 49cd5ba into apache:4.19 Jul 24, 2024

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jul 31, 2024

Fix link to removed volumes being shown in info card and list view (a…

2ab4e7f

…pache#8833) * Framework for validating links in the front-end * Rename valid links map in the list view

Fix link to removed volumes being shown in info card and list view #8833

Fix link to removed volumes being shown in info card and list view #8833

Uh oh!

Conversation

winterhazel commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

codecov bot commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sureshanaparti commented Mar 27, 2024

Uh oh!

blueorangutan commented Mar 27, 2024

Uh oh!

blueorangutan commented Mar 27, 2024

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Mar 29, 2024

Uh oh!

lucas-a-martins left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

winterhazel commented Apr 24, 2024

Uh oh!

winterhazel commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

winterhazel commented Jun 27, 2024

Uh oh!

github-actions bot commented Jun 28, 2024

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

lucas-a-martins left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jul 23, 2024

Uh oh!

blueorangutan commented Jul 23, 2024

Uh oh!

blueorangutan commented Jul 23, 2024

Uh oh!

DaanHoogland commented Jul 23, 2024

Uh oh!

blueorangutan commented Jul 23, 2024

Uh oh!

blueorangutan commented Jul 23, 2024

Uh oh!

blueorangutan commented Jul 24, 2024

Uh oh!

JoaoJandre commented Jul 24, 2024

Uh oh!

Uh oh!

winterhazel commented Mar 26, 2024 •

edited

Loading

codecov bot commented Mar 26, 2024 •

edited

Loading

DaanHoogland commented Apr 24, 2024 •

edited

Loading

winterhazel commented May 28, 2024 •

edited

Loading

lucas-a-martins left a comment •

edited

Loading