From 315c57929a0a24e5a24651356808229cf4416389 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juanjo=20Baza=CC=81n?= Date: Thu, 15 Jun 2017 13:09:45 +0200 Subject: [PATCH 1/2] allows managers to create users without email allows managers to create users without email hides email preferences from account page for email-less users prevents email delivery to users with no email adds spec for user creation from management adds specs for user's email requirement adds spec for no deliveries if no email --- .../management/users_controller.rb | 28 ++++++- app/mailers/mailer.rb | 49 ++++++++--- app/models/user.rb | 2 +- app/views/account/show.html.erb | 84 ++++++++++--------- app/views/management/users/new.html.erb | 2 +- app/views/management/users/show.html.erb | 6 +- config/locales/management.en.yml | 2 + config/locales/management.es.yml | 2 + spec/features/emails_spec.rb | 11 +++ .../features/management/managed_users_spec.rb | 30 ++++++- spec/features/management/users_spec.rb | 30 ++++++- spec/models/user_spec.rb | 25 ++++++ 12 files changed, 210 insertions(+), 61 deletions(-) diff --git a/app/controllers/management/users_controller.rb b/app/controllers/management/users_controller.rb index 8d3d148fa..d46360d89 100644 --- a/app/controllers/management/users_controller.rb +++ b/app/controllers/management/users_controller.rb @@ -6,7 +6,13 @@ class Management::UsersController < Management::BaseController def create @user = User.new(user_params) - @user.skip_password_validation = true + + if @user.email.blank? + user_without_email + else + user_with_email + end + @user.terms_of_service = '1' @user.residence_verified_at = Time.current @user.verified_at = Time.current @@ -40,4 +46,24 @@ class Management::UsersController < Management::BaseController session[:document_number] = nil end + def user_without_email + new_password = "aAbcdeEfghiJkmnpqrstuUvwxyz23456789$!".split('').sample(10).join('') + @user.password = new_password + @user.password_confirmation = new_password + + @user.email = nil + @user.confirmed_at = Time.current + + @user.newsletter = false + @user.email_on_proposal_notification = false + @user.email_digest = false + @user.email_on_direct_message = false + @user.email_on_comment = false + @user.email_on_comment_reply = false + end + + def user_with_email + @user.skip_password_validation = true + end + end diff --git a/app/mailers/mailer.rb b/app/mailers/mailer.rb index 7e8970c61..c12f12087 100644 --- a/app/mailers/mailer.rb +++ b/app/mailers/mailer.rb @@ -1,4 +1,6 @@ class Mailer < ApplicationMailer + after_action :prevent_delivery_to_users_without_email + helper :text_with_links helper :mailer helper :users @@ -6,8 +8,10 @@ class Mailer < ApplicationMailer def comment(comment) @comment = comment @commentable = comment.commentable + @email_to = @commentable.author.email + with_user(@commentable.author) do - mail(to: @commentable.author.email, subject: t('mailers.comment.subject', commentable: t("activerecord.models.#{@commentable.class.name.underscore}", count: 1).downcase)) if @commentable.present? && @commentable.author.present? + mail(to: @email_to, subject: t('mailers.comment.subject', commentable: t("activerecord.models.#{@commentable.class.name.underscore}", count: 1).downcase)) if @commentable.present? && @commentable.author.present? end end @@ -16,96 +20,108 @@ class Mailer < ApplicationMailer @commentable = @reply.commentable parent = Comment.find(@reply.parent_id) @recipient = parent.author + @email_to = @recipient.email + with_user(@recipient) do - mail(to: @recipient.email, subject: t('mailers.reply.subject')) if @commentable.present? && @recipient.present? + mail(to: @email_to, subject: t('mailers.reply.subject')) if @commentable.present? && @recipient.present? end end def email_verification(user, recipient, token, document_type, document_number) @user = user - @recipient = recipient + @email_to = recipient @token = token @document_type = document_type @document_number = document_number with_user(user) do - mail(to: @recipient, subject: t('mailers.email_verification.subject')) + mail(to: @email_to, subject: t('mailers.email_verification.subject')) end end def unfeasible_spending_proposal(spending_proposal) @spending_proposal = spending_proposal @author = spending_proposal.author + @email_to = @author.email with_user(@author) do - mail(to: @author.email, subject: t('mailers.unfeasible_spending_proposal.subject', code: @spending_proposal.code)) + mail(to: @email_to, subject: t('mailers.unfeasible_spending_proposal.subject', code: @spending_proposal.code)) end end def direct_message_for_receiver(direct_message) @direct_message = direct_message @receiver = @direct_message.receiver + @email_to = @receiver.email with_user(@receiver) do - mail(to: @receiver.email, subject: t('mailers.direct_message_for_receiver.subject')) + mail(to: @email_to, subject: t('mailers.direct_message_for_receiver.subject')) end end def direct_message_for_sender(direct_message) @direct_message = direct_message @sender = @direct_message.sender + @email_to = @sender.email with_user(@sender) do - mail(to: @sender.email, subject: t('mailers.direct_message_for_sender.subject')) + mail(to: @email_to, subject: t('mailers.direct_message_for_sender.subject')) end end def proposal_notification_digest(user, notifications) @notifications = notifications + @email_to = user.email with_user(user) do - mail(to: user.email, subject: t('mailers.proposal_notification_digest.title', org_name: Setting['org_name'])) + mail(to: @email_to, subject: t('mailers.proposal_notification_digest.title', org_name: Setting['org_name'])) end end def user_invite(email) + @email_to = email + I18n.with_locale(I18n.default_locale) do - mail(to: email, subject: t('mailers.user_invite.subject', org_name: Setting["org_name"])) + mail(to: @email_to, subject: t('mailers.user_invite.subject', org_name: Setting["org_name"])) end end def budget_investment_created(investment) @investment = investment + @email_to = @investment.author.email with_user(@investment.author) do - mail(to: @investment.author.email, subject: t('mailers.budget_investment_created.subject')) + mail(to: @email_to, subject: t('mailers.budget_investment_created.subject')) end end def budget_investment_unfeasible(investment) @investment = investment @author = investment.author + @email_to = @author.email with_user(@author) do - mail(to: @author.email, subject: t('mailers.budget_investment_unfeasible.subject', code: @investment.code)) + mail(to: @email_to, subject: t('mailers.budget_investment_unfeasible.subject', code: @investment.code)) end end def budget_investment_selected(investment) @investment = investment @author = investment.author + @email_to = @author.email with_user(@author) do - mail(to: @author.email, subject: t('mailers.budget_investment_selected.subject', code: @investment.code)) + mail(to: @email_to, subject: t('mailers.budget_investment_selected.subject', code: @investment.code)) end end def budget_investment_unselected(investment) @investment = investment @author = investment.author + @email_to = @author.email with_user(@author) do - mail(to: @author.email, subject: t('mailers.budget_investment_unselected.subject', code: @investment.code)) + mail(to: @email_to, subject: t('mailers.budget_investment_unselected.subject', code: @investment.code)) end end @@ -116,4 +132,11 @@ class Mailer < ApplicationMailer block.call end end + + def prevent_delivery_to_users_without_email + if @email_to.blank? + mail.perform_deliveries = false + end + end + end diff --git a/app/models/user.rb b/app/models/user.rb index 95b915b02..1b4ed81f1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -247,7 +247,7 @@ class User < ActiveRecord::Base end def email_required? - !erased? + !erased? && unverified? end def locale diff --git a/app/views/account/show.html.erb b/app/views/account/show.html.erb index 247aa6024..a0d9f1693 100644 --- a/app/views/account/show.html.erb +++ b/app/views/account/show.html.erb @@ -40,52 +40,54 @@ <% end %> -

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

