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

Remove duplicate filters in BaseQuery#where #15281

Merged
merged 3 commits into from
Apr 18, 2024
Merged

Conversation

oliverguenther
Copy link
Member

#15162 (comment)

It was not what I assumed, but the new TimeFilter did only set a value for the filters and thus it had an empty value in the select. It works now and also uses the proper translation for the displayed values.

The issue that the number was to high is that we are adding the project filter twice:

    query = ParamsToQueryService.new(Meeting, current_user).call(params)
    # ↳ When there is a project_id filter in the URL this is added as a filter to the query

    query = apply_default_filter_if_none_given(query)

    if @project
      query.where("project_id", '=', @project.id)
      # ↳ This adds the filter regardless and we end up with the ProjectFilter twice in the list
    end

I have added a workaround in 222fef2 but I'm not sure if we should rather fix the underlying problem.

@klaustopher
Copy link
Contributor

LGTM

@klaustopher
Copy link
Contributor

With this removed we can also remove the hack in

def filters_count
@filters_count ||= query
.filters
.map(&:class)
.uniq
.count
end

@oliverguenther oliverguenther marked this pull request as ready for review April 18, 2024 07:44
Copy link
Contributor

@klaustopher klaustopher left a comment

Choose a reason for hiding this comment

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

That was an easy fix

@klaustopher klaustopher merged commit ba4ddbb into dev Apr 18, 2024
8 of 9 checks passed
@klaustopher klaustopher deleted the fix/duplicate-filters branch April 18, 2024 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants