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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix filter by scope on search page #12036

Merged
merged 1 commit into from Nov 22, 2023
Merged

Conversation

fblupi
Copy link
Member

@fblupi fblupi commented Nov 20, 2023

馃帺 What? Why?

Fix the error raised when filtering by scope on the search page. The scope filter has been removed on the redesign, that's why this PR targets the release/0.27-stable branch instead of develop.

This error raises because we are using the ransack _eq filter instead of the _in using an array as input. In the front end, we use the scopes_picker_filter helper to render the scope multi-picker, and it produces an array argument instead of a single value. That's why I have changed the ransack filter in this PR.

IMO, we should remove the references to the scope filter in the search controller and command in the develop branch, as we are no longer using it since the redesign was merged.

馃搶 Related Issues

Testing

  1. Go to the search bar and find by text
  2. Scroll down and filter by scope
  3. See it filters by scope

馃摲 Screenshots

Screen.Recording.2023-11-20.at.12.29.00.mov

鈾ワ笍 Thank you!

@fblupi fblupi added module: core type: fix PRs that implement a fix for a bug labels Nov 20, 2023
@fblupi fblupi requested a review from a team November 20, 2023 12:59
@andreslucena andreslucena self-assigned this Nov 22, 2023
@andreslucena
Copy link
Member

IMO, we should remove the references to the scope filter in the search controller and command in the develop branch, as we are no longer using it since the redesign was merged.

I'm not 100% sure that this was indeed a decision made by @decidim/product or just limitations in the redesign development bandwith (or just an oversights). @decidim/product do you want to drop this feature ("filtering by scopes /search")? Or is it better to just remove the dead code?

Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

Tried it out locally and it works as expected. Can you also add a system spec that would have detected this bug? Thanks

Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

馃憤馃徑

@andreslucena andreslucena added the no-backport Pull Requests that should not be backported label Nov 22, 2023
@andreslucena
Copy link
Member

Adding the no-backport label as this change isn't relevant for v0.26: this was introduced by the big ransack change of v0.27. (Let me know if you think that that's not the case @fblupi)

@andreslucena
Copy link
Member

Only failing check is codecov, so I'm merging this

@andreslucena andreslucena merged commit a7cc641 into release/0.27-stable Nov 22, 2023
45 of 46 checks passed
@andreslucena andreslucena deleted the fix/12023 branch November 22, 2023 16:04
@fblupi
Copy link
Member Author

fblupi commented Nov 22, 2023

Adding the no-backport label as this change isn't relevant for v0.26: this was introduced by the big ransack change of v0.27. (Let me know if you think that that's not the case @fblupi)

That's right. This error appeared with the searchlight to ransack migration.

@andreslucena
Copy link
Member

IMO, we should remove the references to the scope filter in the search controller and command in the develop branch, as we are no longer using it since the redesign was merged.

I'm not 100% sure that this was indeed a decision made by @decidim/product or just limitations in the redesign development bandwith (or just an oversights). @decidim/product do you want to drop this feature ("filtering by scopes /search")? Or is it better to just remove the dead code?

@fblupi we've discussed in the last @decidim/product and we agreed that for the time being this should be removed as we're not planning in introducing that back. Can you do the PR 馃檹馃徑 ? I don't find the references to scopes in search related classes, so you have more in your head what needs to be changed exactly. Thanks!!

@fblupi
Copy link
Member Author

fblupi commented Nov 27, 2023

@andreslucena I did the PR at the same time I did this one, and it was already approved and merged by @alecslupu here: #12038

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: core no-backport Pull Requests that should not be backported type: fix PRs that implement a fix for a bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants