-
Notifications
You must be signed in to change notification settings - Fork 1.2k
UI: Fix all list items appearing twice in search view #10365
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 4.20 #10365 +/- ##
============================================
+ Coverage 16.13% 16.50% +0.36%
- Complexity 12972 13831 +859
============================================
Files 5639 5649 +10
Lines 494297 521066 +26769
Branches 59908 70175 +10267
============================================
+ Hits 79773 85978 +6205
- Misses 405698 425682 +19984
- Partials 8826 9406 +580
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@shwstppr a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress. |
UI build: ✔️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blueorangutan package |
@Pearl1594 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. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12416 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pearl1594, in the #9947 PR, that was targeted into the main
branch, I addressed this fix in a slightly different way.
The SearchView
component returns two HTML elements that do the same thing. One is from line 83 to 92:
cloudstack/ui/src/components/view/SearchView.vue
Lines 83 to 92 in a7beaaf
<span v-if="(field.name.startsWith('domain') || field.name === 'account')"> | |
<span v-if="opt.icon"> | |
<resource-icon :image="opt.icon.base64image" size="1x" style="margin-right: 5px"/> | |
</span> | |
<block-outlined v-else style="margin-right: 5px" /> | |
</span> | |
<span v-if="(field.name.startsWith('managementserver'))"> | |
<status :text="opt.state" /> | |
</span> | |
{{ $t((['storageid'].includes(field.name) || !opt.path) ? opt.name : opt.path) }} |
And the other is between line 93 to 99:
cloudstack/ui/src/components/view/SearchView.vue
Lines 93 to 99 in a7beaaf
<span v-if="(field.name.startsWith('associatednetwork'))"> | |
<span v-if="opt.icon"> | |
<resource-icon :image="opt.icon.base64image" size="1x" style="margin-right: 5px"/> | |
</span> | |
<block-outlined v-else style="margin-right: 5px" /> | |
</span> | |
{{ $t(opt.path || opt.name) }} |
Given that, both redundant HTML elements were grouped into a single one by moving the condition (v-if
directive) from the second element into the first one.
Both ways resolve the bug, so I think that we can merge this one, and, if so, it would not be required to merge it into the main
branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code lgtm
my fault
thanks for the fixing @Pearl1594
Given it is already fixed on |
First, I was completely unaware there was a PR that addressed a similar issue, my bad. Secondly, I maybe wrong, but I am not sure if the above mentioned fix, addresses the duplicate names being printed for any resource of list type. Removing the v-if altogether could have a negative impact in which resources that haven't been specifically mentioned in the v-if block may not have any items in the drop down menu. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, I was completely unaware there was a PR that addressed a similar issue, my bad.
No problem. I should have backported the fix to the 4.20
branch.
Secondly, I maybe wrong, but I am not sure if the above mentioned fix, addresses the duplicate names being printed for any resource of list type. Removing the v-if altogether could have a negative impact in which resources that haven't been specifically mentioned in the v-if block may not have any items in the drop down menu.
Currently, according to the following block of code:
cloudstack/ui/src/components/view/SearchView.vue
Lines 83 to 92 in a7beaaf
<span v-if="(field.name.startsWith('domain') || field.name === 'account')"> | |
<span v-if="opt.icon"> | |
<resource-icon :image="opt.icon.base64image" size="1x" style="margin-right: 5px"/> | |
</span> | |
<block-outlined v-else style="margin-right: 5px" /> | |
</span> | |
<span v-if="(field.name.startsWith('managementserver'))"> | |
<status :text="opt.state" /> | |
</span> | |
{{ $t((['storageid'].includes(field.name) || !opt.path) ? opt.name : opt.path) }} |
If the field.name
starts with domain
or if it is equal to account
, the following HTML is returned:
<span v-if="opt.icon">
<resource-icon :image="opt.icon.base64image" size="1x" style="margin-right: 5px"/>
</span>
<block-outlined v-else style="margin-right: 5px" />
In this situation, the rendered filter option text is equal to:
{{ $t((['storageid'].includes(field.name) || !opt.path) ? opt.name : opt.path) }}
Now, according to the following block of code:
cloudstack/ui/src/components/view/SearchView.vue
Lines 93 to 99 in a7beaaf
<span v-if="(field.name.startsWith('associatednetwork'))"> | |
<span v-if="opt.icon"> | |
<resource-icon :image="opt.icon.base64image" size="1x" style="margin-right: 5px"/> | |
</span> | |
<block-outlined v-else style="margin-right: 5px" /> | |
</span> | |
{{ $t(opt.path || opt.name) }} |
If the field.name
starts with associatednetwork
, the following HTML will be returned:
<span v-if="opt.icon">
<resource-icon :image="opt.icon.base64image" size="1x" style="margin-right: 5px"/>
</span>
<block-outlined v-else style="margin-right: 5px" />
In this scenario, the rendered filter option text is equal to:
{{ $t(opt.path || opt.name) }}
Therefore, since (i) both returned HTML elements are equal; (ii) and both returned filter option texts are semantically equal, the solution proposed in #9947 will not have any side effects.
Regarding this PR, it is important to note that, although it fixes duplicated filter options, it does not fix the selection of domains in the SearchView
, which was the original issue (#10310).
Here is a video of the current behavior in the QA enviroment:
2025-02-12.10-33-42.mp4
If a domain has any associated accounts, the user is never able to select the domain in the SearchView
. This bug is happening because of the following block of code:
cloudstack/ui/src/components/view/SearchView.vue
Lines 710 to 722 in a7beaaf
if (networkIndex > -1) { | |
this.fields[networkIndex].loading = false | |
} | |
if (usageTypeIndex > -1) { | |
this.fields[usageTypeIndex].loading = false | |
} | |
if (Array.isArray(arrayField)) { | |
this.fillFormFieldValues() | |
} | |
if (networkIndex > -1) { | |
this.fields[networkIndex].loading = false | |
} | |
this.fillFormFieldValues() |
Note that lines from 719 up to 722 are duplicated. After removing these duplicated lines, the bug is fixed.
Here is a video with those lines removed in my local environment:
2025-02-12.10-48-59.mp4
Oh, I finally understood what you were trying to convey... I've cherry-picked your commit @bernardodemarco that addressed this issue on main. Thanks. Hope this is good. |
@weizhouapache a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress. |
@Pearl1594, looks good. Now, we just need to fix the bug present in the first video: 2025-02-12.10-33-42.mp4It happens when a domain has accounts within it. To fix it, we should remove the lines 719, 720, 721 and 722 from the following block of code: cloudstack/ui/src/components/view/SearchView.vue Lines 710 to 722 in a7beaaf
|
UI build: ✔️ |
@Pearl1594 @bernardodemarco I verified the original issue in the title on QA, it looks ok now should we merge this PR, address the issue (#10365 (comment)) in another PR ? |
Ok, no problem for me. I'll open a new issue to address this bug and will try to fix it. |
|
good, thanks |
@weizhouapache, @Pearl1594, I've just opened a PR (#10386) to address the issue (#10365 (comment)) |
* UI: Fix all list items appearing twice in search view * Revert "UI: Fix all list items appearing twice in search view" This reverts commit 2739c01. * fix duplicate option names on list filters --------- Co-authored-by: Bernardo De Marco Gonçalves <[email protected]>
Description
Fixes: #10310
This PR fixes the duplicate names appearing in the search filters in drop down menus
Before fix:
After fix:

Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?