Change surveys public page and use TOS by beagleknight · Pull Request #1410 · decidim/decidim

beagleknight · 2017-05-29T14:37:34Z

🎩 What? Why?

I used survey's TOS and fix a few issues of the public survey form. I also changed some styles.

📌 Related Issues

Related to Add Surveys engine #1342

📋 Subtasks

Use better style for the public page
Use TOS
Fix tests
Fix some weird paddings/margins

📷 Screenshots (optional)

👻 GIF

codecov · 2017-05-29T14:41:44Z

Codecov Report

Merging #1410 into feature/surveys will decrease coverage by 27.05%.
The diff coverage is n/a.

@@                 Coverage Diff                 @@
##           feature/surveys   #1410       +/-   ##
===================================================
- Coverage            94.86%   67.8%   -27.06%     
===================================================
  Files                  444      62      -382     
  Lines                 7554     966     -6588     
===================================================
- Hits                  7166     655     -6511     
+ Misses                 388     311       -77

Impacted Files	Coverage Δ
...n/app/models/decidim/admin/abilities/admin_user.rb	`21.42% <0%> (-78.58%)`	⬇️
...mments/lib/decidim/comments/mutation_extensions.rb	`25% <0%> (-75%)`	⬇️
...-comments/lib/decidim/comments/query_extensions.rb	`36.36% <0%> (-63.64%)`	⬇️
...im-proposals/lib/decidim/proposals/admin_engine.rb	`50% <0%> (-50%)`	⬇️
decidim-surveys/lib/decidim/surveys/engine.rb	`50% <0%> (-50%)`	⬇️
decidim-core/lib/decidim/stats_registry.rb	`51.35% <0%> (-48.65%)`	⬇️
decidim-budgets/lib/decidim/budgets/list_engine.rb	`53.33% <0%> (-46.67%)`	⬇️
decidim-proposals/lib/decidim/proposals/engine.rb	`56.25% <0%> (-43.75%)`	⬇️
...dim-core/lib/decidim/features/settings_manifest.rb	`51.28% <0%> (-43.59%)`	⬇️
...ecidim-surveys/lib/decidim/surveys/admin_engine.rb	`62.5% <0%> (-37.5%)`	⬇️
... and 396 more

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 d660080...1ef26f8. Read the comment docs.

codecov · 2017-05-29T14:41:44Z

Codecov Report

Merging #1410 into feature/surveys will increase coverage by <.01%.
The diff coverage is 100%.

@@                 Coverage Diff                 @@
##           feature/surveys    #1410      +/-   ##
===================================================
+ Coverage            94.89%   94.89%   +<.01%     
===================================================
  Files                  445      445              
  Lines                 7593     7599       +6     
===================================================
+ Hits                  7205     7211       +6     
  Misses                 388      388

josepjaume · 2017-05-30T11:08:48Z

Careful with the margins and paddings, especially on the bottom part (the disclaimer)

beagleknight · 2017-05-30T12:15:20Z

@josepjaume Yeah! I'll fix it but we have the same problem in decidim-hospitalet :(

mention-bot · 2017-05-30T13:42:28Z

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

oriolgual · 2017-05-30T14:00:29Z

decidim-surveys/app/forms/decidim/surveys/survey_answer_form.rb

      end
+
+      def body_not_blank
+        errors.add("body", :blank) if body.all?(&:blank?)


Shouldn't it be any??

mmm no. Since body is an array it prevents submitting this [""].

Maybe I'm missing something, why is body an array? What I meant is that if any element of the array is blank we should fail.

The body is an array because the user can choose multiple answers like: ["Answer A", "Answer B"]. I'm using the same field to store single answer questions as well so if the user doesn't fill in the answer it sends [""] to the server.

oriolgual · 2017-05-30T14:01:44Z

decidim-surveys/config/locales/en.yml

            title: Answer the survey
          are_you_sure: This action cannot be undone and you will not be able to edit
            your answers. Are you sure?
+          authorize_toc: Authorize toc


This text is confusing

mrcasals · 2017-05-31T08:02:01Z

decidim-dev/lib/decidim/dev/test/rspec_support/translation_helpers.rb

  # locale - the ID of the locale to check
-  def have_i18n_content(field, locale: I18n.locale)
-    have_content(stripped(translated(field, locale: locale)))
+  # upcase - a boolean to indicate wether the string must be checked upcased or not.


You might want to change this to "whether"...

xDDDDDDDDDDD

mrcasals · 2017-05-31T08:05:41Z

decidim-surveys/app/views/decidim/surveys/surveys/show.html.erb

-              <div class="callout success">
-                <h5><%= t('.survey_answered.title') %></h5>
-                <p><%= t('.survey_answered.body') %></p>
+        <% else %>


I hate these unless ... else ..., I feels it's hard to grasp the logic. Can you switch the order of the blocks?

if survey.published? <the part below> else <the part above

You can do this in another PR

beagleknight added the in-progress label May 29, 2017

beagleknight changed the base branch from master to feature/surveys May 29, 2017 14:37

beagleknight force-pushed the feature/surveys-public-page branch from 770aad2 to 1ef26f8 Compare May 29, 2017 14:41

beagleknight changed the title ~~Feature/surveys public page~~ Change surveys public page and use TOC May 29, 2017

beagleknight mentioned this pull request May 29, 2017

Add Surveys engine #1342

Merged

8 tasks

beagleknight self-assigned this May 29, 2017

beagleknight force-pushed the feature/surveys-public-page branch 2 times, most recently from 3a57a3b to 14e5bb2 Compare May 30, 2017 13:01

beagleknight added 3 commits May 30, 2017 15:04

Several survey public page modifications and add toc

60f2b70

Fix a few tests

e64ddfb

Fix broken specs

7a9a741

beagleknight force-pushed the feature/surveys-public-page branch from 14e5bb2 to 7a9a741 Compare May 30, 2017 13:06

beagleknight added in-review and removed in-progress labels May 30, 2017

Add custom styles

bb2c687

oriolgual suggested changes May 30, 2017

View reviewed changes

Change from toc to tos

09bf87e

beagleknight changed the title ~~Change surveys public page and use TOC~~ Change surveys public page and use TOS May 30, 2017

oriolgual previously approved these changes May 31, 2017

View reviewed changes

Fix broken tests

7149a74

beagleknight dismissed oriolgual’s stale review via 7149a74 May 31, 2017 07:41

mrcasals reviewed May 31, 2017

View reviewed changes

mrcasals approved these changes May 31, 2017

View reviewed changes

beagleknight merged commit 349f400 into feature/surveys May 31, 2017

beagleknight deleted the feature/surveys-public-page branch May 31, 2017 08:09

beagleknight removed the in-review label May 31, 2017

oriolgual restored the feature/surveys-public-page branch May 31, 2017 08:51

This was referenced May 31, 2017

Change surveys public page and use TOS #1418

Closed

Restore missing PRs in feature/surveys #1421

Merged

Uh oh!

Conversation

beagleknight commented May 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

👻 GIF

Uh oh!

codecov bot commented May 29, 2017

Codecov Report

Uh oh!

codecov bot commented May 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

josepjaume commented May 30, 2017

Uh oh!

beagleknight commented May 30, 2017

Uh oh!

mention-bot commented May 30, 2017

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!

5 participants

beagleknight commented May 29, 2017 •

edited

Loading

codecov bot commented May 29, 2017 •

edited

Loading