Add simple dropdown component and change severity filter to use it
What does this MR do and why?
This MR adds simple_dropdown.vue
and updates severity_filter.vue
to use it. There are a bunch of vulnerability report filters that repeat the same boilerplate code, and simple_dropdown.vue
refactors them into a reusable component.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Related to #383346 (closed)
Merge request reports
Activity
added workflowin dev label
assigned to @dftian
- A deleted user
added frontend label
3 Warnings af0b64cd: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. This merge request does not refer to an existing milestone. Most of the time, merge requests should target master
. Otherwise, please set the relevantPick into X.Y
label.1 Message This merge request adds or changes files that require a review from the Database team. This merge request requires a database review. To make sure these changes are reviewed, take the following steps:
- Ensure the merge request has database and databasereview pending labels. If the merge request modifies database files, Danger will do this for you.
- Prepare your MR for database review according to the docs.
- Assign and mention the database reviewer suggested by Reviewer Roulette.
If you no longer require a database review, you can remove this suggestion by removing the database label and re-running the
danger-review
job.Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer database João Pereira (
@jdrpereira
) (UTC+0, 10 hours ahead of@dftian
)Steve Abrams (
@sabrams
) (UTC-7, 3 hours ahead of@dftian
)frontend Tomáš Bulva (
@tbulva
) (UTC+1, 11 hours ahead of@dftian
)Jacques Erasmus (
@jerasmus
) (UTC+1, 11 hours ahead of@dftian
)To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Danger- Resolved by 🤖 GitLab Bot 🤖
Proper labels assigned to this merge request. Please ignore me.
@dftian - please see the following guidance and update this merge request.1 Error Please add typebug typefeature, or typemaintenance label to this merge request. Edited by 🤖 GitLab Bot 🤖
Bundle size analysis [beta]
This compares changes in bundle size for entry points between the commits 6ca48463 and 8b4a6d04
Special assetsEntrypoint / Name Size before Size after Diff Diff in percent average 3.45 MB 3.46 MB - 0.0 % mainChunk 1.86 MB 1.86 MB - 0.0 %
Note: We do not have exact data for 6ca48463. So we have used data from: f257619d.
The target commit was too new, so we used the latest commit from master we have info on.
It might help to rerun thebundle-size-review
job
This might mean that you have a few false positives in this report. If something unrelated to your code changes is reported, you can check this comparison in order to see if they caused this change.Please look at the full report for more details
Read more about how this report works.
Generated by
DangerAllure report
allure-report-publisher
generated test report!e2e-review-qa:
test report for af0b64cdexpand test summary
+-----------------------------------------------------------------------------------------+ | suites summary | +------------------------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------------------------+--------+--------+---------+-------+-------+--------+ | Manage | 39 | 0 | 4 | 2 | 43 | ❗ | | Create | 28 | 0 | 1 | 0 | 29 | ✅ | | Plan | 49 | 0 | 1 | 0 | 50 | ✅ | | Verify | 12 | 0 | 1 | 0 | 13 | ✅ | | Govern | 13 | 0 | 5 | 1 | 18 | ❗ | | Version sanity check | 0 | 0 | 1 | 0 | 1 | ➖ | | Feature flag handler sanity checks | 9 | 0 | 0 | 0 | 9 | ✅ | | Configure | 0 | 0 | 1 | 0 | 1 | ➖ | | Package | 0 | 0 | 1 | 0 | 1 | ➖ | +------------------------------------+--------+--------+---------+-------+-------+--------+ | Total | 150 | 0 | 15 | 3 | 165 | ❗ | +------------------------------------+--------+--------+---------+-------+-------+--------+
e2e-package-and-test:
test report for edb981deexpand test summary
+---------------------------------------------------------------------------+ | suites summary | +----------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +----------------------+--------+--------+---------+-------+-------+--------+ | Manage | 127 | 3 | 10 | 0 | 140 | ❌ | | Create | 318 | 0 | 10 | 0 | 328 | ✅ | | Plan | 120 | 0 | 0 | 0 | 120 | ✅ | | Verify | 94 | 0 | 16 | 6 | 110 | ❗ | | Fulfillment | 4 | 0 | 28 | 0 | 32 | ✅ | | Govern | 83 | 1 | 0 | 0 | 84 | ❌ | | Secure | 14 | 0 | 2 | 2 | 16 | ❗ | | Configure | 0 | 0 | 6 | 0 | 6 | ➖ | | Monitor | 2 | 0 | 0 | 0 | 2 | ✅ | | Version sanity check | 0 | 0 | 2 | 2 | 2 | ➖ | | Package | 0 | 0 | 6 | 0 | 6 | ➖ | | Analytics | 4 | 0 | 0 | 0 | 4 | ✅ | | Release | 12 | 0 | 0 | 0 | 12 | ✅ | | ModelOps | 0 | 0 | 2 | 0 | 2 | ➖ | +----------------------+--------+--------+---------+-------+-------+--------+ | Total | 778 | 4 | 82 | 10 | 864 | ❌ | +----------------------+--------+--------+---------+-------+-------+--------+
Setting label groupthreat insights based on
@dftian
's group.added groupthreat insights label
added devopsgovern sectionsec labels
marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed
67 :is-checked="!selected.length" 68 :text="$options.i18n.allItemsText" 69 :data-testid="$options.ALL_ID" 70 @click="deselectAll" 71 /> 72 <filter-item 73 v-for="{ id, text } in $options.DROPDOWN_OPTIONS" 74 :key="id" 75 :data-testid="id" 76 :is-checked="selected.includes(id)" 77 :text="text" 78 @click="toggleSelected(id)" 79 /> 80 </gl-dropdown> 81 </div> 36 <simple-dropdown 1 <script> simple_dropdown.vue
is for filters that have "simple" behavior (most of our filters), where simple means that the filter:- Renders a flat list of dropdown options with an "All Items" option at the top.
- Allows multiple items to be selected except for "All Items", which deselects everything.
- Uses the dropdown option IDs as the data to be emitted for the
filter-changed
event. - Saves and restores to a querystring value.
This will be used by the following filters:
StatusFilter
SeverityFilter
ToolFilter
ToolFilterDeprecated
ClusterFilter
ImageFilter
Edited by Daniel Tian
20 type: Array, 21 required: true, 22 }, 23 querystringKey: { 24 type: String, 25 required: false, 26 default: '', 27 }, 28 loading: { 29 type: Boolean, 30 required: false, 31 default: false, 32 }, 33 }, 34 computed: { 35 optionTexts() { - Comment on lines +35 to +41
This will take an array of options
[{ id: 'A', text: 'Option A' }, { id: 'B', text: 'Option B' }, { id: 'C', text: 'Option C' }]
and converts it to:
{ A: 'Option A', B: 'Option B', C: 'Option C', }
It's used for
O(1)
lookups to get the text for each ID inselectedTexts()
and to get thevalidIds
..Edited by Daniel Tian
31 default: false, 32 }, 33 }, 34 computed: { 35 optionTexts() { 36 const idLookup = keyBy(this.options, 'id'); 37 return mapValues(idLookup, 'text'); 38 }, 39 selectedTexts() { 40 const texts = this.selectedIds.map((id) => this.optionTexts[id]); 41 return texts.length ? texts : [this.optionTexts[ALL_ID]]; 42 }, 43 shouldShowQuerystringSync() { 44 // Don't show QuerystringSync if we're still loading the dropdown options, otherwise the valid 45 // IDs filtering will always return an empty array because there are no dropdown options yet. 46 return this.querystringKey && !this.loading; 49 methods: { 50 isSelected(id) { 51 return id === ALL_ID ? !this.selectedIds.length : this.selectedIds.includes(id); 52 }, 53 toggleSelected(id) { 54 if (id === ALL_ID) { 55 this.emitSelectedIds([]); 56 } else { 57 // Remove ALL_ID if it's selected before toggling the new ID. 58 const selectedExcludingAllId = without(this.selectedIds, ALL_ID); 59 const newIds = xor(selectedExcludingAllId, [id]); 60 this.emitSelectedIds(newIds); 61 } 62 }, 63 emitSelectedIds(ids) { 64 const validIds = ids.filter((id) => Boolean(this.optionTexts[id])); changed this line in version 5 of the diff
added 253 commits
-
6e813df5...1d2a84c6 - 251 commits from branch
master
- 0f60af58 - Remove valid IDs prop for QuerystringSync component
- 8713206f - Add common vulnerability filter components
-
6e813df5...1d2a84c6 - 251 commits from branch
requested review from @sming-gitlab
added 26 commits
- 75651ff1 - Fix Exec Dashboard time period for 3 months ago
- 222a9928 - Refactor billable user finder for SSOT
- a7574cb7 - Use mock time periods
- 341f931c - Include detached partition tables in truncate_legacy_tables
- fa888e4f - Update gitaly to 434051eb74b
- 2fb26162 - Lock writes to detached partitions as well
- 79a65037 - Fix truncating a table in another schema
- 83414c54 - It turns out FK for partitions are in public
- c006c39d - Fix test failing in PG 11
- 90772c82 - Fix RSpec/MultipleMemoizedHelpers offense
- 792031b7 - Revert "Merge branch 'version-check-reactive-cache-no-nil-value' into 'master'"
- b458c647 - Merge branch 'include_detached_partitions_in_truncate_legacy_tables' into 'master'
- 76e1d697 - Fix Layout/FirstArrayElementIndentatiion offenses
- 81fbb941 - Merge branch '239356-first-array-element-endentation' into 'master'
- 637c3a3b - Merge branch '383264-refactor-billable-into-finder' into 'master'
- 5553fd43 - Fix redirect when cancel GitHub OAuth
- 6ca48463 - Merge branch 'tz-345743-redirect-when-cancel-github-import' into 'master'
- 65cfcab5 - Merge branch 'fix-exec-dashboard-time-period' into 'master'
- 1e558d81 - Set min and max width on work item milestone widget
- 12f870bd - Merge branch '381407-max-min-width-milestone' into 'master'
- 9c8922c5 - Merge branch 'release-tools/update-gitaly' into 'master'
- d36e2a27 - Updating owners for feature flags covered by Workspace team
- b2cc0319 - Merge branch 'adil.farrukh-master-patch-47790' into 'master'
- d1332794 - Merge branch 'revert-104302' into 'master'
- 05279825 - Remove valid IDs prop for QuerystringSync component
- edb981de - Add common vulnerability filter components
Toggle commit list- A deleted user
added backend database databasereview pending feature flag typebug labels
removed review request for @sming-gitlab
added 65 commits
-
e908533e...2a3fd3c2 - 63 commits from branch
383346-remove-valid-ids-check
- 15d2eca0 - Add filter dropdown component and change activity filter to use it
- 1733d9dc - Add common vulnerability filter components
-
e908533e...2a3fd3c2 - 63 commits from branch
added maintenancerefactor label
added typemaintenance label and removed typebug label
removed databasereview pending label
@sming-gitlab Would you be able to review this one?
@dftian I'm a bit hesitant about this refactoring change
It feels like we just went in a circle here. I thought the initial intention of our epic was to decouple these filter components from such a restricted shared component, making them standalone so it's easier to extend and make customization too.We knew there would be some duplication but for the benefit of having more flexibility and less fragile components of one change possibly breaking the others, the refactor was worth this expense. But now it feels like we're bringing everything back together
Maybe I'm missing something, would love to hear your thoughts
requested review from @sming-gitlab