Skip to content

Conversation

@MSD118
Copy link

@MSD118 MSD118 commented Jan 27, 2026

Improvements to case-insensitivity in filtering and searching:

  • Made the search filter in search_filter compare alias names in a case-insensitive way by downcasing both the search fields and the column map alias names (lib/query_helper.rb).
  • Updated the filter creation logic in create_filters to perform case-insensitive matching when finding column maps by alias name (lib/query_helper/sql_filter.rb).

Copilot AI review requested due to automatic review settings January 27, 2026 05:01
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds case-insensitive matching for search fields and filter alias names to improve the flexibility of the query helper. Previously, alias name matching was case-sensitive, which could cause issues when users provided filter or search field names with different casing than defined in the column maps.

Changes:

  • Made filter alias matching case-insensitive in create_filters method
  • Made search field matching case-insensitive in search_filter method

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
lib/query_helper/sql_filter.rb Added case-insensitive comparison when finding column maps by alias name in filter creation
lib/query_helper.rb Added case-insensitive comparison when matching search fields to column map aliases

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@filter_values.each do |comparate_alias, criteria|
# Find the sql mapping if it exists
map = @column_maps.find { |m| m.alias_name == comparate_alias }
map = @column_maps.find { |m| m.alias_name.downcase == comparate_alias.downcase }
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

The case-insensitive comparison approach here is inconsistent with the codebase. The sql_sort.rb file uses the casecmp? method for case-insensitive alias matching (line 37 in sql_sort.rb), which is the idiomatic Ruby approach. Using casecmp? would be more consistent, more readable, and doesn't create intermediate string objects. Consider changing this to: map = @column_maps.find { |m| m.alias_name.casecmp?(comparate_alias) }

Suggested change
map = @column_maps.find { |m| m.alias_name.downcase == comparate_alias.downcase }
map = @column_maps.find { |m| m.alias_name.casecmp?(comparate_alias) }

Copilot uses AI. Check for mistakes.
placement = :where
maps = column_maps.select do |cm|
if @search_fields.include? cm.alias_name
if @search_fields.map(&:downcase).include? cm.alias_name.downcase
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

This implementation has a performance issue: the @search_fields.map(&:downcase) array is created on every iteration of the select block, once for each column_map. This should be computed once before the select block. Additionally, the case-insensitive comparison approach is inconsistent with the codebase - sql_sort.rb uses casecmp? for alias matching (line 37 in sql_sort.rb), which is the idiomatic Ruby approach. Consider refactoring to: downcased_search_fields = @search_fields.map(&:downcase) before the select, then use downcased_search_fields.include?(cm.alias_name.downcase), or better yet, use @search_fields.any? { |sf| sf.casecmp?(cm.alias_name) } for consistency with sql_sort.rb.

Suggested change
if @search_fields.map(&:downcase).include? cm.alias_name.downcase
if @search_fields.any? { |sf| sf.casecmp?(cm.alias_name) }

Copilot uses AI. Check for mistakes.
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.

2 participants