Add proposal filters #474

beagleknight · 2017-01-10T11:40:00Z

🎩 What? Why?

I'm adding a filter form for proposals. I factor out the code created in #399 so it can be used for both resources.

I'm exposing the API as a single helper:

<%= filter_form_for filter do |form| %>
  ...
<% end %>

The filter form receives a filter object which is created if the controller uses the following concern:

class MyController < ApplicationController
  include FilterResource

  private
  
  def search_klass
    DummySearch # explained above
  end

  def default_search_params
    {} # Params used for search but are not used in the filter form
  end

  def default_filter_params
    {} # Params used in the filter form
  end
end

The controller needs a DummySearch class that will be used to search and filter resources. This class must inherit ResourceSearch base class which includes the default behavior for all search classes.

📌 Related Issues

Implements Filter proposals #376
Created Add origin to proposals #487 because a proposal doesn't have an origin.
Created Wrong check in some capybara tests #491 because some tests are wrong.

📋 Subtasks

📷 Screenshots (optional)

👻 GIF

mention-bot · 2017-01-10T11:40:02Z

@beagleknight, thanks for your PR! By analyzing the history of the files in this pull request, we identified @josepjaume, @oriolgual and @divins to be potential reviewers.

codecov-io · 2017-01-10T13:26:25Z

Current coverage is 60.18% (diff: 94.91%)

Merging #474 into master will increase coverage by 0.05%

@@             master       #474   diff @@
==========================================
  Files          2824       2833     +9   
  Lines        121487     121685   +198   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          73048      73231   +183   
- Misses        48439      48454    +15   
  Partials          0          0

Powered by Codecov. Last update 04f9cbc...27ce8a2

houndci-bot · 2017-01-10T14:47:50Z

decidim-proposals/app/controllers/decidim/proposals/proposals_controller.rb

-        @random_seed = @search.random_seed
+        @search = ProposalSearch.new(search_params.merge(context_params))
+        @proposals = @search.results
+        @random_seed = @search.random_seed        


Trailing whitespace detected.

houndci-bot · 2017-01-10T14:47:51Z

decidim-core/app/services/decidim/resource_search.rb

@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+module Decidim
+  class ResourceSearch < Searchlight::Search


Missing top-level class documentation comment.

houndci-bot · 2017-01-10T15:48:16Z

decidim-proposals/app/controllers/decidim/proposals/proposals_controller.rb

-        @proposals = @search.proposals
-        @random_seed = @search.random_seed
+        @proposals = search.results
+        @random_seed = search.random_seed        


Trailing whitespace detected.

houndci-bot · 2017-01-10T15:48:17Z

decidim-proposals/app/controllers/decidim/proposals/proposals_controller.rb

-        @proposals = @search.proposals
-        @random_seed = @search.random_seed
+        @proposals = search.results
+        @random_seed = search.random_seed        


Trailing whitespace detected.

houndci-bot · 2017-01-10T15:48:17Z

decidim-core/app/controllers/concerns/decidim/filter_resource.rb

+      def filter
+        @filter ||= Filter.new(params[:filter])
+      end
+


Trailing whitespace detected.

houndci-bot · 2017-01-10T15:48:17Z

decidim-core/app/controllers/concerns/decidim/filter_resource.rb

+      def filter
+        @filter ||= Filter.new(params[:filter])
+      end
+


Trailing whitespace detected.

houndci-bot · 2017-01-10T15:48:17Z

decidim-core/app/controllers/concerns/decidim/filter_resource.rb

+      end
+
+      def respond_to_missing?(method_name, include_private = false)
+        @filter.present? && @filter.has_key?(method_name) || super


Use Hash#key? instead of Hash#has_key?.

houndci-bot · 2017-01-10T15:48:17Z

decidim-core/app/controllers/concerns/decidim/filter_resource.rb

+      end
+
+      def respond_to_missing?(method_name, include_private = false)
+        @filter.present? && @filter.has_key?(method_name) || super


Use Hash#key? instead of Hash#has_key?.

houndci-bot · 2017-01-10T15:48:18Z

decidim-core/app/controllers/concerns/decidim/filter_resource.rb

+      end
+
+      def method_missing(method_name, *arguments, &block)
+        @filter.present? && @filter.has_key?(method_name) ? @filter[method_name] : ""


Use Hash#key? instead of Hash#has_key?.

houndci-bot · 2017-01-10T15:48:18Z

decidim-core/app/controllers/concerns/decidim/filter_resource.rb

+      end
+
+      def method_missing(method_name, *arguments, &block)
+        @filter.present? && @filter.has_key?(method_name) ? @filter[method_name] : ""


Use Hash#key? instead of Hash#has_key?.

houndci-bot · 2017-01-10T15:48:18Z

decidim-core/app/controllers/concerns/decidim/filter_resource.rb

+        @filter = filter
+      end
+
+      def method_missing(method_name, *arguments, &block)


When using method_missing, fall back on super.
Unused method argument - arguments. If it's necessary, use _ or _arguments as an argument name to indicate that it won't be used.
Unused method argument - block. If it's necessary, use _ or _block as an argument name to indicate that it won't be used.

houndci-bot · 2017-01-10T15:48:18Z

decidim-core/app/controllers/concerns/decidim/filter_resource.rb

+        @filter = filter
+      end
+
+      def method_missing(method_name, *arguments, &block)


When using method_missing, fall back on super.
Unused method argument - arguments. If it's necessary, use _ or _arguments as an argument name to indicate that it won't be used.
Unused method argument - block. If it's necessary, use _ or _block as an argument name to indicate that it won't be used.

mention-bot · 2017-01-11T09:20:10Z

@beagleknight, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mrcasals, @josepjaume and @oriolgual to be potential reviewers.

houndci-bot · 2017-01-11T11:02:37Z

decidim-core/lib/decidim/filter_form_builder.rb

+      end
+    end
+  end
+end


Final newline missing.

houndci-bot · 2017-01-11T11:02:37Z

decidim-core/lib/decidim/filter_form_builder.rb

+        @template.content_tag(:fieldset) do
+          @template.content_tag(:legend) do
+            @template.content_tag(:h6, legend_title, class: 'heading6')
+          end + block.call


Use yield instead of block.call.

houndci-bot · 2017-01-11T11:02:37Z

decidim-core/lib/decidim/filter_form_builder.rb

+      end
+    end
+  end
+end


Final newline missing.

houndci-bot · 2017-01-11T11:02:37Z

decidim-core/lib/decidim/filter_form_builder.rb

+      @template.content_tag(:div, '', class: "filters__section") do
+        @template.content_tag(:fieldset) do
+          @template.content_tag(:legend) do
+            @template.content_tag(:h6, legend_title, class: 'heading6')


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-01-11T11:02:37Z

decidim-core/lib/decidim/filter_form_builder.rb

+        @template.content_tag(:fieldset) do
+          @template.content_tag(:legend) do
+            @template.content_tag(:h6, legend_title, class: 'heading6')
+          end + block.call


Use yield instead of block.call.

houndci-bot · 2017-01-11T11:02:37Z

decidim-core/lib/decidim/filter_form_builder.rb

+
+    # Private: Renders a custom fieldset and execute the given block.
+    def fieldset_wrapper(legend_title, &block)
+      @template.content_tag(:div, '', class: "filters__section") do


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-01-11T11:02:37Z

decidim-core/lib/decidim/filter_form_builder.rb

+      @template.content_tag(:div, '', class: "filters__section") do
+        @template.content_tag(:fieldset) do
+          @template.content_tag(:legend) do
+            @template.content_tag(:h6, legend_title, class: 'heading6')


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-01-11T11:02:37Z

decidim-core/lib/decidim/filter_form_builder.rb

+      fieldset_wrapper options[:legend_title] do
+        super(method, collection, value_method, label_method, options, html_options) do |b|
+          if block_given?
+            block.call b


Use yield instead of block.call.

houndci-bot · 2017-01-11T11:02:38Z

decidim-core/lib/decidim/filter_form_builder.rb

+
+    # Private: Renders a custom fieldset and execute the given block.
+    def fieldset_wrapper(legend_title, &block)
+      @template.content_tag(:div, '', class: "filters__section") do


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-01-11T11:02:38Z

decidim-core/lib/decidim/filter_form_builder.rb

+
+    # Wrap the check_boxes collection in a custom fieldset.
+    # It also renders the inputs inside its labels.    
+    def collection_check_boxes(method, collection, value_method, label_method, options = {}, html_options = {}, &block)


Avoid parameter lists longer than 5 parameters.

houndci-bot · 2017-01-11T11:02:38Z

decidim-core/lib/decidim/filter_form_builder.rb

+      fieldset_wrapper options[:legend_title] do
+        super(method, collection, value_method, label_method, options, html_options) do |b|
+          if block_given?
+            block.call b


Use yield instead of block.call.

houndci-bot · 2017-01-11T11:02:38Z

decidim-core/lib/decidim/filter_form_builder.rb

+    end
+
+    # Wrap the check_boxes collection in a custom fieldset.
+    # It also renders the inputs inside its labels.    


Trailing whitespace detected.

houndci-bot · 2017-01-11T11:02:38Z

decidim-core/lib/decidim/filter_form_builder.rb

+
+    # Wrap the check_boxes collection in a custom fieldset.
+    # It also renders the inputs inside its labels.    
+    def collection_check_boxes(method, collection, value_method, label_method, options = {}, html_options = {}, &block)


Avoid parameter lists longer than 5 parameters.

houndci-bot · 2017-01-11T11:02:38Z

decidim-core/lib/decidim/filter_form_builder.rb

+      fieldset_wrapper options[:legend_title] do
+        super(method, collection, value_method, label_method, options, html_options) do |b|
+          if block_given?
+            block.call b


Use yield instead of block.call.

mrcasals

Good job!

oriolgual · 2017-01-13T09:23:03Z

decidim-core/app/services/decidim/resource_search.rb

+    # Creates the SearchLight base query.
+    # Check if the option feature was provided.
+    def base_query
+      # raise order_start_time.inspect


lol xD Forgot to erase that line of RDD (Raise-driven development)

oriolgual · 2017-01-13T09:23:53Z

decidim-core/app/services/decidim/resource_search.rb

+    # Initialize the Searchlight::Search base class with the options provided.
+    #
+    # scope - The scope used to create the base query
+    # options - A hash of options to modify the search. These options will be


Can you document the options not used by searchlight? Like the paginate ones and other if there are any

oriolgual · 2017-01-13T09:24:37Z

decidim-core/lib/decidim/filter_form_builder.rb

+      fieldset_wrapper options[:legend_title] do
+        super(method, collection, value_method, label_method, options, html_options) do |b|
+          if block_given?
+            yield b


OriolCI: Avoid variables with meaningless names

oriolgual · 2017-01-13T09:24:42Z

decidim-core/lib/decidim/filter_form_builder.rb

+      fieldset_wrapper options[:legend_title] do
+        super(method, collection, value_method, label_method, options, html_options) do |b|
+          if block_given?
+            yield b


oriolgual · 2017-01-13T09:25:12Z

decidim-core/spec/helpers/decidim/filters_helper_spec.rb

+        allow(helper).to receive(:dummies_path)
+      end
+
+      it "should wrap everything in a div with class 'filters'" do


Ok but can you explain why you don't like 'should'?

oriolgual · 2017-01-13T09:25:18Z

decidim-core/spec/helpers/decidim/filters_helper_spec.rb

+        end
+      end
+
+      it "should call form_for helper with specific arguments" do


oriolgual · 2017-01-13T09:25:34Z

decidim-core/spec/lib/filter_form_builder_spec.rb

+    let(:builder) { FilterFormBuilder.new(:resource, resource, helper, {}) }
+
+    shared_examples "fieldset_wrapper" do
+      it "should wrap fields in a fieldset inside a div with class 'filters__section'" do


remove should everywhere xD

oriolgual · 2017-01-13T09:27:48Z

decidim-proposals/app/controllers/decidim/proposals/proposals_controller.rb

+          search_text: "",
+          origin: "all",
+          category_id: "",
+          random_seed: params[:random_seed] || @random_seed


??? You're passing @random_seed to search but @random_seed it's a value from search.

oriolgual · 2017-01-13T09:29:36Z

decidim-proposals/app/services/decidim/proposals/proposal_search.rb

+      # Handle the search_text filter
+      def search_search_text
+        query
+          .where("title ILIKE ?", "%#{search_text}%")


Should we add an index for this?

I'm agree. Also... we may need to add a better text search in the future.

josepjaume · 2017-01-13T09:15:49Z

decidim-core/app/assets/javascripts/decidim/filters.js.es6

-  const { DecidimMeetings: { MeetingsFormFilterComponent } } = exports;
-  const meetingsFormFilter = new MeetingsFormFilterComponent('form#new_filter');
+  const { Decidim: { FormFilterComponent } } = exports;
+  const formFilter = new FormFilterComponent('form#new_filter');


Not sure I like using global DOM ids here. Would it work if we accept a class instead?

Since the form is created using the Rails Form Builder it's a trivial change.

josepjaume · 2017-01-13T09:16:27Z

decidim-core/app/assets/javascripts/decidim/form_filter.component.js.es6

@@ -1,12 +1,15 @@
 /* eslint-disable no-div-regex, no-useless-escape, no-param-reassign */

 /**
- * A component that handles the meetings filter form.


I would clarify: "A plain JavaScript component" in order not to get confused with any other framework's components.

josepjaume · 2017-01-13T09:17:07Z

decidim-core/app/assets/javascripts/decidim/form_filter.component.js.es6

+     */
+    _parseLocationFilterValues() {
+      // Every location param is constructed like this: filter[key]=value
+      let regexpResult = decodeURIComponent(this._getLocation()).match(/filter\[([^\]]*)\](?:\[\])?=([^&]*)/g);


Isn't there any other way to parse GET params?

There is no native way to handle query params. You can find a lot of jQuery plugins or npm packages to do that but I think there is no need of that if you can use plain regular expressions. Thoughts?

Hey, I'm JavaScript, I know all about the DOM and I know about CORS and x-frame-options but let's not handle URLs properly.

josepjaume · 2017-01-13T09:23:46Z

decidim-core/app/helpers/decidim/filters_helper.rb

+          yield form
+        end
+      end
+      filters_container + javascript_include_tag("decidim/filters")


You sure this won't break with turbolinks? Does it work if we navigate back / on again?

I didn't find any issue so far. We are dealing with turbolinks events to bind and unbind change form events. We also use history API to handle the back and forward buttons and update the form values as well.

Maybe it doesn't break because we're using delegation. Cool.

josepjaume · 2017-01-13T09:25:06Z

decidim-core/app/services/decidim/resource_search.rb

+
+    # Handle the category_id filter
+    def search_category_id
+      query.where(decidim_category_id: category_ids)


Nice although not sure if we should make the assumption that category_id will be available in all resources. Maybe we could separate common searches into Query classes if we want to share them amongst resources.

In this scenario, something like:

def search_category_ids CategoryFilterQuery.new(query, categories).query end

or even, if we want to go for the rectify composition way:

def query ResourceSearch::BaseQuery.new(@scope) end def search_category_ids query | CategoryFilterQuery.new(categories) end

I feel like this is a good place to use rectify query composition. I would even consider removing Searchlight, as it feels like we have some functionality overlap and we could implement this filter ourselves.

Check out Rectify query composition's documentation.

I think combining Rectify with Searchlight is very useful for complex queries like the category_id filter. I think Searchlight provides us with a simple mechanism for checking the filter properties and adding conditions to the query.
And related to having resources without category_id is not a big deal since the method search_category_id will not be called if the filter doesn't contain a category_id attribute.

@josepjaume Rectify and Searchlight doesn't play nice together... I will not implement this yet but in the future we can rethink about the approach to drop Searchlight in favor of Rectify 😞

@beagleknight I KNEW IT HAHAHAHA 💥

If you don't send the category_id param, then this method doesn't get executed and will not break, so there's no need to worry about that I guess 😄

josepjaume · 2017-01-13T09:32:32Z

decidim-core/lib/decidim/filter_form_builder.rb

+      fieldset_wrapper options[:legend_title] do
+        super(method, collection, value_method, label_method, options, html_options) do |b|
+          if block_given?
+            yield b


Looks like yield is never being used (coverage reports).

It's not used because I'm not using the block in my filters but I thought it is a good practise to handle that edge case (since I'm overriding a form builder method).

josepjaume · 2017-01-13T09:34:36Z

decidim-core/lib/decidim/filter_form_builder.rb

+      fieldset_wrapper options[:legend_title] do
+        super(method, collection, value_method, label_method, options, html_options) do |b|
+          if block_given?
+            yield b


Looks like this isn't tested.

josepjaume · 2017-01-13T09:35:16Z

decidim-proposals/app/services/decidim/proposals/proposal_search.rb

+        if origin == "official"
+          query.where(decidim_author_id: nil)
+        elsif origin == "citizenship"
+          query.where.not(decidim_author_id: nil)


Looks like citizenship filter isn't being tested (coverage)

houndci-bot reviewed Jan 10, 2017

View reviewed changes

oriolgual added the wip label Jan 11, 2017

houndci-bot reviewed Jan 11, 2017

View reviewed changes

decidim-core/lib/decidim/filter_form_builder.rb

end

end

end

end

Copy link

houndci-bot Jan 11, 2017

Choose a reason for hiding this comment

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

Final newline missing.

houndci-bot reviewed Jan 11, 2017

View reviewed changes

decidim-core/lib/decidim/filter_form_builder.rb

end

end

end

end

Copy link

houndci-bot Jan 11, 2017

Choose a reason for hiding this comment

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

Final newline missing.

houndci-bot reviewed Jan 11, 2017

View reviewed changes

beagleknight requested review from josepjaume, mrcasals, divins, lastpotion and ItsGenis January 12, 2017 10:13

beagleknight added 13 commits January 13, 2017 09:12

Add feature test

24d784e

Add simple filter working using copy/paste

6cd4b03

Factor our search services code

eabe1b6

Add FilterResource concern

2b54640

Refactor filters views using filter_form_builder

3deb7d5

Fix rubocop issues

8c54f0d

Add missing documentation

ecb7125

Add default filter params

3afda24

Add origin filter

9be60cf

Refactor frontend code with issues

44861da

Update proposals count

d499a32

Update bundle

723b97f

Fix minor rubocop issues

3e3989e

beagleknight force-pushed the feature/proposals/filter branch from b7b1f03 to 3e3989e Compare January 13, 2017 08:13

mrcasals approved these changes Jan 13, 2017

View reviewed changes

Disable specific rubocop rules for specific files

fb3d714

oriolgual suggested changes Jan 13, 2017

View reviewed changes

josepjaume suggested changes Jan 13, 2017

View reviewed changes

Add some feedback

27ce8a2

oriolgual approved these changes Jan 13, 2017

View reviewed changes

josepjaume approved these changes Jan 13, 2017

View reviewed changes

beagleknight merged commit 6004cee into master Jan 13, 2017

oriolgual deleted the feature/proposals/filter branch January 16, 2017 09:01

oriolgual mentioned this pull request Feb 1, 2017

Filter proposals by links #728

Closed

2 tasks

aitorlb pushed a commit to CodiTramuntana/decidim that referenced this pull request Aug 8, 2019

🐛 💄 [participatory-space] decidim#474

e888a80

Add proposal filters #474

Add proposal filters #474

Conversation

beagleknight commented Jan 10, 2017 • edited Loading

🎩 What? Why?

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

👻 GIF

mention-bot commented Jan 10, 2017

codecov-io commented Jan 10, 2017 • edited Loading

Current coverage is 60.18% (diff: 94.91%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mention-bot commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrcasals left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beagleknight commented Jan 10, 2017 •

edited

Loading

codecov-io commented Jan 10, 2017 •

edited

Loading