Warn the user when not meetings or upcoming meetings scheduled. by lastpotion · Pull Request #995 · decidim/decidim

lastpotion · 2017-02-17T09:20:26Z

🎩 What? Why?

This PR adds a couple of warnings when there's no upcoming meetings or there's not meetings at all. Also, now we're showing all the past meetings the process had if aren't any upcoming meetings scheduled.

📌 Related Issues

Fixes Meetings should show past by default when there's no upcoming meetings #970

📷 Screenshots (optional)

👻 GIF

codecov-io · 2017-02-17T11:01:17Z

Codecov Report

Merging #995 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #995      +/-   ##
==========================================
+ Coverage   97.18%   97.19%   +<.01%     
==========================================
  Files         385      385              
  Lines        6363     6371       +8     
==========================================
+ Hits         6184     6192       +8     
  Misses        179      179

Impacted Files	Coverage Δ
...ontrollers/decidim/meetings/meetings_controller.rb	`100% <100%> (ø)`	✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4eb0a1...ad52dc5. Read the comment docs.

codecov-io · 2017-02-17T11:01:18Z

Codecov Report

Merging #995 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #995      +/-   ##
==========================================
+ Coverage   97.19%   97.19%   +<.01%     
==========================================
  Files         386      386              
  Lines        6384     6385       +1     
==========================================
+ Hits         6205     6206       +1     
  Misses        179      179

Impacted Files	Coverage Δ
...ontrollers/decidim/meetings/meetings_controller.rb	`100% <100%> (ø)`	✅
decidim-core/db/seeds.rb	`100% <ø> (ø)`	✅
...decidim/admin/update_participatory_process_step.rb	`100% <ø> (ø)`	✅
...dim-core/lib/decidim/core/api/process_step_type.rb	`100% <ø> (ø)`	✅
...s/decidim/admin/participatory_process_step_form.rb	`100% <ø> (ø)`	✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 014eba1...7cd79c3. Read the comment docs.

josepjaume · 2017-02-17T14:55:17Z

+          }
+          results = search_klass.new(search_params).results
+          if results.length == 0
+            params[:filter] = {


I'm unsure that mutating the params hash is a good option here (or anywhere). Thoughts @oriolgual? You're more acquainted with the code.

josepjaume · 2017-02-17T14:59:31Z

+        if !params[:filter] && results.length == 0
+          params[:filter] = {
+            date: "past",
+            meetings_warning: true


This could probably be done by checking if any filter is applied, instead of propagating the parameter around. Looks a bit convoluted to me and we're mixing view logic with controller logic.

josepjaume · 2017-02-17T15:00:39Z

  </div>

-  <%= form.collection_radio_buttons :date, [["upcoming", t(".upcoming")], ["past", t(".past")]], :first, :last, legend_title: t(".date") %>
+  <% unless @meetings_warning %>


I'd use a more meaningful variable name here: @upcoming_meetings or something like that.

josepjaume · 2017-02-17T15:01:10Z

@@ -1,5 +1,17 @@
+  <% if @meetings_warning %>


All the states in this view should be handled directly. Looks like the controller knows too much about view logic. See https://github.com/AjuntamentdeBarcelona/decidim/pull/995/files#r101773425

I'd simplify this, if meetings.length == 0 just show a message telling the user that there are no meetings, no need to have two different warnings.

@oriolgual I think it's useful to have two different warnings. If we change the behavior on load time (past instead of upcoming) it's nice to show a warning to the user.

@josepjaume but following this logic we could have a different message based on the filters you have selected.

josepjaume · 2017-02-17T15:01:47Z

      end
    end
+
+    context "No upcoming meetings scheduled" do


Tests are OK so fortunately you should be able to refactor the internals 👍

oriolgual

Remember to add specs

oriolgual · 2017-02-20T08:45:59Z

+          }
+          results = search_klass.new(search_params).results
+          if results.length == 0
+            params[:filter] = {


I think we need to take another approach on this, and create a different search object instead of creating the results object, this way we can still use all the helper methods.

Something like:

def index if search.results.length == 0 && params.dig("filter", "date") != "past" # Maybe this could be extracted to a method @search = search_klass.new(search_params.merge(filter: { date: "past" })) end end

oriolgual · 2017-02-20T08:50:04Z

@@ -1,5 +1,17 @@
+  <% if @meetings_warning %>


I'd simplify this, if meetings.length == 0 just show a message telling the user that there are no meetings, no need to have two different warnings.

oriolgual · 2017-02-20T08:51:15Z

+        params[:filter] ||= {}
+        @meetings_alert = params[:filter]["meetings_alert"]
+        @meetings_warning = params[:filter]["meetings_warning"]
+        @meetings = results.page(params[:page]).per(12)


And revert all this to helper methods.

oriolgual · 2017-02-20T14:00:01Z

        @geocoded_meetings ||= search.results.select(&:geocoded?)
      end

+      def static_map


oriolgual · 2017-02-20T14:00:20Z

+        end
      end

-      private


Don't remove this

oriolgual · 2017-02-20T14:02:01Z

        index:
          view_meeting: View meeting
+        meetings:
+          no_meetings_warning: At the moment, this process doesn't have any meeting scheduled.


This seems a weird translation from Catalan, what about:

"No meetings match your search criteria or no there isn't any meeting scheduled".

oriolgual · 2017-02-20T14:02:47Z

+    end
+
+    context "No meetings scheduled" do
+      let!(:meetings){[]}


missing spaces: let!(:meetings) { [] }

oriolgual · 2017-02-20T14:03:01Z

        start_time: date.beginning_of_day,
        end_time: date.end_of_day
      )
-


Don't remove this line

oriolgual · 2017-02-20T14:03:24Z

    context "without category or scope" do
      it "does not show any tag" do
-        expect(page).not_to have_selector("ul.tags.tags--meeting")
+          expect(page).not_to have_selector("ul.tags.tags--meeting")


Fix indentation everywhere

oriolgual · 2017-02-20T14:04:07Z

-        @meeting = Meeting.where(feature: current_feature).find(params[:id])
-        send_data StaticMapGenerator.new(@meeting).data, type: "image/jpeg", disposition: "inline"
+      def index
+        if search.results.length == 0 && params.dig("filter", "date") != "past" # Maybe this could be extracted to a method


Remove the comment (sorry I told you to keep it while offline xD)

josepjaume

Good work, way simpler! Please check my comment about a translation and that's it!

josepjaume · 2017-02-20T18:59:41Z

        index:
          view_meeting: View meeting
+        meetings:
+          no_meetings_warning: No meetings match your search criteria or no there isn't any meeting scheduled.


Typo: "Or there isn't"

…dim#995)

lastpotion added the in-progress label Feb 17, 2017

lastpotion changed the title ~~Fix/meetings/no upcoming meetings~~ Warn the user when not meetings or upcoming meetings schedulded. Feb 17, 2017

lastpotion changed the title ~~Warn the user when not meetings or upcoming meetings schedulded.~~ Warn the user when not meetings or upcoming meetings scheduled. Feb 17, 2017

josepjaume suggested changes Feb 17, 2017

View reviewed changes

oriolgual suggested changes Feb 20, 2017

View reviewed changes

lastpotion added 6 commits February 20, 2017 14:17

Add warning on missing meetings

1589d4c

Add tests

ab38590

Amend test

dff8de0

more meaningfull labels added

e8a68fd

Refactorization

f0fa707

Amend changes

5acf5d6

lastpotion force-pushed the fix/meetings/no_upcoming_meetings branch from ad52dc5 to 5acf5d6 Compare February 20, 2017 13:36

oriolgual suggested changes Feb 20, 2017

View reviewed changes

lastpotion added 2 commits February 20, 2017 15:11

amend changes

d3be9e4

Add missing space

bc6c376

josepjaume suggested changes Feb 20, 2017

View reviewed changes

josepjaume approved these changes Feb 20, 2017

View reviewed changes

fix typo

7cd79c3

oriolgual approved these changes Feb 21, 2017

View reviewed changes

lastpotion merged commit 3384621 into master Feb 21, 2017

lastpotion deleted the fix/meetings/no_upcoming_meetings branch February 21, 2017 15:22

lastpotion removed the in-progress label Feb 21, 2017

Quentinchampenois added a commit to Quentinchampenois/decidim that referenced this pull request Nov 12, 2020

update export_signature_pdf permissions to accept manual states (deci…

5bbf311

…dim#995)

andreslucena mentioned this pull request Nov 22, 2023

Review empty pages for all the components #11875

Closed

10 tasks

+                      end
                     end
-                    private

Uh oh!

Conversation

lastpotion commented Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📌 Related Issues

📷 Screenshots (optional)

👻 GIF

Uh oh!

codecov-io commented Feb 17, 2017

Codecov Report

Uh oh!

codecov-io commented Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

oriolgual left a comment

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!

josepjaume left a comment

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!

4 participants

lastpotion commented Feb 17, 2017 •

edited

Loading

codecov-io commented Feb 17, 2017 •

edited

Loading