Filter meetings by mrcasals · Pull Request #399 · decidim/decidim

mrcasals · 2016-12-20T14:07:27Z

🎩 What? Why?

This PR adds a way to filter meetings by scope and to order them by start time.

📌 Related Issues

Fixes Filter and paginate meetings #372

📋 Subtasks

📷 Screenshots (optional)

codecov-io · 2016-12-21T09:44:45Z

Current coverage is 93.58% (diff: 100%)

Merging #399 into master will increase coverage by 0.07%

@@             master       #399   diff @@
==========================================
  Files           346        350     +4   
  Lines          5044       5115    +71   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           4717       4787    +70   
- Misses          327        328     +1   
  Partials          0          0

Powered by Codecov. Last update cd7f557...759ac3c

josepjaume · 2016-12-21T11:54:40Z

decidim-meetings/app/views/decidim/meetings/meetings/_meetings.html.erb

Please create a decidim_paginate helper so we don't mutate params.

This has to be changed in the proposals engine too.

Also, this helper could also set the theme

beagleknight · 2016-12-22T10:49:04Z

decidim-meetings/app/assets/javascripts/decidim/meetings/meetings_filters.js.es6

We should bind this event on turbolinks:load as well. I recommend moving this code into a class, something like MeetingFilterFormComponent. Then you can bind both events document:ready and turbolinks:load and create an instance of this class (or method, or whatev) to bind the form events.
Also I'm not a huge fan of exposing random globals so it's better if you use a namespace to include the previous class (or method, or whatev) into a global called DecidimMeetings.

@beagleknight I'll need your help on this then.

beagleknight · 2016-12-22T13:39:49Z

decidim-meetings/app/assets/javascripts/decidim/meetings/meetings_filters.js.es6

I think it will be more maintainable if we move those regex into separate variables or just factor out some kind of function to extract the params. We can review it later @mrcasals

houndci-bot · 2017-01-04T09:22:48Z

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

Use only ascii symbols in comments.

beagleknight · 2017-01-04T10:37:16Z

decidim-meetings/app/assets/javascripts/decidim/meetings/meetings_form_filter.component.js.es6

Docs? I used jsdoc to document my components. Take a look at decidim-comments components.

beagleknight · 2017-01-04T10:40:58Z

decidim-meetings/app/views/decidim/meetings/meetings/index.js.erb

This is not transpiled so I recommend using var instead of const.

beagleknight · 2017-01-04T10:43:01Z

decidim-meetings/app/assets/javascripts/decidim/meetings/meetings_filters.js.es6

This code still feels very strange to me... I'm not really sure why we need to mix document events with turbolinks ones. Can you add some documentation about this so we remember why we didn't use turbolinks:load event instead of document ready event.

oriolgual · 2017-01-05T11:04:56Z

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

Wrongs docs

oriolgual · 2017-01-05T11:05:27Z

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

?? Shouldn't you use paginate_params?

No, since the paginate helper uses the global params. paginate_params are extra parameters that need to be sent to the helper but do not affect the current URL.

Since kaminari will merge the given params with the global ones, instead of deleting them from params we can pass "participatory_process_id" and "feature_id" as empty values.

oriolgual · 2017-01-05T11:07:38Z

decidim-meetings/app/commands/decidim/meetings/admin/create_meeting.rb

I don't like using ids here, see Admin::ProposalForm since it also has scope & category.

oriolgual · 2017-01-05T11:07:43Z

decidim-meetings/app/commands/decidim/meetings/admin/update_meeting.rb

oriolgual · 2017-01-05T11:09:02Z

decidim-meetings/app/forms/decidim/meetings/admin/meeting_form.rb

Should be the same for category?

oriolgual · 2017-01-05T11:14:09Z

decidim-meetings/spec/commands/create_meeting_spec.rb

Use objects instead of ids

oriolgual · 2017-01-05T11:14:24Z

decidim-meetings/spec/commands/update_meeting_spec.rb

oriolgual · 2017-01-05T11:15:17Z

decidim-meetings/spec/factories.rb

start_time.advance(hours: 2)

oriolgual · 2017-01-05T11:15:24Z

decidim-meetings/spec/forms/meeting_form_spec.rb

oriolgual · 2017-01-05T11:16:47Z

decidim-proposals/app/views/decidim/proposals/proposals/index.html.erb

You aren't passing the params here. Maybe we should pass the with params.to_h or .dup

I can't pass the URL params here, since paginate (from Kaminari) will keep using the global params object to generate the pagination. The hash containing random_seed is the one passed to the decidim_paginate helper as extra params.

josepjaume · 2017-01-09T09:40:17Z

decidim-meetings/app/assets/javascripts/decidim/meetings/meetings_form_filter.component.js.es6

+    _onPopState() {
+      this._clearForm();
+
+      let [sortValue] = this._getLocationParams(/order_start_time=([^&]*)/g) || ["asc"];


So freaking sad that JavaScript doesn't have a built-in way to deal with GET parameters.

mrcasals force-pushed the filter_meetings branch from 81071e7 to 01527dc Compare December 21, 2016 09:44

josepjaume suggested changes Dec 21, 2016

View reviewed changes

mrcasals mentioned this pull request Dec 21, 2016

Improve FormBuilder#categories_select #403

Merged

mrcasals force-pushed the filter_meetings branch from 7262125 to 458f446 Compare December 21, 2016 14:55

beagleknight suggested changes Dec 22, 2016

View reviewed changes

beagleknight reviewed Dec 22, 2016

View reviewed changes

mrcasals force-pushed the filter_meetings branch from 89179ec to 5cbb664 Compare January 4, 2017 08:14

houndci-bot reviewed Jan 4, 2017

View reviewed changes

mrcasals force-pushed the filter_meetings branch from 8992cda to 0c52659 Compare January 4, 2017 09:48

beagleknight suggested changes Jan 4, 2017

View reviewed changes

mrcasals force-pushed the filter_meetings branch from ee1aa5f to 2b5ec3f Compare January 4, 2017 13:25

oriolgual suggested changes Jan 5, 2017

View reviewed changes

beagleknight approved these changes Jan 5, 2017

View reviewed changes

mrcasals force-pushed the filter_meetings branch from 18caa10 to cf388be Compare January 9, 2017 08:31

mrcasals added 15 commits January 9, 2017 10:10

Add scopes to seeds

5aaf91a

Let users add scopes to meetings

b2c3c80

Add MeetingsSearch

e2857b6

Add MeetingsSearch service

1e70a52

Let users filter meetings by date and scope

96583a2

Paginate meetings

987448f

Improve MeetingSearch specs

2dfd0b6

Remove deprecation warning

2f7bd00

Update specs with meeting-scope relation

2dae357

Add missing translations

85cf323

Start adding categories to meetings

821ae64

Fix specs

51da8f6

Filter meetings by categories

a5810d6

Filtering by a parent category returns its children too

97a6dbe

Improve layouts

db5f49b

mrcasals added 12 commits January 9, 2017 10:10

Little JS refactor

6397918

Refactor meetings filters js

d7de6f1

Add JS tests

671919a

Move pagination logic to a helper

710008b

Move destroy hook to before_destroy

460e8db

Fix paginate helper

d4cfa9b

Update JS docs

b153060

Improve code

bbb5cbf

use objects instead of ids

25aec66

Inject the feature from the controller

842edfe

Do not remove anything form global params

63f1d12

Link categories and scopes to their filters

759ac3c

mrcasals force-pushed the filter_meetings branch from cf388be to 759ac3c Compare January 9, 2017 09:36

oriolgual approved these changes Jan 9, 2017

View reviewed changes

josepjaume approved these changes Jan 9, 2017

View reviewed changes

mrcasals merged commit 22acb67 into master Jan 9, 2017

mrcasals deleted the filter_meetings branch January 9, 2017 10:23

This was referenced Jan 12, 2017

Add proposal filters #474

Merged

Add filters templates for mobile devices #532

Closed

andreslucena mentioned this pull request Feb 14, 2017

Filter by global scope (proposals, meetings, results, etc) #750

Closed

Uh oh!

Conversation

mrcasals commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

Uh oh!

codecov-io commented Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 93.58% (diff: 100%)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

mrcasals commented Dec 20, 2016 •

edited

Loading

codecov-io commented Dec 21, 2016 •

edited

Loading