Add Projects (Budgets) to filtered search #11740

greenwoodt · 2023-10-11T10:16:48Z

🎩 What? Why?

Upon searching through modules as a user, Projects under Budgets were not searchable or available.

📌 Related Issues

Fixes Projects not displayed in search filters #11719

Testing

Head to Decidim
Search by typing a Project name (or just by pressing 'Enter' while in the search)
See the searchable Projects in filters on the left.

RSPEC test added to decidim-core/spec/system/search_spec.rb also.

📷 Screenshots

Before:

After:

♥️ Thank you!

greenwoodt · 2023-10-11T10:47:43Z

I think you were right about the tests @andreslucena.

I'm having a discussion with @carolromero on what icon to replace Projects with to which we have concluded it would be the same as Budgets "coin-line"in the CONST located in: decidim-core/app/helpers/decidim/icon_helper.rb.

greenwoodt · 2023-10-13T10:00:18Z

Seems to a recurring theme in the test failures. Possibly to do with seeds..

andreslucena · 2023-10-13T10:56:13Z

Seems to a recurring theme in the test failures. Possibly to do with seeds..

I think it's related to this association:

https://github.com/greenwoodt/decidim/blob/85e53e4b491066c2e12a79c46c343fc4c70c73fa/decidim-budgets/app/models/decidim/budgets/project.rb#L26-L27

And this other ActiveRecord method: https://github.com/greenwoodt/decidim/blob/85e53e4b491066c2e12a79c46c343fc4c70c73fa/decidim-core/lib/decidim/has_component.rb#L12-L13

That we're including in the same class. As far as I remember, the problem is that we usually have a simpler relation between the Components and its resources, (like you have a set of Meetings in this Component, and there are Proposals in this other Component). The issue comes that in this case, the relation between the Component and the Projects is done through the Budgets, and adding this concern like that is breaking the Projects creation.

You check it out by creating a new Project in the Admin panel. I see the same exception as the specs.

I think a better approach could be to actually change the code that registers these models in the Search. Let's try this:

Remove the new line from the Project model, including the HasComponent concern
Change the searchable.rb check for ancestors from HasComponent to Searchable, as that's the other concern that all the Resources have in common and semantically sounds right

As we're changing code from lib/, to see that it works you'll need to stop and start again the bin/dev script

This is what I have in mind:

diff --git a/decidim-budgets/app/models/decidim/budgets/project.rb b/decidim-budgets/app/models/decidim/budgets/project.rb
index 545c632cff..508c73bc49 100644
--- a/decidim-budgets/app/models/decidim/budgets/project.rb
+++ b/decidim-budgets/app/models/decidim/budgets/project.rb
@@ -19,7 +19,6 @@ module Decidim
       include Decidim::Searchable
       include Decidim::TranslatableResource
       include Decidim::FilterableResource
-      include Decidim::HasComponent
 
       translatable_fields :title, :description
 
diff --git a/decidim-core/lib/decidim/searchable.rb b/decidim-core/lib/decidim/searchable.rb
index 6da2b23a94..fa132ffbc1 100644
--- a/decidim-core/lib/decidim/searchable.rb
+++ b/decidim-core/lib/decidim/searchable.rb
@@ -35,7 +35,7 @@ module Decidim
     end
 
     def self.searchable_resources_of_type_component
-      searchable_resources.select { |r| r.constantize.ancestors.include?(Decidim::HasComponent) }
+      searchable_resources.select { |r| r.constantize.ancestors.include?(Decidim::Searchable) }
     end
 
     def self.searchable_resources_of_type_comment

Try it out locally and if you see that it works then commit/push and wait for the specs 🤞🏽

greenwoodt · 2023-10-13T11:28:08Z

Seems to a recurring theme in the test failures. Possibly to do with seeds..

I think it's related to this association:

https://github.com/greenwoodt/decidim/blob/85e53e4b491066c2e12a79c46c343fc4c70c73fa/decidim-budgets/app/models/decidim/budgets/project.rb#L26-L27

And this other ActiveRecord method: https://github.com/greenwoodt/decidim/blob/85e53e4b491066c2e12a79c46c343fc4c70c73fa/decidim-core/lib/decidim/has_component.rb#L12-L13

That we're including in the same class. As far as I remember, the problem is that we usually have a simpler relation between the Components and its resources, (like you have a set of Meetings in this Component, and there are Proposals in this other Component). The issue comes that in this case, the relation between the Component and the Projects is done through the Budgets, and adding this concern like that is breaking the Projects creation.

You check it out by creating a new Project in the Admin panel. I see the same exception as the specs.

I think a better approach could be to actually change the code that registers these models in the Search. Let's try this:
1. Remove the new line from the Project model, including the HasComponent concern

2. Change the searchable.rb check for ancestors from HasComponent to Searchable, as that's the other concern that all the Resources have in common and semantically sounds right
As we're changing code from lib/, to see that it works you'll need to stop and start again the bin/dev script

This is what I have in mind:
diff --git a/decidim-budgets/app/models/decidim/budgets/project.rb b/decidim-budgets/app/models/decidim/budgets/project.rb
index 545c632cff..508c73bc49 100644
--- a/decidim-budgets/app/models/decidim/budgets/project.rb
+++ b/decidim-budgets/app/models/decidim/budgets/project.rb
@@ -19,7 +19,6 @@ module Decidim
       include Decidim::Searchable
       include Decidim::TranslatableResource
       include Decidim::FilterableResource
-      include Decidim::HasComponent
 
       translatable_fields :title, :description
 
diff --git a/decidim-core/lib/decidim/searchable.rb b/decidim-core/lib/decidim/searchable.rb
index 6da2b23a94..fa132ffbc1 100644
--- a/decidim-core/lib/decidim/searchable.rb
+++ b/decidim-core/lib/decidim/searchable.rb
@@ -35,7 +35,7 @@ module Decidim
     end
 
     def self.searchable_resources_of_type_component
-      searchable_resources.select { |r| r.constantize.ancestors.include?(Decidim::HasComponent) }
+      searchable_resources.select { |r| r.constantize.ancestors.include?(Decidim::Searchable) }
     end
 
     def self.searchable_resources_of_type_comment
Try it out locally and if you see that it works then commit/push and wait for the specs 🤞🏽

Thanks Andres I'll give it a go!

…idim core

greenwoodt · 2023-10-13T12:25:51Z

@andreslucena looks like your suggestion worked! Thanks very much! I will now open this for a review by the maintainers!

andreslucena · 2023-10-13T12:35:14Z

@andreslucena looks like your suggestion worked! Thanks very much! I will now open this for a review by the maintainers!

Glad to hear that! Lets add an spec here, I think this would be enough:

diff --git a/decidim-core/spec/system/search_spec.rb b/decidim-core/spec/system/search_spec.rb
index 9ccb45e386..8bffc83eca 100644
--- a/decidim-core/spec/system/search_spec.rb
+++ b/decidim-core/spec/system/search_spec.rb
@@ -32,6 +32,13 @@ describe "Search", type: :system do
       expect(page).to have_content(/results for the search: "#{term}"/i)
       expect(page).to have_selector(".filter-search.filter-container")
     end
+
+    it "has all the resources to search" do
+      within ".search__filter" do
+        expect(page).to have_content("All")
+        expect(page).to have_content("Participants")
+        # TODO: all the resources
+    end
   end
 
   context "when the device is a mobile" do

You need to add the list of all the Searchable models, of course. Just having this list would be enough IMO.

…oodt/decidim into fix/projects-displayed-in-filters

greenwoodt · 2023-10-13T12:51:41Z

@andreslucena looks like your suggestion worked! Thanks very much! I will now open this for a review by the maintainers!

Glad to hear that! Lets add an spec here, I think this would be enough:

diff --git a/decidim-core/spec/system/search_spec.rb b/decidim-core/spec/system/search_spec.rb
index 9ccb45e386..8bffc83eca 100644
--- a/decidim-core/spec/system/search_spec.rb
+++ b/decidim-core/spec/system/search_spec.rb
@@ -32,6 +32,13 @@ describe "Search", type: :system do
       expect(page).to have_content(/results for the search: "#{term}"/i)
       expect(page).to have_selector(".filter-search.filter-container")
     end
+
+    it "has all the resources to search" do
+      within ".search__filter" do
+        expect(page).to have_content("All")
+        expect(page).to have_content("Participants")
+        # TODO: all the resources
+    end
   end
 
   context "when the device is a mobile" do

You need to add the list of all the Searchable models, of course. Just having this list would be enough IMO.

Done!

andreslucena

A change in the spec and this would be good to me

decidim-core/spec/system/search_spec.rb

andreslucena

LGTM!

