Skip to content
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

Introduce a filter for job groups in blocked page #303

Merged
merged 8 commits into from
Apr 3, 2023

Conversation

michaelgrifalconi
Copy link
Contributor

  • Allow to filter for multiple job groups, separated by ','

Follow-up to continue discussion from #302

- Allow to filter for multiple job groups, separated by ','
okurz
okurz previously requested changes Mar 22, 2023
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

From kraih's review suggestions:

  1. more modern JavaScript style
  2. existing filter field reused if possible
  3. some basic tests at least (same as other filters)
  4. Find a better way than var results = []

assets/vue/components/BlockedIncident.vue Outdated Show resolved Hide resolved
assets/vue/components/BlockedIncident.vue Outdated Show resolved Hide resolved
assets/vue/components/PageBlocked.vue Outdated Show resolved Hide resolved
assets/vue/components/PageBlocked.vue Outdated Show resolved Hide resolved
assets/vue/components/PageBlocked.vue Outdated Show resolved Hide resolved
assets/vue/components/PageBlocked.vue Outdated Show resolved Hide resolved
@kraih
Copy link
Member

kraih commented Mar 22, 2023

Regarding tests, unfortunately we only have the bare minimum for filtering so far. You can watch the test run locally with something like TEST_HEADLESS=0 TEST_ONLINE=postgres:///dashboard node t/ui.t.js. And adjust the slowMo value to slow it down further during debugging.

@michaelgrifalconi
Copy link
Contributor Author

All changes requested should be done and squashed in a commit. Now will try to add some tests

@kraih
Copy link
Member

kraih commented Mar 27, 2023

Is it still a separate search field below the other search field?

@michaelgrifalconi
Copy link
Contributor Author

Is it still a separate search field below the other search field?

@kraih
Yes, since the logic is different:

  • first search for package name/number (partial match is ok)
  • second search is for job group (need exact match, allow multiple job groups separated by comma)

assets/vue/components/BlockedIncident.vue Outdated Show resolved Hide resolved
assets/vue/components/PageBlocked.vue Outdated Show resolved Hide resolved
@kraih
Copy link
Member

kraih commented Mar 27, 2023

@kraih Yes, since the logic is different:

  • first search for package name/number (partial match is ok)
  • second search is for job group (need exact match, allow multiple job groups separated by comma)

Wonder if it would look cleaner if the search fields were each above their respective table column, instead of above one another. 🤔

assets/vue/components/PageBlocked.vue Outdated Show resolved Hide resolved
assets/vue/components/PageBlocked.vue Outdated Show resolved Hide resolved
assets/vue/components/PageBlocked.vue Outdated Show resolved Hide resolved
@kraih kraih dismissed okurz’s stale review April 3, 2023 13:20

Raised points have been addressed.

@mergify mergify bot merged commit c30c704 into openSUSE:main Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants