From 9b908d72641728457d6357ea50a4c2c191559fac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 11 Aug 2022 01:24:08 +0200 Subject: [PATCH 1/4] Extract component to render account permissions We were using similar code in four different places; six, if we count the welcome pages seeds. Reducing duplication in the pages seeds is a bit tricky because administrators are supposed to edit their content and might remove the HTML class we use to define styles. However, we can share the code everywhere else. Note that there's a bug in the application since we show that level 2 users cannot vote for budget projects but we give them permission to do so in the abilities model. We're keeping the same behavior after this refactoring but we might change it in the future. --- .../stylesheets/account/permissions_list.scss | 23 ++++++++++++++++ app/assets/stylesheets/admin.scss | 26 ------------------- app/assets/stylesheets/application.scss | 1 + app/assets/stylesheets/layout.scss | 24 ----------------- .../permissions_list_component.html.erb | 20 ++++++++++++++ .../account/permissions_list_component.rb | 7 +++++ .../management/users_controller.rb | 2 +- app/views/account/show.html.erb | 21 +-------------- .../management/_user_permissions.html.erb | 15 +---------- .../invalid_document.html.erb | 2 +- .../document_verifications/new.html.erb | 2 +- .../document_verifications/verified.html.erb | 2 +- .../email_verifications/sent.html.erb | 2 +- app/views/management/users/new.html.erb | 2 +- app/views/management/users/show.html.erb | 2 +- app/views/verification/letter/new.html.erb | 8 +----- app/views/verification/residence/new.html.erb | 7 +---- 17 files changed, 62 insertions(+), 104 deletions(-) create mode 100644 app/assets/stylesheets/account/permissions_list.scss create mode 100644 app/components/account/permissions_list_component.html.erb create mode 100644 app/components/account/permissions_list_component.rb diff --git a/app/assets/stylesheets/account/permissions_list.scss b/app/assets/stylesheets/account/permissions_list.scss new file mode 100644 index 000000000..2e476d6b0 --- /dev/null +++ b/app/assets/stylesheets/account/permissions_list.scss @@ -0,0 +1,23 @@ +.permissions-list { + list-style-type: none; + margin-bottom: 0; + margin-left: 0; + + li { + font-size: $small-font-size; + margin-bottom: $line-height / 2; + + span { + color: $text-medium; + font-size: rem-calc(12); + } + + .icon-check { + color: $check; + } + + .icon-x { + color: $delete; + } + } +} diff --git a/app/assets/stylesheets/admin.scss b/app/assets/stylesheets/admin.scss index 0eae30b8b..5b2a19228 100644 --- a/app/assets/stylesheets/admin.scss +++ b/app/assets/stylesheets/admin.scss @@ -525,32 +525,6 @@ code { // 05. Management // -------------- -.user-permissions { - - ul { - list-style-type: none; - margin-left: 0; - } - - li { - font-size: rem-calc(14); - margin-bottom: rem-calc(12); - - span { - color: $text-medium; - font-size: rem-calc(12); - } - - .icon-check { - color: $check; - } - - .icon-x { - color: $delete; - } - } -} - .account-info, .login-as { background-color: $highlight; diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index 7d47c5015..de9749210 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -41,6 +41,7 @@ @import "stats"; @import "sticky_overrides"; @import "tags"; +@import "account/**/*"; @import "admin/**/*"; @import "budgets/**/*"; @import "debates/**/*"; diff --git a/app/assets/stylesheets/layout.scss b/app/assets/stylesheets/layout.scss index 83a9f6cd6..0197288f4 100644 --- a/app/assets/stylesheets/layout.scss +++ b/app/assets/stylesheets/layout.scss @@ -1278,30 +1278,6 @@ form { font-size: rem-calc(12); } } - - ul { - list-style-type: none; - margin-bottom: 0; - margin-left: 0; - } - - li { - font-size: $small-font-size; - margin-bottom: $line-height / 2; - - span { - color: $text-medium; - font-size: rem-calc(12); - } - - .icon-check { - color: $check; - } - - .icon-x { - color: $delete; - } - } } .notifications-list { diff --git a/app/components/account/permissions_list_component.html.erb b/app/components/account/permissions_list_component.html.erb new file mode 100644 index 000000000..ed3b2fe37 --- /dev/null +++ b/app/components/account/permissions_list_component.html.erb @@ -0,0 +1,20 @@ + diff --git a/app/components/account/permissions_list_component.rb b/app/components/account/permissions_list_component.rb new file mode 100644 index 000000000..d4ce55548 --- /dev/null +++ b/app/components/account/permissions_list_component.rb @@ -0,0 +1,7 @@ +class Account::PermissionsListComponent < ApplicationComponent + attr_reader :user + + def initialize(user) + @user = user + end +end diff --git a/app/controllers/management/users_controller.rb b/app/controllers/management/users_controller.rb index 75a76d2aa..43f23f40e 100644 --- a/app/controllers/management/users_controller.rb +++ b/app/controllers/management/users_controller.rb @@ -1,6 +1,6 @@ class Management::UsersController < Management::BaseController def new - @user = User.new(user_params) + @user = User.new(user_params.merge(verified_at: Time.current)) end def create diff --git a/app/views/account/show.html.erb b/app/views/account/show.html.erb index 22a1462cd..34c9d104c 100644 --- a/app/views/account/show.html.erb +++ b/app/views/account/show.html.erb @@ -93,26 +93,7 @@

<%= t("account.show.user_permission_info") %>

- + <%= render Account::PermissionsListComponent.new(current_user) %>

<%= t("account.show.user_permission_verify") %> diff --git a/app/views/management/_user_permissions.html.erb b/app/views/management/_user_permissions.html.erb index 37556f138..d1ab68171 100644 --- a/app/views/management/_user_permissions.html.erb +++ b/app/views/management/_user_permissions.html.erb @@ -1,18 +1,5 @@ -<%# # Parameters: - # message: A string explaining the permissions - # permissions: An array of symbols containing the permissions - # (can be :debates, :proposal, :support_proposal, :votes) %>

-

<%= message %>

- - + <%= render Account::PermissionsListComponent.new(user) %>
diff --git a/app/views/management/document_verifications/invalid_document.html.erb b/app/views/management/document_verifications/invalid_document.html.erb index 8de89d6f9..086145d98 100644 --- a/app/views/management/document_verifications/invalid_document.html.erb +++ b/app/views/management/document_verifications/invalid_document.html.erb @@ -8,7 +8,7 @@ <%= render "management/user_permissions", message: t("management.document_verifications.not_in_census_info"), - permissions: [:debates, :proposal] %> + user: User.new %>

<%= sanitize(t("management.document_verifications.has_no_account", diff --git a/app/views/management/document_verifications/new.html.erb b/app/views/management/document_verifications/new.html.erb index a816520c5..ee9cd1267 100644 --- a/app/views/management/document_verifications/new.html.erb +++ b/app/views/management/document_verifications/new.html.erb @@ -4,7 +4,7 @@ <%= render "management/user_permissions", message: t("management.document_verifications.in_census_has_following_permissions"), - permissions: [:debates, :proposal, :support_proposal] %> + user: @document_verification.user %> <%= form_for @document_verification, as: :document_verification, diff --git a/app/views/management/document_verifications/verified.html.erb b/app/views/management/document_verifications/verified.html.erb index b20af33d3..1365c9910 100644 --- a/app/views/management/document_verifications/verified.html.erb +++ b/app/views/management/document_verifications/verified.html.erb @@ -4,6 +4,6 @@ <%= render "management/user_permissions", message: t("management.document_verifications.in_census_has_following_permissions"), - permissions: [:debates, :proposal, :support_proposal, :votes] %> + user: @document_verification.user %> <%= t("management.print_info") %> diff --git a/app/views/management/email_verifications/sent.html.erb b/app/views/management/email_verifications/sent.html.erb index 9eae46bff..7e7ca2e60 100644 --- a/app/views/management/email_verifications/sent.html.erb +++ b/app/views/management/email_verifications/sent.html.erb @@ -4,7 +4,7 @@ <%= render "management/user_permissions", message: t("management.email_verifications.document_found_in_census"), - permissions: [:debates, :proposal, :support_proposal, :votes] %> + user: @email_verification.user %>

<%= t("management.print_info") %> diff --git a/app/views/management/users/new.html.erb b/app/views/management/users/new.html.erb index 632ce1cf7..6c7baad7d 100644 --- a/app/views/management/users/new.html.erb +++ b/app/views/management/users/new.html.erb @@ -25,6 +25,6 @@

<%= render "management/user_permissions", message: t("management.document_verifications.in_census_has_following_permissions"), - permissions: [:debates, :proposal, :support_proposal, :votes] %> + user: @user %>
diff --git a/app/views/management/users/show.html.erb b/app/views/management/users/show.html.erb index 89bf3bba5..86bc4a377 100644 --- a/app/views/management/users/show.html.erb +++ b/app/views/management/users/show.html.erb @@ -6,6 +6,6 @@ <%= render "management/user_permissions", message: t("management.document_verifications.in_census_has_following_permissions"), - permissions: [:debates, :proposal, :support_proposal, :votes] %> + user: @user %> <%= t("management.print_info") %> diff --git a/app/views/verification/letter/new.html.erb b/app/views/verification/letter/new.html.erb index a60a762d7..60b38f9ae 100644 --- a/app/views/verification/letter/new.html.erb +++ b/app/views/verification/letter/new.html.erb @@ -24,15 +24,9 @@

<%= t("verification.letter.new.title") %>

-

<%= t("verification.letter.new.user_permission_info") %>

- + <%= render Account::PermissionsListComponent.new(current_user) %>
<%= link_to t("verification.letter.new.go_to_index"), root_path, class: "button warning" %> diff --git a/app/views/verification/residence/new.html.erb b/app/views/verification/residence/new.html.erb index 817408bc6..8cc5c745a 100644 --- a/app/views/verification/residence/new.html.erb +++ b/app/views/verification/residence/new.html.erb @@ -24,12 +24,7 @@

<%= t("verification.user_permission_info") %>

- + <%= render Account::PermissionsListComponent.new(User.new(level_two_verified_at: Time.current)) %>
<%= form_for @residence, as: "residence", url: residence_path do |f| %> From 0c629776685a70c5b9ad2da66dee39ee1a0991c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 11 Aug 2022 01:43:45 +0200 Subject: [PATCH 2/4] Extract method to define permissions This way we simplify the ERB code. Due to the bug mentioned in the previous commit, we're keeping the original code instead of using `can?` to check permissions. --- .../permissions_list_component.html.erb | 29 +++++++------------ .../account/permissions_list_component.rb | 11 +++++++ 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/app/components/account/permissions_list_component.html.erb b/app/components/account/permissions_list_component.html.erb index ed3b2fe37..2d7c63687 100644 --- a/app/components/account/permissions_list_component.html.erb +++ b/app/components/account/permissions_list_component.html.erb @@ -1,20 +1,13 @@ diff --git a/app/components/account/permissions_list_component.rb b/app/components/account/permissions_list_component.rb index d4ce55548..6120ea3f7 100644 --- a/app/components/account/permissions_list_component.rb +++ b/app/components/account/permissions_list_component.rb @@ -4,4 +4,15 @@ class Account::PermissionsListComponent < ApplicationComponent def initialize(user) @user = user end + + private + + def permissions + { + t("verification.user_permission_debates") => true, + t("verification.user_permission_proposal") => true, + t("verification.user_permission_support_proposal") => user.level_two_or_three_verified?, + t("verification.user_permission_votes") => user.level_three_verified? + } + end end From 66c258355707e9bc6d66aebcf145fd735ae30a31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 11 Aug 2022 02:35:06 +0200 Subject: [PATCH 3/4] Render icons using CSS in user permissions As mentioned in commit 925f04e3f, icon classes make screen readers announce strange symbols and aren't properly displayed for people who have changed their preferred font family. --- .../stylesheets/account/permissions_list.scss | 22 +++++++++++++------ .../permissions_list_component.html.erb | 8 +------ .../account/permissions_list_component.rb | 8 +++++++ .../permissions_list_component_spec.rb | 10 +++++++++ 4 files changed, 34 insertions(+), 14 deletions(-) create mode 100644 spec/components/account/permissions_list_component_spec.rb diff --git a/app/assets/stylesheets/account/permissions_list.scss b/app/assets/stylesheets/account/permissions_list.scss index 2e476d6b0..fbd0a5043 100644 --- a/app/assets/stylesheets/account/permissions_list.scss +++ b/app/assets/stylesheets/account/permissions_list.scss @@ -7,17 +7,25 @@ font-size: $small-font-size; margin-bottom: $line-height / 2; - span { - color: $text-medium; - font-size: rem-calc(12); + &::before { + font-size: 0.9em; + margin-#{$global-right}: 0.2em; } - .icon-check { - color: $check; + &.permission-allowed { + @include has-fa-icon(check, solid); + + &::before { + color: $check; + } } - .icon-x { - color: $delete; + &.permission-denied { + @include has-fa-icon(times, solid); + + &::before { + color: $delete; + } } } } diff --git a/app/components/account/permissions_list_component.html.erb b/app/components/account/permissions_list_component.html.erb index 2d7c63687..055c9a922 100644 --- a/app/components/account/permissions_list_component.html.erb +++ b/app/components/account/permissions_list_component.html.erb @@ -1,12 +1,6 @@
    <% permissions.each do |text, allowed| %> -
  • - <% if allowed %> - - <% else %> - - <% end %> - +
  • <%= text %>
    • <% end %> diff --git a/app/components/account/permissions_list_component.rb b/app/components/account/permissions_list_component.rb index 6120ea3f7..619ef8b5f 100644 --- a/app/components/account/permissions_list_component.rb +++ b/app/components/account/permissions_list_component.rb @@ -15,4 +15,12 @@ class Account::PermissionsListComponent < ApplicationComponent t("verification.user_permission_votes") => user.level_three_verified? } end + + def allowed_class(allowed) + if allowed + "permission-allowed" + else + "permission-denied" + end + end end diff --git a/spec/components/account/permissions_list_component_spec.rb b/spec/components/account/permissions_list_component_spec.rb new file mode 100644 index 000000000..864a21bd5 --- /dev/null +++ b/spec/components/account/permissions_list_component_spec.rb @@ -0,0 +1,10 @@ +require "rails_helper" + +describe Account::PermissionsListComponent do + it "adds different classes for actions that can and cannot be performed" do + render_inline Account::PermissionsListComponent.new(User.new) + + expect(page).to have_css "li.permission-allowed", text: "Participate in debates" + expect(page).to have_css "li.permission-denied", text: "Support proposals" + end +end From 2bac80215f49e90e889e279f95bbb0a03ac7336f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 11 Aug 2022 14:30:55 +0200 Subject: [PATCH 4/4] Add permissions hint for visually impaired people We were displaying an icon showing that certain actions can't be performed. However, people who can't see the icons were hearing that they _can_ perform certain actions while the opposite is true. We've considered other options to solve this problem. One was to split the list in two: actions that can be performed and actions that can't be performed. It was tricky because in some cases we're listing that actions that can be performed now and in other cases we're displaying the actions that people will be able to perform once they verify their account. Another option was to include the word "Cannot" as a prefix instead of "Additional verification needed". We haven't done so because, while in English we say "cannot do this thing", in other languages they say "this thing cannot do". So we've gone with a solution where people hearing what's on the screen know what's going on and we don't have to make big changes in the code. --- app/components/account/permissions_list_component.html.erb | 4 ++++ config/locales/en/verification.yml | 1 + config/locales/es/verification.yml | 1 + spec/components/account/permissions_list_component_spec.rb | 7 +++++++ 4 files changed, 13 insertions(+) diff --git a/app/components/account/permissions_list_component.html.erb b/app/components/account/permissions_list_component.html.erb index 055c9a922..028976b03 100644 --- a/app/components/account/permissions_list_component.html.erb +++ b/app/components/account/permissions_list_component.html.erb @@ -1,6 +1,10 @@
      <% permissions.each do |text, allowed| %>
    • + <% unless allowed %> + <%= t("verification.verification_needed") %> + <% end %> + <%= text %>
      • <% end %> diff --git a/config/locales/en/verification.yml b/config/locales/en/verification.yml index 276d23791..7b6ac56ee 100644 --- a/config/locales/en/verification.yml +++ b/config/locales/en/verification.yml @@ -92,6 +92,7 @@ en: user_permission_proposal: Create new proposals user_permission_support_proposal: Support proposals user_permission_votes: Vote for budget projects + verification_needed: Additional verification needed verified_user: form: submit_button: Send code diff --git a/config/locales/es/verification.yml b/config/locales/es/verification.yml index c3620df4c..4f52a3d5a 100644 --- a/config/locales/es/verification.yml +++ b/config/locales/es/verification.yml @@ -92,6 +92,7 @@ es: user_permission_proposal: Crear nuevas propuestas user_permission_support_proposal: Apoyar propuestas user_permission_votes: Votar proyectos en los presupuestos participativos + verification_needed: Verificación adicional necesaria verified_user: form: submit_button: Enviar código diff --git a/spec/components/account/permissions_list_component_spec.rb b/spec/components/account/permissions_list_component_spec.rb index 864a21bd5..c8d30f5a7 100644 --- a/spec/components/account/permissions_list_component_spec.rb +++ b/spec/components/account/permissions_list_component_spec.rb @@ -7,4 +7,11 @@ describe Account::PermissionsListComponent do expect(page).to have_css "li.permission-allowed", text: "Participate in debates" expect(page).to have_css "li.permission-denied", text: "Support proposals" end + + it "adds a hint when an action cannot be performed" do + render_inline Account::PermissionsListComponent.new(User.new) + + expect(page).to have_css "li", exact_text: "Additional verification needed Support proposals", normalize_ws: true + expect(page).to have_css "li", exact_text: "Participate in debates", normalize_ws: true + end end