Separate forms in steps by mrcasals · Pull Request #6108 · decidim/decidim

mrcasals · 2020-05-18T13:09:45Z

🎩 What? Why?

Newer, simpler take on #5728. This PR replaces #6010.

📌 Related Issues

Related to Survey steps #6010
Related to EPIC: Improve the Survey component #5728

📋 Subtasks

Add CHANGELOG entry
Add "separator" type question to keep track of how to separate in steps
Admin: Ensure separators appear clearly
Admin: fix bug when adding a question, button to ove "Up" seemed not to work
Admin: add tests adding separators
Public: ensure separatable questionnaires appear separated
Public: be able to move between questionnaire steps (use JS to change the step)
Ensure everything work as expected for meetings

📷 Screenshots (optional)

Form with a separator to split into steps:

Public area: First step of the form

Public area: second step of the form

they were being added to the end of the element, but there are some invisible elements there, so moving the question up seemed not to work for a while, but it was actually moving betwenn invisible elements. This is now fixed, since new questions are added right after the last one, thus avoiding the invisible elements.

This made the binding to fire for every click on the wrapper element, thus creating a lot of child answer options.

agustibr

Looks good to me! 👌

leio10

Great work, it seems much easier this way! 👏 👏 👍

leio10 · 2020-05-20T13:47:44Z

decidim-forms/app/views/decidim/forms/questionnaires/show.html.erb

-                      <%= cell("decidim/represent_user_group", form) %>
-                    </div>
-                  <% end %>
+                    <% step_answers.each do |answer| %>


could you use each_with_index for the answer_idx variable?

I don't think so, because I need to keep the counter between steps (which would be lost). Also, I need to skip the separators, so I'd need to keep track of those elements... I think it's easier to keep track using the answer_ids, but I'll give it another thought!

leio10 · 2020-05-20T13:50:29Z

decidim-forms/app/cells/decidim/forms/step_navigation/show.erb

-      class: "hollow secondary"
+      class: "hollow secondary",
+      data: {
+        toggle: [previous_step_dom_id, current_step_dom_id].join(" ")


Wow, I was looking for the JS part! 🤩 ❤️

It turned out to be much easier than expected! 😄 Luckily, Foundation's toggler component allows for multiple toggles from the same element, this allowed me to implement the steps 😄

mrcasals · 2020-05-21T11:02:58Z

@microstudi whoops, sorry! I removed it myself, thanks for noticing!

microstudi

This is an amazing work @mrcasals!

I have a couple of comments between the code, I might be wrong in both of them, please let me know.

About the general behaviour, I am only missing one thing, it is no possible to go from one step to the other using only URL changes (in this case it should be hashes).
I think it would be nice to show in the URL the current step:

=>

One problem of not doing so, is that when you are submitting the survey with errors it returns you to the step1, and it might well be that the error is not in that page, which is confusing. If URL navigation was in place, then we could return the first step with an error in it. That would give the user a much better feedback.

However, I am not sure if this is would be out of scope regarding what @decidim/product has asked.
Thanks again.

microstudi · 2020-05-21T13:49:19Z

decidim-forms/app/forms/decidim/forms/questionnaire_form.rb

+      # Public: Splits reponses by step, keeping the separator. Ruby doesn't
+      # seem to give a way to do this in the standard library, so we had to
+      # write our own version.
+      #
+      # Returns an array of steps. Each step is a list of the questions in that
+      # step, including the separator.
+      def responses_by_step
+        @responses_by_step ||=
+          begin
+            steps_acc = [[]]
+            step_index = 0
+
+            responses.each do |response|
+              steps_acc[step_index] << response
+
+              if response.question.separator?
+                step_index += 1
+                steps_acc[step_index] = []
+              end
+            end
+            steps_acc
+          end
+      end


Not that I found anything wrong with this, but since you comment that there's no ruby method to do that, could this do the trick?

@responses_by_step ||= responses.chunk_while { |a,b| !a.question.separator? }

Oh, I didn't know about this one, thanks!

decidim-forms/app/models/decidim/forms/question.rb

mrcasals · 2020-05-21T14:57:10Z

One problem of not doing so, is that when you are submitting the survey with errors it returns you to the step1, and it might well be that the error is not in that page, which is confusing. If URL navigation was in place, then we could return the first step with an error in it. That would give the user a much better feedback.

@microstudi for that we'd need a much more intelligent system: we'd need to locate the first field with an error, find its step and jump to that page... And sincerely I think that's out of the scope 😅

microstudi · 2020-05-21T15:10:09Z

@microstudi for that we'd need a much more intelligent system: we'd need to locate the first field with an error, find its step and jump to that page... And sincerely I think that's out of the scope sweat_smile

All right, but as I understand, this is a part of #5728 right?, because I see that it does not cover everything. Would it be more PRs to complete the EPIC? Maybe @carolromero could tell us what she thinks about this.
I'm going to need the ok of @decidim/product (have you tested this?) to approve it.

mrcasals · 2020-05-21T16:01:28Z

@microstudi as explained in #5728, we treat that issue as an EPIC (I actually think Product has updated the title). This PR closes some of the criterias, while the rest will be covered in upcoming PRs.

Anyway, the tests are now failing after the refactor you suggested, I'll look at this tomorrow.

microstudi · 2020-05-21T16:18:03Z

@mrcasals oops, sorry for the fuss then, thanks!

mrcasals · 2020-05-22T07:40:05Z

@microstudi all green now! Can you re-review this please? 😄

/cc @decidim/core

mrcasals · 2020-05-25T08:41:54Z

I've added a design tweak as explained in #5728 (comment)

mrcasals · 2020-05-25T11:02:02Z

@decidim/core @microstudi I'm getting failures in decidim-core/spec/system/messaging/profile_conversations_spec.rb:281 after merging from develop. This is unrelated to this issue and appeared after #6009 was merged. For example, see the build for the merge commit of #6121, which fails for the sme reason (https://github.com/decidim/decidim/runs/704413614)

Can we get this PR merged anyway to avoid conflicts?

microstudi · 2020-05-25T15:15:21Z

@mrcasals it seems to be a flaky test. Capybara sometimes seems to execute press a button, some times doesn't. I'm not really sure how to solve it, I'll keep investigating it.

* develop: Include year in meetings card (decidim#6102) Add attachment enabled option to initiative types and initiatives (decidim#6036) Fix a flaky test in group profile conversations (decidim#6123) Add attachments to Initiatives (decidim#5844) Add initiatives export (decidim#6070) Improvements to conversations with more than one participant (decidim#6094) Elections module and election administration (decidim#6065) Separate forms in steps (decidim#6108) Add sorting by publishing date to initiatives (decidim#6016) Improve proposal preview: Use proposal card when previewing a proposal draft (decidim#6064) Newsletter templates fixes (decidim#6096) # Conflicts: # decidim-initiatives/app/models/decidim/initiative.rb # decidim-initiatives/spec/system/admin/initiative_types_controller_spec.rb

* feature/add_areas_to_initiatives: (30 commits) Adds areas to FO filters Fix lint issue Fixes rubocop issues Updates changelog Adds areas to initiatives Send notification when signature threshold reached (decidim#6098) Adds filter by initiative type to admin panel (decidim#6093) Require confirmation on exiting a survey mid-answering (decidim#6118) Information message when there isn't any Proposal (decidim#6063) Set email asset host dynamically (decidim#5888) Harmonizes the design of initiatives search in FO (decidim#6090) Include year in meetings card (decidim#6102) Add attachment enabled option to initiative types and initiatives (decidim#6036) Fix a flaky test in group profile conversations (decidim#6123) Add attachments to Initiatives (decidim#5844) Add initiatives export (decidim#6070) Improvements to conversations with more than one participant (decidim#6094) Elections module and election administration (decidim#6065) Separate forms in steps (decidim#6108) Add sorting by publishing date to initiatives (decidim#6016) ... # Conflicts: # decidim-initiatives/app/cells/decidim/initiatives/initiative_m_cell.rb # decidim-initiatives/app/commands/decidim/initiatives/admin/update_initiative.rb # decidim-initiatives/app/controllers/decidim/initiatives/initiatives_controller.rb # decidim-initiatives/app/forms/decidim/initiatives/admin/initiative_form.rb # decidim-initiatives/app/helpers/decidim/initiatives/application_helper.rb # decidim-initiatives/app/models/decidim/initiative.rb # decidim-initiatives/app/services/decidim/initiatives/initiative_search.rb # decidim-initiatives/app/views/decidim/initiatives/create_initiative/fill_data.html.erb # decidim-initiatives/app/views/decidim/initiatives/initiatives/_filters.html.erb # decidim-initiatives/app/views/decidim/initiatives/initiatives/_tags.html.erb # decidim-initiatives/config/locales/en.yml # decidim-initiatives/db/migrate/20200514085422_add_area_to_initiatives.rb # decidim-initiatives/db/migrate/20200514102631_add_area_enabled_option_to_initiatives.rb # decidim-initiatives/spec/forms/initiative_form_spec.rb # decidim-initiatives/spec/services/decidim/initiatives/initiative_search_spec.rb # decidim-initiatives/spec/shared/update_initiative_type_example.rb # decidim-initiatives/spec/system/admin/admin_manages_initiatives_spec.rb # decidim-initiatives/spec/system/admin/initiative_types_controller_spec.rb # decidim-initiatives/spec/system/filter_initiatives_spec.rb

* Survey steps * Add basic system tests * Add separators in admin area * Persist separators * Fix weird behavior when adding questions and moving them up they were being added to the end of the element, but there are some invisible elements there, so moving the question up seemed not to work for a while, but it was actually moving betwenn invisible elements. This is now fixed, since new questions are added right after the last one, thus avoiding the invisible elements. * Fix bug when adding a separator, buttons where not correct * Fix bug when adding questions on new survey * Hide separator from question types list * Fix bug on events binding This made the binding to fire for every click on the wrapper element, thus creating a lot of child answer options. * Prepare for step layout in public area * Paginate steps in public area * Handle survey submissions * Delete unused locale keys * Fix tests for public area on surveys * Generalize use case * Add missing tests * Add changelog * Fix wrong locale key * Hide spearators for anonymous users * Simplify code * Fix refactored method * Surveys font style fixes Co-authored-by: Javier Usobiaga <javier@swwweet.com> # Conflicts: # decidim-forms/app/commands/decidim/forms/answer_questionnaire.rb # decidim-forms/app/models/decidim/forms/question.rb # decidim-forms/app/views/decidim/forms/admin/questionnaires/_form.html.erb # decidim-forms/app/views/decidim/forms/questionnaires/_answer.html.erb # decidim-forms/app/views/decidim/forms/questionnaires/show.html.erb # decidim-forms/config/locales/en.yml # decidim-forms/lib/decidim/forms/test/factories.rb # decidim-forms/lib/decidim/forms/test/shared_examples/has_questionnaire.rb # decidim-forms/lib/decidim/forms/test/shared_examples/manage_questionnaires.rb # decidim_app-design/app/views/public/index.html.erb

* Survey steps * Add basic system tests * Add separators in admin area * Persist separators * Fix weird behavior when adding questions and moving them up they were being added to the end of the element, but there are some invisible elements there, so moving the question up seemed not to work for a while, but it was actually moving betwenn invisible elements. This is now fixed, since new questions are added right after the last one, thus avoiding the invisible elements. * Fix bug when adding a separator, buttons where not correct * Fix bug when adding questions on new survey * Hide separator from question types list * Fix bug on events binding This made the binding to fire for every click on the wrapper element, thus creating a lot of child answer options. * Prepare for step layout in public area * Paginate steps in public area * Handle survey submissions * Delete unused locale keys * Fix tests for public area on surveys * Generalize use case * Add missing tests * Add changelog * Fix wrong locale key * Hide spearators for anonymous users * Simplify code * Fix refactored method * Surveys font style fixes Co-authored-by: Javier Usobiaga <javier@swwweet.com> # Conflicts: # decidim_app-design/app/views/public/index.html.erb

htmlboy and others added 2 commits May 18, 2020 15:01

Survey steps

29de81d

Add basic system tests

153acdf

mrcasals added status: WIP project: PAM2020 Barcelona City Council contract labels May 18, 2020

mrcasals mentioned this pull request May 18, 2020

Survey steps #6010

Closed

16 tasks

mrcasals changed the title ~~Forms/separate in steps~~ Separate forms in steps May 18, 2020

mrcasals added 4 commits May 18, 2020 16:17

Add separators in admin area

b68f965

Persist separators

eaa69f2

Fix bug when adding a separator, buttons where not correct

c0b9015

mrcasals force-pushed the forms/separate-in-steps branch from 70ff8d2 to c0b9015 Compare May 19, 2020 08:53

mrcasals added 9 commits May 19, 2020 11:04

Fix bug when adding questions on new survey

9102c4c

Hide separator from question types list

4a0e88d

Fix bug on events binding

9cf3189

This made the binding to fire for every click on the wrapper element, thus creating a lot of child answer options.

Prepare for step layout in public area

871f4c0

Paginate steps in public area

ec7106c

Handle survey submissions

eff6f7b

Delete unused locale keys

55c63bb

Fix tests for public area on surveys

4128414

Generalize use case

4d6b015

mrcasals force-pushed the forms/separate-in-steps branch from 839bf6c to 4d6b015 Compare May 20, 2020 07:30

Add missing tests

91de7e7

mrcasals requested review from agustibr and leio10 May 20, 2020 09:58

agustibr previously approved these changes May 20, 2020

View reviewed changes

Add changelog

702b1e6

mrcasals dismissed agustibr’s stale review via 702b1e6 May 20, 2020 10:42

mrcasals added 2 commits May 20, 2020 12:44

Fix wrong locale key

a136ba7

Merge branch 'develop' into forms/separate-in-steps

4f6c648

leio10 previously approved these changes May 20, 2020

View reviewed changes

microstudi reviewed May 21, 2020

View reviewed changes

mrcasals changed the title ~~Separate forms in steps~~ Improve forms: steps and confirmation request when exiting the page May 21, 2020

mrcasals changed the title ~~Improve forms: steps and confirmation request when exiting the page~~ Separate forms in steps May 21, 2020

Simplify code

ffbeda8

Fix refactored method

70090c4

mrcasals self-assigned this May 22, 2020

mrcasals mentioned this pull request May 22, 2020

EPIC: Improve the Survey component #5728

Closed

6 tasks

mrcasals linked an issue May 22, 2020 that may be closed by this pull request

EPIC: Improve the Survey component #5728

Closed

6 tasks

mrcasals and others added 2 commits May 22, 2020 15:40

Merge branch 'develop' into forms/separate-in-steps

32864ea

Surveys font style fixes

cf1c87d

microstudi approved these changes May 25, 2020

View reviewed changes

microstudi merged commit 1138d9e into develop May 25, 2020

microstudi deleted the forms/separate-in-steps branch May 25, 2020 15:14

microstudi removed the in-review label May 27, 2020

microstudi mentioned this pull request Jul 8, 2020

Temp/0.22 surveys Platoniq/decidim#26

Closed

microstudi mentioned this pull request Jul 23, 2020

Temp/0.21 surveys Platoniq/decidim#29

Closed

13 tasks

verarojman mentioned this pull request Jan 28, 2021

Don't render separators when exporting questionnaires #7243

Merged

Uh oh!

Conversation

mrcasals commented May 18, 2020 • 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!

agustibr left a comment

Choose a reason for hiding this comment

Uh oh!

leio10 left a comment

Choose a reason for hiding this comment

Uh oh!

leio10 May 20, 2020

Choose a reason for hiding this comment

Uh oh!

mrcasals May 20, 2020

Choose a reason for hiding this comment

Uh oh!

leio10 May 20, 2020

Choose a reason for hiding this comment

Uh oh!

mrcasals May 21, 2020

Choose a reason for hiding this comment

Uh oh!

mrcasals commented May 21, 2020

Uh oh!

microstudi left a comment

Choose a reason for hiding this comment

Uh oh!

microstudi May 21, 2020

Choose a reason for hiding this comment

Uh oh!

mrcasals May 21, 2020

Choose a reason for hiding this comment

Uh oh!

mrcasals May 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrcasals commented May 21, 2020

Uh oh!

microstudi commented May 21, 2020

Uh oh!

mrcasals commented May 21, 2020

Uh oh!

microstudi commented May 21, 2020

Uh oh!

mrcasals commented May 22, 2020

Uh oh!

mrcasals commented May 25, 2020

Uh oh!

mrcasals commented May 25, 2020

Uh oh!

microstudi commented May 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mrcasals commented May 18, 2020 •

edited

Loading