+ <% if @account.email.present? %> +

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

-
- <%= f.label :email_on_comment do %> - <%= f.check_box :email_on_comment, title: t('account.show.email_on_comment_label'), label: false %> - - <%= t("account.show.email_on_comment_label") %> - - <% end %> -
+
+ <%= f.label :email_on_comment do %> + <%= f.check_box :email_on_comment, title: t('account.show.email_on_comment_label'), label: false %> + + <%= t("account.show.email_on_comment_label") %> + + <% end %> +
-
- <%= f.label :email_on_comment_reply do %> - <%= f.check_box :email_on_comment_reply, title: t('account.show.email_on_comment_reply_label'), label: false %> - - <%= t("account.show.email_on_comment_reply_label") %> - - <% end %> -
+
+ <%= f.label :email_on_comment_reply do %> + <%= f.check_box :email_on_comment_reply, title: t('account.show.email_on_comment_reply_label'), label: false %> + + <%= t("account.show.email_on_comment_reply_label") %> + + <% end %> +
-
- <%= f.label :email_newsletter_subscribed do %> - <%= f.check_box :newsletter, title: t('account.show.subscription_to_website_newsletter_label'), label: false %> - - <%= t("account.show.subscription_to_website_newsletter_label") %> - - <% end %> -
+
+ <%= f.label :email_newsletter_subscribed do %> + <%= f.check_box :newsletter, title: t('account.show.subscription_to_website_newsletter_label'), label: false %> + + <%= t("account.show.subscription_to_website_newsletter_label") %> + + <% end %> +
-
- <%= f.label :email_digest do %> - <%= f.check_box :email_digest, title: t('account.show.email_digest_label'), label: false %> - - <%= t("account.show.email_digest_label") %> - - <% end %> -
+
+ <%= f.label :email_digest do %> + <%= f.check_box :email_digest, title: t('account.show.email_digest_label'), label: false %> + + <%= t("account.show.email_digest_label") %> + + <% end %> +
-
- <%= f.label :email_on_direct_message do %> - <%= f.check_box :email_on_direct_message, title: t('account.show.email_on_direct_message_label'), label: false %> - - <%= t("account.show.email_on_direct_message_label") %> - - <% end %> -
+
+ <%= f.label :email_on_direct_message do %> + <%= f.check_box :email_on_direct_message, title: t('account.show.email_on_direct_message_label'), label: false %> + + <%= t("account.show.email_on_direct_message_label") %> + + <% end %> +
+ <% end %> <% if @account.official_level == 1 %>
diff --git a/app/views/management/users/new.html.erb b/app/views/management/users/new.html.erb index d52597590..fd28a1248 100644 --- a/app/views/management/users/new.html.erb +++ b/app/views/management/users/new.html.erb @@ -13,7 +13,7 @@ label: t('management.username_label'), placeholder: t('management.username_label') %> <%= f.text_field :email, - label: t('management.email_label'), + label: t('management.users.email_optional_label'), placeholder: t('management.email_label') %>
<%= f.label t("management.date_of_birth") %> diff --git a/app/views/management/users/show.html.erb b/app/views/management/users/show.html.erb index ed78d1aea..ae29d9d73 100644 --- a/app/views/management/users/show.html.erb +++ b/app/views/management/users/show.html.erb @@ -1,4 +1,8 @@ -

