Skip to content

UI: Fix delete ISO navigation after job is finished#6598

Merged
shwstppr merged 3 commits intoapache:4.17from
shapeblue:fixdelisonavigation
Aug 3, 2022
Merged

UI: Fix delete ISO navigation after job is finished#6598
shwstppr merged 3 commits intoapache:4.17from
shapeblue:fixdelisonavigation

Conversation

@nvazquez
Copy link
Copy Markdown
Contributor

@nvazquez nvazquez commented Aug 2, 2022

Description

This PR fixes the navigation for ISOs after deletion. The following issues are addressed:

  • After removal of an ISO without bulk operation -> The view keeps displaying the data of the removed ISO
  • After removal of an ISO selecting bulk operation -> The view redirects to the 404 exception view

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

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

The following scenarios were tested on a single zone:

  • Delete ISO without selecting any zone -> After removal, the ISO view is displayed
  • Delete ISO selecting the zone and clicking on 'Bulk operation' -> After removal, the ISO view is displayed

@acs-robot
Copy link
Copy Markdown

Found UI changes, kicking a new UI QA build
@blueorangutan ui

@blueorangutan
Copy link
Copy Markdown

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

@blueorangutan
Copy link
Copy Markdown

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6598 (SL-JID-2050)

Copy link
Copy Markdown

@utchoang utchoang left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

tested in qa, lgtm

Copy link
Copy Markdown
Contributor

@shwstppr shwstppr left a comment

Choose a reason for hiding this comment

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

@nvazquez I see an issue with this. After iso is deleted view is correctly moved iso list view but if I press back there then 404 is seen as this is adding item to the stack

Copy link
Copy Markdown
Member

@weizhouapache weizhouapache 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

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Aug 2, 2022

so we should remove from the history @shwstppr ?

@shwstppr
Copy link
Copy Markdown
Contributor

shwstppr commented Aug 2, 2022

@rohityadavcloud yes, instead of pushing new route maybe we can replace or rather use something like $router.go(-2) when $router.go(-1) isn't resolvable

@slavkap
Copy link
Copy Markdown
Contributor

slavkap commented Aug 2, 2022

We discussed offline with @nvazquez for a few scenarios (if you close the progress window before the job is finnished) in which the 404 page shows again.

@nvazquez
Copy link
Copy Markdown
Contributor Author

nvazquez commented Aug 2, 2022

@shwstppr @utchoang do you know how to deal with 404 error when closing the bulk operation dialog? This 404 error is shown while the delete operation is in progress, after the delete finishes then it redirects correctly to the ISOs view

@shwstppr
Copy link
Copy Markdown
Contributor

shwstppr commented Aug 2, 2022

I think the 404 error can be prevented by checking the result of navigation while moving back from a view. With following changes I was able to see the difference

diff --git a/ui/src/views/image/IsoZones.vue b/ui/src/views/image/IsoZones.vue
index 7ca07ebb4b..74f4e39add 100644
--- a/ui/src/views/image/IsoZones.vue
+++ b/ui/src/views/image/IsoZones.vue
@@ -340,6 +340,13 @@ export default {
       this.selectedRowKeys = []
       this.fetchData()
       if (this.dataSource.length === 0) {
+        this.moveToPreviousView()
+      }
+    },
+    async moveToPreviousView () {
+      const lastPath = this.$router.currentRoute.value.fullPath
+      const navigationResult = await this.$router.go(-1)
+      if (navigationResult !== undefined || this.$router.currentRoute.value.fullPath === lastPath) {
         this.$router.go(-1)
       }
     },
@@ -379,7 +386,7 @@ export default {
           successMethod: result => {
             if (singleZone) {
               if (this.selectedItems.length === 0) {
-                this.$router.go(-1)
+                this.moveToPreviousView()
               }
             } else {
               if (this.selectedItems.length === 0) {
@@ -388,6 +395,9 @@ export default {
             }
             if (this.selectedItems.length > 0) {
               eventBus.emit('update-resource-state', { selectedItems: this.selectedItems, resource: record.zoneid, state: 'success' })
+              if (this.selectedItems.length === this.zones.length) {
+                this.moveToPreviousView()
+              }
             }
           },
           errorMethod: () => {

@acs-robot
Copy link
Copy Markdown

Found UI changes, kicking a new UI QA build
@blueorangutan ui

@blueorangutan
Copy link
Copy Markdown

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

@blueorangutan
Copy link
Copy Markdown

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6598 (SL-JID-2053)

@acs-robot
Copy link
Copy Markdown

Found UI changes, kicking a new UI QA build
@blueorangutan ui

@blueorangutan
Copy link
Copy Markdown

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

@nvazquez
Copy link
Copy Markdown
Contributor Author

nvazquez commented Aug 3, 2022

Thanks @shwstppr @slavkap, I think the issue is solved after applying @shwstppr's fix and a minor tweak to it, can you re-review?

@blueorangutan
Copy link
Copy Markdown

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6598 (SL-JID-2061)

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Aug 3, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Aug 3, 2022

@shwstppr is this ready for merging?

@shwstppr shwstppr merged commit 7d50b65 into apache:4.17 Aug 3, 2022
neogismm pushed a commit to neogismm/cloudstack that referenced this pull request Aug 6, 2022
* UI: Fix delete ISO navigation after job is finished

* Apply suggestion

* Fix redirection
@nvazquez nvazquez deleted the fixdelisonavigation branch January 30, 2023 14:36
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.

9 participants