Skip to content
Snippets Groups Projects

Add simple dropdown component and change severity filter to use it

5 unresolved threads

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.

Related to #383346 (closed)

Edited by Daniel Tian

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • assigned to @dftian

  • A deleted user added frontend label

    added frontend label

  • 3 Warnings
    :warning: 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.
    :warning: This merge request does not refer to an existing milestone.
    :warning: Most of the time, merge requests should target master. Otherwise, please set the relevant Pick into X.Y label.
    1 Message
    :book: 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:

    1. Ensure the merge request has database and databasereview pending labels. If the merge request modifies database files, Danger will do this for you.
    2. Prepare your MR for database review according to the docs.
    3. 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 current availability (@jdrpereira) (UTC+0, 10 hours ahead of @dftian) Steve Abrams current availability (@sabrams) (UTC-7, 3 hours ahead of @dftian)
    frontend Tomáš Bulva current availability (@tbulva) (UTC+1, 11 hours ahead of @dftian) Jacques Erasmus current availability (@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 :repeat: danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

  • Bundle size analysis [beta]

    This compares changes in bundle size for entry points between the commits 6ca48463 and 8b4a6d04

    :sparkles: Special assets

    Entrypoint / 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 the bundle-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 :no_entry_sign: Danger

  • Allure report

    allure-report-publisher generated test report!

    e2e-review-qa: :exclamation: test report for af0b64cd

    expand 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: :x: test report for edb981de

    expand 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   | ❌     |
    +----------------------+--------+--------+---------+-------+-------+--------+
  • Daniel Tian added 1 commit

    added 1 commit

    • 858d3b04 - Add common vulnerability filter components

    Compare with previous version

  • Setting label groupthreat insights based on @dftian's group.

  • Daniel Tian added 1 commit

    added 1 commit

    • 0d43e308 - Add common vulnerability filter components

    Compare with previous version

  • Daniel Tian marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

    marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

  • Daniel Tian
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
  • Author Developer

    SeverityFilter has now been reduced down to just a SimpleDropdown. It passes down the dropdown options to render and will emit the filter-changed event when the selected IDs change, but everything else is handled by SimpleDropdown.

  • Please register or sign in to reply
  • Daniel Tian
  • 1 <script>
    • Author Developer

      simple_dropdown.vue is for filters that have "simple" behavior (most of our filters), where simple means that the filter:

      1. Renders a flat list of dropdown options with an "All Items" option at the top.
      2. Allows multiple items to be selected except for "All Items", which deselects everything.
      3. Uses the dropdown option IDs as the data to be emitted for the filter-changed event.
      4. 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
    • Please register or sign in to reply
  • 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
      Author Developer

      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 in selectedTexts() and to get the validIds..

      Edited by Daniel Tian
    • Please register or sign in to reply
  • 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;
    • Author Developer

      The loading check is to support ClusterFilter and ImageFilter, which uses GraphQL to load the dropdown options rather than having them available immediately.

    • Please register or sign in to reply
  • 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]));
  • Daniel Tian added 1 commit

    added 1 commit

    • 8b4a6d04 - Add common vulnerability filter components

    Compare with previous version

  • Daniel Tian added 1 commit

    added 1 commit

    • 6e813df5 - Add common vulnerability filter components

    Compare with previous version

  • Daniel Tian added 253 commits

    added 253 commits

    Compare with previous version

  • Daniel Tian changed target branch from master to 383346-remove-valid-ids-check

    changed target branch from master to 383346-remove-valid-ids-check

  • Daniel Tian added 1 commit

    added 1 commit

    • ae38b53d - Add common vulnerability filter components

    Compare with previous version

  • Daniel Tian requested review from @sming-gitlab

    requested review from @sming-gitlab

  • Daniel Tian added 26 commits

    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

    Compare with previous version

  • Daniel Tian removed review request for @sming-gitlab

    removed review request for @sming-gitlab

  • Daniel Tian added 2 commits

    added 2 commits

    • 0f60af58 - 1 commit from branch 383346-remove-valid-ids-check
    • e908533e - Add common vulnerability filter components

    Compare with previous version

  • Daniel Tian added 65 commits

    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

    Compare with previous version

  • Daniel Tian added 1 commit

    added 1 commit

    • af0b64cd - Add common vulnerability filter components

    Compare with previous version

  • Daniel Tian changed title from Add common vulnerability filter components to Add simple dropdown component and change severity filter to use it

    changed title from Add common vulnerability filter components to Add simple dropdown component and change severity filter to use it

  • Daniel Tian changed the description

    changed the description

  • Daniel Tian changed target branch from 383346-remove-valid-ids-check to 383346-add-filter-dropdown-component

    changed target branch from 383346-remove-valid-ids-check to 383346-add-filter-dropdown-component

  • added typemaintenance label and removed typebug label

    • Author Developer

      @sming-gitlab Would you be able to review this one? :bow:

    • @dftian I'm a bit hesitant about this refactoring change :persevere: 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 :thinking: Maybe I'm missing something, would love to hear your thoughts :grinning:

    • Please register or sign in to reply
  • Daniel Tian requested review from @sming-gitlab

    requested review from @sming-gitlab

  • closed

  • Please register or sign in to reply
    Loading