UI: fix 404 after bulk deleting template zones#12681
UI: fix 404 after bulk deleting template zones#12681dheeraj12347 wants to merge 2 commits intoapache:mainfrom
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
|
shwstppr
left a comment
shwstppr
left a comment
There was a problem hiding this comment.
Sorry but I'm just seeing some refactors. Am I missing something?
There was a problem hiding this comment.
Pull request overview
This PR claims to fix a 404 issue after bulk deleting template zones by redirecting to the Templates list view when all zones are deleted. However, the actual changes consist entirely of formatting modifications with no functional bug fix implemented.
Changes:
- Indentation fixes for router-link elements
- Multi-line reformatting of pageSizeOptions array (inconsistent with codebase conventions)
- String concatenation refactoring for v-html attribute (inconsistent with codebase conventions)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, | ||
| computed: { | ||
| isActionsOnTemplatePermitted () { | ||
| return (['Admin'].includes(this.$store.getters.userInfo.roletype) || // If admin or owner or belongs to current project | ||
| return (['Admin'].includes(this.$store.getters.userInfo.roletype) || | ||
| (this.resource.domainid === this.$store.getters.userInfo.domainid && this.resource.account === this.$store.getters.userInfo.account) || | ||
| (this.resource.domainid === this.$store.getters.userInfo.domainid && this.resource.projectid && this.$store.getters.project && this.$store.getters.project.id && this.resource.projectid === this.$store.getters.project.id)) && | ||
| (this.resource.isready || !this.resource.status || this.resource.status.indexOf('Downloaded') === -1) && // Template is ready or downloaded | ||
| (this.resource.isready || !this.resource.status || this.resource.status.indexOf('Downloaded') === -1) && | ||
| this.resource.templatetype !== 'SYSTEM' | ||
| } | ||
| }, |
There was a problem hiding this comment.
This PR description claims to fix a 404 issue after bulk deleting template zones by redirecting to the Templates list view when all zones are deleted. However, the diff only contains formatting changes with no actual logic modifications to address this issue. The actual bug fix for the 404 navigation issue is missing from this PR. The handleCancel method (around line 486) still uses this.$router.go(-1) which goes back in browser history and could lead to a 404 page. It should be changed to this.$router.push({ path: '/template' }) to explicitly redirect to the Templates list.
ui/src/views/image/TemplateZones.vue
Outdated
| :pageSizeOptions="[ | ||
| '10', | ||
| '20', | ||
| '40', | ||
| '80', | ||
| '100' | ||
| ]" |
There was a problem hiding this comment.
This multi-line formatting of the pageSizeOptions array is inconsistent with the codebase convention. Throughout the codebase (e.g., IsoZones.vue:133, FirewallRules.vue:136, MigrateWizard.vue:90), pageSizeOptions arrays are consistently formatted as inline arrays. This should be kept as :pageSizeOptions="['10', '20', '40', '80', '100']" to maintain consistency.
| :pageSizeOptions="[ | |
| '10', | |
| '20', | |
| '40', | |
| '80', | |
| '100' | |
| ]" | |
| :pageSizeOptions="['10', '20', '40', '80', '100']" |
ui/src/views/image/TemplateZones.vue
Outdated
| <span style="padding-left: 5px" v-html="`<b>${selectedRowKeys.length} ` + $t('label.items.selected') + `. </b>`" /> | ||
| <span | ||
| style="padding-left: 5px" | ||
| v-html="'<b>' + selectedRowKeys.length + ' ' + $t('label.items.selected') + '. </b>'" /> |
There was a problem hiding this comment.
This string concatenation change is inconsistent with the codebase convention and reduces readability. The codebase consistently uses template literals for v-html attributes with interpolated values (see AutogenView.vue:229, 241 and SnapshotZones.vue:202). This should use template literals like: v-html="\${selectedRowKeys.length} ` + $t('label.items.selected') + `. `"`
| v-html="'<b>' + selectedRowKeys.length + ' ' + $t('label.items.selected') + '. </b>'" /> | |
| v-html="`<b>${selectedRowKeys.length} ` + $t('label.items.selected') + `. </b>`" /> |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12681 +/- ##
============================================
- Coverage 17.94% 17.92% -0.02%
+ Complexity 16165 16158 -7
============================================
Files 5939 5939
Lines 533015 533181 +166
Branches 65218 65237 +19
============================================
- Hits 95649 95595 -54
- Misses 426638 426846 +208
- Partials 10728 10740 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@shwstppr Thanks for your question! You were correct—the first commit (2244c01) only had formatting changes. I've now pushed a new commit (cf2f331) that includes the actual 404 bug fix: What was added in the new commit:
The previous comments from Copilot about missing the actual bug fix have now been addressed. Please let me know if you'd like me to make any other adjustments! |
|
@dheeraj12347 maybe it would make sense to keep the changes just from the second commit |
2 participants
Description
Fixes an issue where the UI navigates to a 404 page after bulk deleting template zones from the Templates → Zones tab.
After this change, when all selected zones for a template are deleted, the UI redirects back to the Templates list view instead of showing the 404 screen. When some zones remain, the page stays on the template view as expected. [web:327][web:335]
Types of changes
How Has This Been Tested?