I'll not merge until @alecslupu checks this out, as I was involved in the code

alecslupu

Sweet !
LGTM

* develop: (30 commits) Add `process-content` to erb-lint's deprecated classes (#11762) Add possibility of overriding the tailwind.config.js (#11763) Ask old password when changing email or password (#11737) Add Projects (Budgets) to filtered search (#11740) Fix missing results on Geocoded when search without diacritics (#11761) Add robots.txt instructions (#11693) Add missing activerecord budget locales for search (#11766) Improve design of Admin's Sidebar pages (#11759) Show small static map on admin's meetings index with big screens (#11715) Remove "Manage" button when there's a Sidebar (#11717) Fix admin breadcrumb in Process (#11757) Apply new rubocop rules on erb - Layout/MultilineMethodCallIndentation (#11756) Remove xlarge-* references from admin forms (#11712) Apply new rubocop rules on erb - Argument identation (#11707) Update HERE API autocomplete (#11507) Admin redesign proposal issues (#11668) Redesign: responsive links on cards (#11538) Refactor CI pipelines (#11196) Update postcss and graphql to latest versions (#11733) Fix develop pipeline (#11750) ...

Model change to projects

7fce7d4

greenwoodt added type: bug Issues that describe a bug module: core labels Oct 11, 2023

greenwoodt requested review from andreslucena and carolromero October 11, 2023 10:16

greenwoodt self-assigned this Oct 11, 2023

Icon change for projects in filters

153bd12

andreslucena changed the title ~~Adding Projects (Budgets) to filtered search~~ Add Projects (Budgets) to filtered search Oct 13, 2023

greenwoodt and others added 2 commits October 13, 2023 13:36

Suggested changes: Changing the concern to alerting the method in dec…

65c2985

…idim core

Merge branch 'decidim:develop' into fix/projects-displayed-in-filters

eee9987

greenwoodt requested review from a team and removed request for carolromero October 13, 2023 12:26

greenwoodt marked this pull request as ready for review October 13, 2023 12:26

greenwoodt added 2 commits October 13, 2023 14:49

Spec test added to test change

a1fad07

Merge branch 'fix/projects-displayed-in-filters' of github.com:greenw…

a039b41

…oodt/decidim into fix/projects-displayed-in-filters

Fixes to rspec test in search_spec.rb

76c1951

andreslucena requested changes Oct 14, 2023

View reviewed changes

decidim-core/spec/system/search_spec.rb Outdated Show resolved Hide resolved

added all resource content expection in rspec test for search_spec

4ddd4b7

greenwoodt requested a review from andreslucena October 16, 2023 09:24

andreslucena approved these changes Oct 16, 2023

View reviewed changes

alecslupu approved these changes Oct 16, 2023

View reviewed changes

alecslupu merged commit bc75cd9 into decidim:develop Oct 16, 2023
102 checks passed

andreslucena added type: fix PRs that implement a fix for a bug and removed type: bug Issues that describe a bug labels Nov 29, 2023

github-actions bot approved these changes Nov 29, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Projects (Budgets) to filtered search #11740

Add Projects (Budgets) to filtered search #11740

greenwoodt commented Oct 11, 2023 •

edited

Loading

greenwoodt commented Oct 11, 2023

greenwoodt commented Oct 13, 2023

andreslucena commented Oct 13, 2023

greenwoodt commented Oct 13, 2023

greenwoodt commented Oct 13, 2023

andreslucena commented Oct 13, 2023

greenwoodt commented Oct 13, 2023

andreslucena left a comment

andreslucena left a comment

alecslupu left a comment

Add Projects (Budgets) to filtered search #11740

Add Projects (Budgets) to filtered search #11740

Conversation

greenwoodt commented Oct 11, 2023 • edited Loading

🎩 What? Why?

📌 Related Issues

Testing

📷 Screenshots

greenwoodt commented Oct 11, 2023

greenwoodt commented Oct 13, 2023

andreslucena commented Oct 13, 2023

greenwoodt commented Oct 13, 2023

greenwoodt commented Oct 13, 2023

andreslucena commented Oct 13, 2023

greenwoodt commented Oct 13, 2023

andreslucena left a comment

Choose a reason for hiding this comment

andreslucena left a comment

Choose a reason for hiding this comment

alecslupu left a comment

Choose a reason for hiding this comment

greenwoodt commented Oct 11, 2023 •

edited

Loading