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.

Sign up for GitHub

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

Jump to bottom

Allow filtering by blank items. #2016

Merged
merged 1 commit into from
Aug 30, 2024
Merged

Allow filtering by blank items. #2016

merged 1 commit into from
Aug 30, 2024

Conversation

caiosba
Copy link
Contributor

@caiosba caiosba commented Aug 30, 2024

Description

Allow filtering by blank items (reverting previous behavior).

Fixes CV2-5156.

How has this been tested?

Existing tests should cover this.

Checklist

  • I have performed a self-review of my own code
  • I have added unit and feature tests, if the PR implements a new feature or otherwise would benefit from additional testing
  • I have added regression tests, if the PR fixes a bug
  • I have added logging, exception reporting, and custom tracing with any additional information required for debugging
  • I considered secure coding practices when writing this code. Any security concerns are noted above.
  • I have commented my code in hard-to-understand areas, if any
  • I have made needed changes to the README
  • My changes generate no new warnings
  • If I added a third party module, I included a rationale for doing so and followed our current guidelines

melsawy
melsawy requested changes Aug 30, 2024
View reviewed changes
lib/check_search.rb
@@ -172,7 +172,7 @@ def should_hit_elasticsearch?
end

def media_types_filter
MEDIA_TYPES & @options['show']
[MEDIA_TYPES, 'blank'].flatten & @options['show']
Copy link
Contributor

Choose a reason for hiding this comment

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

I expected in this case to add blank to MEDIA_TYPES array why not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@melsawy we want to allow to filter by blank but not include it as a default media type, as a way to keep moving towards deprecating it.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK @caiosba

@caiosba caiosba requested a review from melsawy August 30, 2024 14:58
@caiosba caiosba merged commit 326cccd into develop Aug 30, 2024
10 checks passed
@caiosba caiosba deleted the fix/CV2-5156-filter-by-blank branch August 30, 2024 15:19
caiosba added a commit that referenced this pull request Aug 30, 2024
@caiosba
Allow filtering by blank items. (#2016)
c61e73e 
Fixes CV2-5156.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melsawy melsawy melsawy approved these changes

@DGaffney DGaffney Awaiting requested review from DGaffney DGaffney is a code owner

@jayjay-w jayjay-w Awaiting requested review from jayjay-w jayjay-w is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@caiosba @melsawy