<%= t("management.users.create_user_success_html", email: @user.email) %>

+<% if @user.email.blank? %> +

<%= t("management.users.autogenerated_password_html", password: @user.password) %>

+<% else %> +

<%= t("management.users.create_user_success_html", email: @user.email) %>

+<% end %> <%= render 'management/user_permissions', message: t("management.document_verifications.in_census_has_following_permissions"), diff --git a/config/locales/management.en.yml b/config/locales/management.en.yml index d8aaf826c..d2524990a 100644 --- a/config/locales/management.en.yml +++ b/config/locales/management.en.yml @@ -114,6 +114,8 @@ en: create_user_info: 'We will create an account with the following data:' create_user_submit: Create user create_user_success_html: We have sent an email to the email address %{email} in order to verify that it belongs to this user. It contains a link they have to click. Then they will have to set their access password before being able to log in to the website + autogenerated_password_html: "Autogenerated password is %{password}, you can change it in the 'My account' section of the web" + email_optional_label: Email (optional) erased_notice: User account deleted. erased_by_manager: "Deleted by manager: %{manager}" erase_account_link: Delete user diff --git a/config/locales/management.es.yml b/config/locales/management.es.yml index 10543798a..5ef4eee19 100644 --- a/config/locales/management.es.yml +++ b/config/locales/management.es.yml @@ -114,6 +114,8 @@ es: create_user_info: 'Procedemos a crear un usuario con la siguiente información:' create_user_submit: Crear usuario create_user_success_html: Hemos enviado un correo electrónico a %{email} para verificar que es suya. El correo enviado contiene un link que el usuario deberá pulsar. Entonces podrá seleccionar una clave de acceso, y entrar en la web de participación. + autogenerated_password_html: "Se ha asignado la contraseña %{password} a este usuario. Puede modificarla desde el apartado 'Mi cuenta' de la web." + email_optional_label: "Email (recomendado pero opcional)" erased_notice: Cuenta de usuario borrada. erased_by_manager: "Borrada por el manager: %{manager}" erase_account_link: Borrar cuenta diff --git a/spec/features/emails_spec.rb b/spec/features/emails_spec.rb index 47d0e7f1d..dd0d71f2c 100644 --- a/spec/features/emails_spec.rb +++ b/spec/features/emails_spec.rb @@ -370,4 +370,15 @@ feature 'Emails' do end end + + context "Users without email" do + scenario "should not receive emails", :js do + user = create(:user, :verified, email_on_comment: true) + proposal = create(:proposal, author: user) + user.update(email: nil) + comment_on(proposal) + + expect { open_last_email }.to raise_error "No email has been sent!" + end + end end diff --git a/spec/features/management/managed_users_spec.rb b/spec/features/management/managed_users_spec.rb index 311b8f5cd..d35dc35e1 100644 --- a/spec/features/management/managed_users_spec.rb +++ b/spec/features/management/managed_users_spec.rb @@ -84,7 +84,7 @@ feature 'Managed User' do end end - scenario "User is created as level three from scratch" do + scenario "User is created with email as level three from scratch" do login_as_manager visit management_document_verifications_path @@ -101,6 +101,7 @@ feature 'Managed User' do click_button 'Create user' expect(page).to have_content "We have sent an email" + expect(page).to_not have_content "Autogenerated password is" user = User.last within(".account-info") do @@ -110,6 +111,33 @@ feature 'Managed User' do expect(page).to have_content "#{user.document_number}" end end + + scenario "User is created without email as level three from scratch" do + login_as_manager + + visit management_document_verifications_path + fill_in 'document_verification_document_number', with: '12345678Z' + click_button 'Check' + + expect(page).to have_content "Please introduce the email used on the account" + + click_link 'Create a new account' + + fill_in 'user_username', with: 'peppa' + fill_in 'user_email', with: '' + + click_button 'Create user' + + expect(page).to_not have_content "We have sent an email" + expect(page).to have_content "Autogenerated password is" + + user = User.last + within(".account-info") do + expect(page).to have_content "Identified as" + expect(page).to have_content "#{user.username}" + expect(page).to have_content "#{user.document_number}" + end + end end scenario "Close the currently managed user session" do diff --git a/spec/features/management/users_spec.rb b/spec/features/management/users_spec.rb index 5d08ce711..e7c43f4a9 100644 --- a/spec/features/management/users_spec.rb +++ b/spec/features/management/users_spec.rb @@ -6,8 +6,7 @@ feature 'Users' do login_as_manager end - scenario 'Create a level 3 user from scratch' do - + scenario 'Create a level 3 user with email from scratch' do visit management_document_verifications_path fill_in 'document_verification_document_number', with: '12345678Z' click_button 'Check' @@ -23,6 +22,7 @@ feature 'Users' do click_button 'Create user' expect(page).to have_content "We have sent an email" + expect(page).to_not have_content "Autogenerated password is" user = User.find_by_email('pepe@gmail.com') @@ -46,6 +46,32 @@ feature 'Users' do expect(page).to have_content "Your account has been confirmed." end + scenario 'Create a level 3 user without email from scratch' do + visit management_document_verifications_path + fill_in 'document_verification_document_number', with: '12345678Z' + click_button 'Check' + + expect(page).to have_content "Please introduce the email used on the account" + + click_link 'Create a new account' + + fill_in 'user_username', with: 'Kelly Sue' + fill_in 'user_email', with: '' + select_date '31-December-1980', from: 'user_date_of_birth' + + click_button 'Create user' + + expect(page).to_not have_content "We have sent an email" + expect(page).to have_content "Autogenerated password is" + + user = User.find_by_username('Kelly Sue') + + expect(user).to be_level_three_verified + expect(user).to be_residence_verified + expect(user).to be_confirmed + expect(user.date_of_birth).to have_content (Date.new(1980,12,31)) + end + scenario 'Delete a level 2 user account from document verification page', :js do level_2_user = create(:user, :level_two, document_number: "12345678Z") diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 283a01c96..2bf09f378 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -632,4 +632,29 @@ describe User do end + describe "email_required?" do + it "is true for regular users" do + expect(subject.email_required?).to eq(true) + expect(create(:user, :hidden).email_required?).to eq(true) + end + + it "is false for erased users" do + user = create(:user) + user.erase + user.reload + + expect(user.email_required?).to eq(false) + end + + it "is false for verified users with no email" do + user = create(:user, + username: "Lois", + email: "", + verified_at: Time.current) + + expect(user).to be_valid + expect(user.email_required?).to eq(false) + end + end + end From 74783ffad26a8d69f398fcea49f99c5cf6d3b6e7 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Mon, 19 Jun 2017 18:13:00 +0200 Subject: [PATCH 2/2] Add missing :verified trait for User model factory --- spec/factories.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/spec/factories.rb b/spec/factories.rb index 7527a0ba1..6d9cb8ddd 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -42,6 +42,9 @@ FactoryGirl.define do confirmed_hide_at Time.current end + trait :verified do + verified_at Time.current + end end factory :identity do