Feature/2728 endorsement to proposals apply new design #2733

tramuntanal · 2018-02-14T15:41:00Z

🎩 What? Why?

📌 Related Issues

Related to Endorsement to proposals: apply new design #2728

📋 Subtasks

Add CHANGELOG entry
Apply new designs.

…ments_card?.

…already endorsed.

codecov · 2018-02-14T15:53:29Z

Codecov Report

Merging #2733 into master will decrease coverage by 0.02%.
The diff coverage is 73.07%.

@@            Coverage Diff             @@
##           master    #2733      +/-   ##
==========================================
- Coverage   98.85%   98.83%   -0.03%     
==========================================
  Files        1581     1581              
  Lines       37412    37407       -5     
==========================================
- Hits        36985    36972      -13     
- Misses        427      435       +8

Crashillo · 2018-02-14T17:21:33Z

Done, design app updated. :)

tramuntanal · 2018-02-15T08:51:23Z

Thanks @Crashillo, but it seems you broke the build of ci/circleci: admin 😢

Crashillo · 2018-02-15T09:04:24Z

O_o, holy s***. How come? If you check the commit out, you'll see I edited just one view! It seems that a test is broken, somehow related to admin/users? Sorry, but I've no idea how to solve it. Let's see if @decidim/lot-core could help us here.

mrcasals · 2018-02-15T09:48:51Z

It looks like you found a random failing test, #2738 fixes it. I'll reschedule yours to make things work 😄

mrcasals · 2018-02-15T10:10:10Z

@Crashillo @tramuntanal tests are green!

tramuntanal · 2018-02-15T10:44:32Z

@mrcasals Thanks!! 👍

@Crashillo I've just added the login required part. Waiting for the identities popup implemented. Thanks a lot!

tramuntanal · 2018-02-15T11:07:19Z

@mrcasals I've executed the specs locally and all passed, is it possible that this "ci/circleci: comments" failure is another random failure?

oriolgual · 2018-02-16T11:02:36Z

I've re-enqueued the job @tramuntanal

…y_new_design

tramuntanal · 2018-02-20T18:06:30Z

@decidim/lot-core @decidim/product I think it is ready to be merged.

…in the select identity dialog).

arnaumonty · 2018-02-21T13:03:58Z

Hi @tramuntanal I've been checking on staging and it works well. I have just a few comments:

When you click on endorse and the window to select your identity is opened, after to select the identity and click "ok" the window should close it and it remains open.
In the list of endorsements when you have more than 4 endorsements you can see "see all" endorsements, but if you click "see all" you keep seeing "see all" link but I was expecting to see something to "See all" and "See less" or something similar.
And when you click on "see all" it displays a list and not a continuous text separated by comma, and the behaviour expected is to have a continuous text.

xabier · 2018-02-21T14:23:50Z

Also (I commented this via Telegram)

The behaviour of the "endorse" button should mimick the behaviour of the suport/vote button: it displays as actuable for anyone it asks login or registration when clicking if participant is not.

xabier · 2018-02-21T14:25:31Z

@tramuntanal if any of these improvements (except for @arnaumonty 's first suggestion which I think is critical) involves long complication, I see better to merge and then iterate.

xabier · 2018-02-21T14:26:27Z

@tramuntanal @Crashillo ... BTW: it looks fantastic!!! Congrats!

…n or registration when clicking if participant is not.

…ing the identities and click "Done" the window should close but remained open.

…eparated.

tramuntanal · 2018-02-22T13:02:20Z

@decidim/lot-core After applying fixes required by @decidim/product this PR is ready to be merged 🥁

oriolgual · 2018-02-22T14:38:54Z

decidim-proposals/app/helpers/decidim/proposals/proposal_endorsements_helper.rb

@@ -68,6 +75,36 @@ def endorsement_button(proposal, from_proposals_list, btn_label = nil, user_grou
                                                                                    current_endorsement_url: current_endorsement_url,
                                                                                    endorse_label: endorse_label, unendorse_label: unendorse_label }
      end
+
+      def fully_endorsed?(proposal, current_user)
+        if current_user


I'd rewrite this like:

return false unless current_user user_group_endorsements = current_user.user_groups.verified.all? { |user_group| proposal.endorsed_by?(current_user, user_group) } user_group_endorsements & proposal.endorsed_by?(current_user)

oriolgual · 2018-02-22T14:39:20Z

decidim-proposals/app/helpers/decidim/proposals/proposal_endorsements_helper.rb

@@ -68,6 +75,36 @@ def endorsement_button(proposal, from_proposals_list, btn_label = nil, user_grou
                                                                                    current_endorsement_url: current_endorsement_url,
                                                                                    endorse_label: endorse_label, unendorse_label: unendorse_label }
      end
+
+      def fully_endorsed?(proposal, current_user)


Add some docs please

oriolgual · 2018-02-22T14:41:10Z

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

@@ -6,6 +6,7 @@ module Proposals
    class ProposalEndorsementsController < Decidim::Proposals::ApplicationController
      helper_method :proposal

+      protect_from_forgery except: [:destroy, :create]


Is this really necessary? It seems that with this another site could trigger a creation or deletion of an endorsement (maybe I'm wrong)

This has been added when applying new designs because the CTA is no longer a button with a form, but a whole div that on click executes a jquery method using vanilla $.ajax. This ajax call does not have any authentication_token to be used when invoking :destroy or :create.

I'll try to rework this.

I've managed to supply the auth token through the url so now protection from forgery has been re-enabled.

oriolgual · 2018-02-22T14:42:59Z

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

+        <%= render partial: "endorsements_count", locals: { proposal: @proposal, fully_endorsed: fully_endorsed } %>
+        <% if current_settings.endorsements_blocked? %>
+          <%= content_tag :span, t('.endorse'), class: "card__button button #{endorsement_button_classes(false)} disabled", disabled: true, title: t('.endorse') %>
+        <% elsif not current_user %>


elsif not current_user it's kinds complicated, maybe we can use elsif current_user and swap the block with the next one?

Yeah, done.

…y_new_design

…se of too much hurry for finishing the task :(.

All requested changes applied.

tramuntanal · 2018-02-26T12:35:07Z

All tests passing, automatic merging ready, @decidim/lot-core @decidim/product ready to be merged.

tramuntanal · 2018-02-27T09:59:52Z

@decidim/lot-core Should I click the "Squash and merge" button? normally it's you that do it. I fear that if we don't do it soon new conflicts will arise 😨

mrcasals · 2018-02-27T10:08:01Z

@tramuntanal done!

tramuntanal and others added 4 commits February 14, 2018 16:38

[WIP] Partial for endorsements row extracted + specs for show_endorse…

686ea1f

…ments_card?.

[LAYOUT] New design for proposal endorsements, covers: not endorsed, …

9891ab5

…already endorsed.

[FIX] Undo bad commit that addded unwanted class to vote button.

947cc71

[LAYOUT] New design for endorsements disabled and blocked.

b7abc37

ghost assigned tramuntanal Feb 14, 2018

ghost added the in-progress label Feb 14, 2018

Apply rubocop automatic corrections.

fc75e52

Oliver Valls added 2 commits February 14, 2018 16:59

Remove unused i18n en keys.

4f3aba3

[DOC] Added CHANGELOG.md entry.

c5f44b4

tramuntanal mentioned this pull request Feb 14, 2018

Adhesion to proposals decidim-archive/design#156

Open

login required page

5cce3e4

ghost assigned Crashillo Feb 14, 2018

Crashillo removed their assignment Feb 14, 2018

tramuntanal assigned Crashillo Feb 15, 2018

Crashillo added the help welcome label Feb 15, 2018

mrcasals mentioned this pull request Feb 15, 2018

Fix a couple of random failing tests #2738

Merged

[LAYOUT] Apply endorsements / loging required.

683fc4d

Crashillo removed the help welcome label Feb 16, 2018

Crashillo added 3 commits February 16, 2018 14:43

update author data partial

ad36617

identity selector

c218307

Merge branch 'master' into feature/2728-endorsement_to_proposals_appl…

3334e9d

…y_new_design

Oliver Valls and others added 2 commits February 20, 2018 14:23

[FIX|I18N] Normalize translations.

24b4f58

[FIX] Js lint, format and style corrections.

c1662d3

Oliver Valls added 2 commits February 21, 2018 13:33

[I18N] In select identity dialog, change "Ok" button content for "Done".

085f758

Remove the 'Cancel' button as it does the same as the 'Done' button (…

a55ba90

…in the select identity dialog).

Crashillo and others added 7 commits February 21, 2018 16:26

generalization button groups

966ed16

[FIX] Endorsement button displays as actuable for anyone it asks logi…

e7e03cd

…n or registration when clicking if participant is not.

[FIX] When the window to select your identity is opened, after select…

72e1b65

…ing the identities and click "Done" the window should close but remained open.

[FIX] Render a "See less" button, render list of endorsements comma s…

6e775af

…eparated.

Rubocop automatic corrections.

4448e62

[FIX|I18N] Remove unused i18n key.

6e23e34

[FIX|I18N] Remove another unused i18n key.

2c6470a

oriolgual previously requested changes Feb 22, 2018

View reviewed changes

Oliver Valls added 4 commits February 23, 2018 09:16

[REFACTOR] For better readability in template.

4124876

[REFACTOR] Better readability.

df18259

Merge branch 'master' into feature/2728-endorsement_to_proposals_appl…

61f7d7f

…y_new_design

[FIX] Re-enable the use of authenticity token which was removed becau…

4163638

…se of too much hurry for finishing the task :(.

oriolgual approved these changes Feb 26, 2018

View reviewed changes

mrcasals merged commit e08dc58 into master Feb 27, 2018

mrcasals deleted the feature/2728-endorsement_to_proposals_apply_new_design branch February 27, 2018 10:07

ghost removed the in-progress label Feb 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/2728 endorsement to proposals apply new design #2733

Feature/2728 endorsement to proposals apply new design #2733

tramuntanal commented Feb 14, 2018 •

edited

Loading

codecov bot commented Feb 14, 2018 •

edited

Loading

Crashillo commented Feb 14, 2018

tramuntanal commented Feb 15, 2018

Crashillo commented Feb 15, 2018

mrcasals commented Feb 15, 2018

mrcasals commented Feb 15, 2018

tramuntanal commented Feb 15, 2018

tramuntanal commented Feb 15, 2018

oriolgual commented Feb 16, 2018

tramuntanal commented Feb 20, 2018 •

edited

Loading

arnaumonty commented Feb 21, 2018 •

edited by tramuntanal

Loading

xabier commented Feb 21, 2018 •

edited by tramuntanal

Loading

xabier commented Feb 21, 2018

xabier commented Feb 21, 2018

tramuntanal commented Feb 22, 2018

oriolgual Feb 22, 2018

tramuntanal Feb 23, 2018

oriolgual Feb 22, 2018

oriolgual Feb 22, 2018

tramuntanal Feb 22, 2018

tramuntanal Feb 26, 2018

oriolgual Feb 22, 2018

tramuntanal Feb 23, 2018

tramuntanal commented Feb 26, 2018

tramuntanal commented Feb 27, 2018

mrcasals commented Feb 27, 2018

Feature/2728 endorsement to proposals apply new design #2733

Feature/2728 endorsement to proposals apply new design #2733

Conversation

tramuntanal commented Feb 14, 2018 • edited Loading

🎩 What? Why?

📌 Related Issues

📋 Subtasks

codecov bot commented Feb 14, 2018 • edited Loading

Codecov Report

Crashillo commented Feb 14, 2018

tramuntanal commented Feb 15, 2018

Crashillo commented Feb 15, 2018

mrcasals commented Feb 15, 2018

mrcasals commented Feb 15, 2018

tramuntanal commented Feb 15, 2018

tramuntanal commented Feb 15, 2018

oriolgual commented Feb 16, 2018

tramuntanal commented Feb 20, 2018 • edited Loading

arnaumonty commented Feb 21, 2018 • edited by tramuntanal Loading

xabier commented Feb 21, 2018 • edited by tramuntanal Loading

xabier commented Feb 21, 2018

xabier commented Feb 21, 2018

tramuntanal commented Feb 22, 2018

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

tramuntanal commented Feb 26, 2018

tramuntanal commented Feb 27, 2018

mrcasals commented Feb 27, 2018

tramuntanal commented Feb 14, 2018 •

edited

Loading

codecov bot commented Feb 14, 2018 •

edited

Loading

tramuntanal commented Feb 20, 2018 •

edited

Loading

arnaumonty commented Feb 21, 2018 •

edited by tramuntanal

Loading

xabier commented Feb 21, 2018 •

edited by tramuntanal

Loading