From 3bc8fce5fa79c3ce9b0b8a3c3a47f8746470bd7a Mon Sep 17 00:00:00 2001 From: rgarcia Date: Thu, 17 May 2018 16:46:12 +0200 Subject: [PATCH 01/12] Process newsletters deliveries asynchronously We were seeing a timeout when queuing the 300.000+ emails to be delivered With this commit we are dealing asynchronously with this creations and responding promptly, to avoid the web timeout of 30 seconds --- app/controllers/admin/newsletters_controller.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/app/controllers/admin/newsletters_controller.rb b/app/controllers/admin/newsletters_controller.rb index 966556082..8b1c2010f 100644 --- a/app/controllers/admin/newsletters_controller.rb +++ b/app/controllers/admin/newsletters_controller.rb @@ -48,10 +48,7 @@ class Admin::NewslettersController < Admin::BaseController @newsletter = Newsletter.find(params[:id]) if @newsletter.valid? - @newsletter.list_of_recipient_emails.each do |recipient_email| - Mailer.newsletter(@newsletter, recipient_email).deliver_later - end - + @newsletter.delay.deliver @newsletter.update(sent_at: Time.current) flash[:notice] = t("admin.newsletters.send_success") else From 8960928a76becb4429ff1d4a374b26bc66ec437b Mon Sep 17 00:00:00 2001 From: rgarcia Date: Thu, 17 May 2018 16:48:08 +0200 Subject: [PATCH 02/12] Skip invalid emails when sending newsletter We were seeing an exception when sending some emails due to having invalid format With this commit we are skipping this invalid formatted emails when sending the newsletter --- app/models/newsletter.rb | 32 ++++++++++++++ spec/models/newsletter_spec.rb | 76 ++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/app/models/newsletter.rb b/app/models/newsletter.rb index 8c3a83cfa..e5aac95bf 100644 --- a/app/models/newsletter.rb +++ b/app/models/newsletter.rb @@ -20,9 +20,41 @@ class Newsletter < ActiveRecord::Base sent_at.nil? end + def deliver + run_at = first_batch_run_at + list_of_recipient_emails_in_batches.each do |recipient_emails| + recipient_emails.each do |recipient_email| + Mailer.delay(run_at: run_at).newsletter(self, recipient_email) + if valid_email?(recipient_email) + end + end + run_at += batch_interval + end + end + + def batch_size + 10000 + end + + def batch_interval + 20.minutes + end + + def first_batch_run_at + Time.current + end + + def list_of_recipient_emails_in_batches + list_of_recipient_emails.in_groups_of(batch_size, false) + end + private def validate_segment_recipient errors.add(:segment_recipient, :invalid) unless valid_segment_recipient? end + + def valid_email?(email) + email.match(/\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/i) + end end diff --git a/spec/models/newsletter_spec.rb b/spec/models/newsletter_spec.rb index bd42e9177..788326e88 100644 --- a/spec/models/newsletter_spec.rb +++ b/spec/models/newsletter_spec.rb @@ -63,4 +63,80 @@ describe Newsletter do expect(newsletter.list_of_recipient_emails).not_to include('erased_user@consul.dev') end end + + describe "#deliver" do + let!(:proposals) { Array.new(3) { create(:proposal) } } + let!(:debate) { create(:debate) } + + let!(:recipients) { proposals.map(&:author).map(&:email) } + let!(:newsletter) { create(:newsletter, segment_recipient: "proposal_authors") } + + before do + reset_mailer + Delayed::Worker.delay_jobs = true + end + + after do + Delayed::Worker.delay_jobs = false + end + + it "sends an email with the newsletter to every recipient" do + newsletter.deliver + + recipients.each do |recipient| + email = Mailer.newsletter(newsletter, recipient) + expect(email).to deliver_to(recipient) + end + + Delayed::Job.all.map(&:invoke_job) + expect(ActionMailer::Base.deliveries.count).to eq(3) + end + + it "sends emails in batches" do + allow(newsletter).to receive(:batch_size).and_return(1) + + newsletter.deliver + + expect(Delayed::Job.count).to eq(3) + end + + it "sends batches in time intervals" do + allow(newsletter).to receive(:batch_size).and_return(1) + allow(newsletter).to receive(:batch_interval).and_return(1.second) + allow(newsletter).to receive(:first_batch_run_at).and_return(Time.current) + + newsletter.deliver + + now = newsletter.first_batch_run_at + first_batch_run_at = now.change(usec: 0) + second_batch_run_at = (now + 1.second).change(usec: 0) + third_batch_run_at = (now + 2.seconds).change(usec: 0) + + expect(Delayed::Job.count).to eq(3) + expect(Delayed::Job.first.run_at.change(usec: 0)).to eq(first_batch_run_at) + expect(Delayed::Job.second.run_at.change(usec: 0)).to eq(second_batch_run_at) + expect(Delayed::Job.third.run_at.change(usec: 0)).to eq(third_batch_run_at) + end + + it "skips invalid emails" do + Proposal.destroy_all + + valid_email = "john@gmail.com" + invalid_email = "john@gmail..com" + + valid_email_user = create(:user, email: valid_email) + proposal = create(:proposal, author: valid_email_user) + + invalid_email_user = create(:user, email: invalid_email) + proposal = create(:proposal, author: invalid_email_user) + + newsletter.deliver + + expect(Activity.count).to eq(1) + expect(Activity.first.user_id).to eq(valid_email_user.id) + expect(Activity.first.action).to eq("email") + expect(Activity.first.actionable).to eq(newsletter) + end + + end end From 9264763ec5055f5eb75873080a98dfbd75e565bd Mon Sep 17 00:00:00 2001 From: rgarcia Date: Thu, 17 May 2018 16:53:47 +0200 Subject: [PATCH 03/12] Log newsletter emails sent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With this commit we are logging which emails have already received the newsletter This could be important if something goes wrong sending the newsletter, to be able to identify which users have already received the newsletter and be able to skip them We’ve had to add a new action to the Activity model (email) and add paranoia features to be able to deal gracefully with the default `with_hidden` scope in Activities[1] [1] https://github.com/AyuntamientoMadrid/consul/blob/master/app/models/acti vity.rb#L2 --- app/models/activity.rb | 2 +- app/models/newsletter.rb | 11 ++++++++++- ...20180517141120_add_hidden_at_to_newsletters.rb | 5 +++++ db/schema.rb | 1 + spec/models/activity_spec.rb | 2 ++ spec/models/newsletter_spec.rb | 15 +++++++++++++++ 6 files changed, 34 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20180517141120_add_hidden_at_to_newsletters.rb diff --git a/app/models/activity.rb b/app/models/activity.rb index 6c357d891..834f7721c 100644 --- a/app/models/activity.rb +++ b/app/models/activity.rb @@ -2,7 +2,7 @@ class Activity < ActiveRecord::Base belongs_to :actionable, -> { with_hidden }, polymorphic: true belongs_to :user, -> { with_hidden } - VALID_ACTIONS = %w(hide block restore valuate) + VALID_ACTIONS = %w(hide block restore valuate email) validates :action, inclusion: {in: VALID_ACTIONS} diff --git a/app/models/newsletter.rb b/app/models/newsletter.rb index e5aac95bf..b99e3e2ca 100644 --- a/app/models/newsletter.rb +++ b/app/models/newsletter.rb @@ -8,6 +8,9 @@ class Newsletter < ActiveRecord::Base validates_format_of :from, :with => /@/ + acts_as_paranoid column: :hidden_at + include ActsAsParanoidAliases + def list_of_recipient_emails UserSegments.user_segment_emails(segment_recipient) if valid_segment_recipient? end @@ -24,8 +27,9 @@ class Newsletter < ActiveRecord::Base run_at = first_batch_run_at list_of_recipient_emails_in_batches.each do |recipient_emails| recipient_emails.each do |recipient_email| - Mailer.delay(run_at: run_at).newsletter(self, recipient_email) if valid_email?(recipient_email) + Mailer.delay(run_at: run_at).newsletter(self, recipient_email) + log_delivery(recipient_email) end end run_at += batch_interval @@ -57,4 +61,9 @@ class Newsletter < ActiveRecord::Base def valid_email?(email) email.match(/\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/i) end + + def log_delivery(recipient_email) + user = User.where(email: recipient_email).first + Activity.log(user, :email, self) + end end diff --git a/db/migrate/20180517141120_add_hidden_at_to_newsletters.rb b/db/migrate/20180517141120_add_hidden_at_to_newsletters.rb new file mode 100644 index 000000000..5e1b4b8cf --- /dev/null +++ b/db/migrate/20180517141120_add_hidden_at_to_newsletters.rb @@ -0,0 +1,5 @@ +class AddHiddenAtToNewsletters < ActiveRecord::Migration + def change + add_column :newsletters, :hidden_at, :datetime + end +end diff --git a/db/schema.rb b/db/schema.rb index 878c16869..142990d93 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -682,6 +682,7 @@ ActiveRecord::Schema.define(version: 20180711224810) do t.date "sent_at" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.datetime "hidden_at" end create_table "notifications", force: :cascade do |t| diff --git a/spec/models/activity_spec.rb b/spec/models/activity_spec.rb index 902320ec6..d048a6bb8 100644 --- a/spec/models/activity_spec.rb +++ b/spec/models/activity_spec.rb @@ -7,12 +7,14 @@ describe Activity do expect(build(:activity, actionable: create(:debate))).to be_valid expect(build(:activity, actionable: create(:comment))).to be_valid expect(build(:activity, actionable: create(:user))).to be_valid + expect(build(:activity, actionable: create(:newsletter))).to be_valid end it "is a valid only with allowed actions" do expect(build(:activity, action: "hide")).to be_valid expect(build(:activity, action: "block")).to be_valid expect(build(:activity, action: "restore")).to be_valid + expect(build(:activity, action: "email")).to be_valid expect(build(:activity, action: "dissapear")).not_to be_valid end diff --git a/spec/models/newsletter_spec.rb b/spec/models/newsletter_spec.rb index 788326e88..61abb3daf 100644 --- a/spec/models/newsletter_spec.rb +++ b/spec/models/newsletter_spec.rb @@ -118,6 +118,21 @@ describe Newsletter do expect(Delayed::Job.third.run_at.change(usec: 0)).to eq(third_batch_run_at) end + it "logs users that have received the newsletter" do + newsletter.deliver + + expect(Activity.count).to eq(3) + + recipients.each do |email| + user = User.where(email: email).first + activity = Activity.where(user: user).first + + expect(activity.user_id).to eq(user.id) + expect(activity.action).to eq("email") + expect(activity.actionable).to eq(newsletter) + end + end + it "skips invalid emails" do Proposal.destroy_all From b7a1599fdfde41ee87f7e12598cac7d92f777ba8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mar=C3=ADa=20Checa?= Date: Thu, 24 May 2018 17:53:16 +0200 Subject: [PATCH 04/12] Adds logic to send proposal notifications in batches WIP batches tests --- .../admin/system_emails_controller.rb | 13 ++++ app/models/notification.rb | 25 ++++++++ lib/email_digest.rb | 12 +++- spec/features/emails_spec.rb | 22 ++++++- spec/features/notifications_spec.rb | 63 +++++++++++++++++++ spec/lib/email_digests_spec.rb | 21 ++++++- 6 files changed, 150 insertions(+), 6 deletions(-) diff --git a/app/controllers/admin/system_emails_controller.rb b/app/controllers/admin/system_emails_controller.rb index 626680f43..7a20877d5 100644 --- a/app/controllers/admin/system_emails_controller.rb +++ b/app/controllers/admin/system_emails_controller.rb @@ -24,6 +24,19 @@ class Admin::SystemEmailsController < Admin::BaseController end end + def moderate_pending + ProposalNotification.find(params[:id]).moderate_system_email(current_user) + + redirect_to admin_system_email_preview_pending_path("proposal_notification_digest") + end + + def send_pending + Notification.send_pending + + flash[:notice] = t("admin.system_emails.preview_pending.send_pending_notification") + redirect_to admin_system_emails_path + end + private def load_system_email diff --git a/app/models/notification.rb b/app/models/notification.rb index dacedb762..f99cbc1ca 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -68,4 +68,29 @@ class Notification < ActiveRecord::Base end end + def self.send_pending + run_at = first_batch_run_at + User.email_digest.find_in_batches(batch_size: batch_size) do |users| + users.each do |user| + email_digest = EmailDigest.new(user) + email_digest.deliver(run_at) + end + run_at += batch_interval + end + end + + private + + def self.batch_size + 10000 + end + + def self.batch_interval + 20.minutes + end + + def self.first_batch_run_at + Time.current + end + end diff --git a/lib/email_digest.rb b/lib/email_digest.rb index 511d137a6..b20ab5a6d 100644 --- a/lib/email_digest.rb +++ b/lib/email_digest.rb @@ -14,9 +14,11 @@ class EmailDigest notifications.any? end - def deliver - if pending_notifications? - Mailer.proposal_notification_digest(user, notifications.to_a).deliver_later + + def deliver(run_at) + if valid_email? && pending_notifications? + Mailer.delay(run_at: run_at).proposal_notification_digest(user, notifications.to_a) + mark_as_emailed end end @@ -25,4 +27,8 @@ class EmailDigest user.update(failed_email_digests_count: 0) end + def valid_email? + user.email.match(/\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/i) + end + end diff --git a/spec/features/emails_spec.rb b/spec/features/emails_spec.rb index 91f8b31b6..bbb92230f 100644 --- a/spec/features/emails_spec.rb +++ b/spec/features/emails_spec.rb @@ -294,7 +294,7 @@ feature 'Emails' do notification3 = create_proposal_notification(proposal3) email_digest = EmailDigest.new(user) - email_digest.deliver + email_digest.deliver(Time.current) email_digest.mark_as_emailed email = open_last_email @@ -327,6 +327,26 @@ feature 'Emails' do expect(notification2.emailed_at).to be end + scenario "notifications moderated are not sent" do + user = create(:user, email_digest: true) + proposal = create(:proposal) + proposal_notification = create(:proposal_notification, proposal: proposal) + notification = create(:notification, notifiable: proposal_notification) + + reset_mailer + + proposal_notification.moderate_system_email(create(:administrator).user) + + email_digest = EmailDigest.new(user) + email_digest.deliver(Time.current) + email_digest.mark_as_emailed + + expect { open_last_email }.to raise_error "No email has been sent!" + end + + xscenario "Delete all Notifications included in the digest after email sent" do + end + end context "User invites" do diff --git a/spec/features/notifications_spec.rb b/spec/features/notifications_spec.rb index 482d38b5a..807a644e6 100644 --- a/spec/features/notifications_spec.rb +++ b/spec/features/notifications_spec.rb @@ -182,4 +182,67 @@ feature "Notifications" do end end + describe "#send_pending" do + let!(:user1) { create(:user) } + let!(:user2) { create(:user) } + let!(:user3) { create(:user) } + let!(:proposal_notification) { create(:proposal_notification) } + let!(:notification1) { create(:notification, notifiable: proposal_notification, user: user1) } + let!(:notification2) { create(:notification, notifiable: proposal_notification, user: user2) } + let!(:notification3) { create(:notification, notifiable: proposal_notification, user: user3) } + + before do + reset_mailer + Delayed::Worker.delay_jobs = true + end + + after do + Delayed::Worker.delay_jobs = false + end + + it "sends pending proposal notifications" do + Delayed::Worker.delay_jobs = false + Notification.send_pending + + email = open_last_email + expect(email).to have_subject("Proposal notifications in Decide Madrid") + end + + it "sends emails in batches" do + Notification.send_pending + + expect(Delayed::Job.count).to eq(3) + end + + it "sends batches in time intervals" do + allow(Notification).to receive(:batch_size).and_return(1) + allow(Notification).to receive(:batch_interval).and_return(1.second) + allow(Notification).to receive(:first_batch_run_at).and_return(Time.current) + + remove_users_without_pending_notifications + + Notification.send_pending + + now = Notification.first_batch_run_at + first_batch_run_at = now.change(usec: 0) + second_batch_run_at = (now + 1.second).change(usec: 0) + third_batch_run_at = (now + 2.seconds).change(usec: 0) + + expect(Delayed::Job.count).to eq(3) + expect(Delayed::Job.first.run_at.change(usec: 0)).to eq(first_batch_run_at) + expect(Delayed::Job.second.run_at.change(usec: 0)).to eq(second_batch_run_at) + expect(Delayed::Job.third.run_at.change(usec: 0)).to eq(third_batch_run_at) + end + + end + + def remove_users_without_pending_notifications + users_without_notifications.each { |user| user.destroy } + end + + def users_without_notifications + User.all.select { |user| user.notifications.not_emailed + .where(notifiable_type: 'ProposalNotification').blank? } + end + end diff --git a/spec/lib/email_digests_spec.rb b/spec/lib/email_digests_spec.rb index 7992c3706..2622b76e7 100644 --- a/spec/lib/email_digests_spec.rb +++ b/spec/lib/email_digests_spec.rb @@ -75,7 +75,7 @@ describe EmailDigest do reset_mailer email_digest = described_class.new(user) - email_digest.deliver + email_digest.deliver(Time.current) email = open_last_email expect(email).to have_subject("Proposal notifications in CONSUL") @@ -89,7 +89,7 @@ describe EmailDigest do reset_mailer email_digest = described_class.new(user) - email_digest.deliver + email_digest.deliver(Time.current) expect(all_emails.count).to eq(0) end @@ -137,4 +137,21 @@ describe EmailDigest do end + describe "#valid_email?" do + + it "returns a MatchData if email is valid" do + user = create(:user, email: 'valid_email@email.com') + + email_digest = described_class.new(user) + expect(email_digest.valid_email?).to be_a(MatchData) + end + + it "returns nil if email is invalid" do + user = create(:user, email: 'invalid_email@email..com') + + email_digest = described_class.new(user) + expect(email_digest.valid_email?).to be(nil) + end + end + end From 7bddd97ed3edbf8042e90421a70f601340444090 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mar=C3=ADa=20Checa?= Date: Tue, 29 May 2018 11:54:17 +0200 Subject: [PATCH 05/12] Adds validation of email present in `valid_email?` method --- lib/email_digest.rb | 2 +- spec/lib/email_digests_spec.rb | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/email_digest.rb b/lib/email_digest.rb index b20ab5a6d..502c68f41 100644 --- a/lib/email_digest.rb +++ b/lib/email_digest.rb @@ -28,7 +28,7 @@ class EmailDigest end def valid_email? - user.email.match(/\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/i) + user.email.present? && user.email.match(/\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/i) end end diff --git a/spec/lib/email_digests_spec.rb b/spec/lib/email_digests_spec.rb index 2622b76e7..c5477610d 100644 --- a/spec/lib/email_digests_spec.rb +++ b/spec/lib/email_digests_spec.rb @@ -152,6 +152,14 @@ describe EmailDigest do email_digest = described_class.new(user) expect(email_digest.valid_email?).to be(nil) end + + it "returns false if email does not exist" do + user = create(:user) + user.update_attribute(:email, nil) + + email_digest = described_class.new(user) + expect(email_digest.valid_email?).to be(false) + end end end From d58941ef9c954a8251c120221c84898800a0f815 Mon Sep 17 00:00:00 2001 From: decabeza Date: Thu, 26 Jul 2018 13:46:37 +0200 Subject: [PATCH 06/12] Updates text on notifications spec --- spec/features/notifications_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/notifications_spec.rb b/spec/features/notifications_spec.rb index 807a644e6..f596375b9 100644 --- a/spec/features/notifications_spec.rb +++ b/spec/features/notifications_spec.rb @@ -205,7 +205,7 @@ feature "Notifications" do Notification.send_pending email = open_last_email - expect(email).to have_subject("Proposal notifications in Decide Madrid") + expect(email).to have_subject("Proposal notifications in CONSUL") end it "sends emails in batches" do From eb8c021451b8fd325542c3a72c54de58b29960ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mar=C3=ADa=20Checa?= Date: Thu, 24 May 2018 11:12:11 +0200 Subject: [PATCH 07/12] Adds button to send pending proposal notifications --- .../admin/system_emails_controller.rb | 2 +- app/views/admin/system_emails/index.html.erb | 6 ++- config/locales/en/admin.yml | 3 ++ config/locales/es/admin.yml | 3 ++ config/routes/admin.rb | 2 + spec/features/admin/system_emails_spec.rb | 43 +++++++++++++++++++ 6 files changed, 57 insertions(+), 2 deletions(-) diff --git a/app/controllers/admin/system_emails_controller.rb b/app/controllers/admin/system_emails_controller.rb index 7a20877d5..c7d1c8dd0 100644 --- a/app/controllers/admin/system_emails_controller.rb +++ b/app/controllers/admin/system_emails_controller.rb @@ -31,7 +31,7 @@ class Admin::SystemEmailsController < Admin::BaseController end def send_pending - Notification.send_pending + Notification.delay.send_pending flash[:notice] = t("admin.system_emails.preview_pending.send_pending_notification") redirect_to admin_system_emails_path diff --git a/app/views/admin/system_emails/index.html.erb b/app/views/admin/system_emails/index.html.erb index 4d135d615..573e8a848 100644 --- a/app/views/admin/system_emails/index.html.erb +++ b/app/views/admin/system_emails/index.html.erb @@ -5,7 +5,7 @@ <%= t("admin.shared.title") %> <%= t("admin.shared.description") %> - <%= t("admin.shared.actions") %> + <%= t("admin.shared.actions") %> @@ -27,6 +27,10 @@ admin_system_email_preview_pending_path(system_email_title), class: "button" %> <% end %> + <%= link_to t("admin.system_emails.preview_pending.send_pending"), + admin_system_email_send_pending_path(system_email_title), + class: "button", + method: :put %> <% end %> diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index 7871b377b..52dafc762 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -655,6 +655,9 @@ en: action: Preview Pending preview_of: Preview of %{name} pending_to_be_sent: This is the content pending to be sent + moderate_pending: Moderate notification send + send_pending: Send pending + send_pending_notification: Pending notifications sent succesfully proposal_notification_digest: title: Proposal Notification Digest description: Gathers all proposal notifications for an user in a single message, to avoid too much emails. diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index c6761b568..bae2c43c1 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -656,6 +656,9 @@ es: action: Previsualizar Pendientes preview_of: Vista previa de %{name} pending_to_be_sent: Este es todo el contenido pendiente de enviar + moderate_pending: Moderar envío de notificación + send_pending : Enviar pendientes + send_pending_notification: Notificaciones pendientes enviadas correctamente proposal_notification_digest: title: Resumen de Notificationes de Propuestas description: Reune todas las notificaciones de propuestas en un único mensaje, para evitar demasiados emails. diff --git a/config/routes/admin.rb b/config/routes/admin.rb index 150d1b28a..31138e48c 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -168,6 +168,8 @@ namespace :admin do resources :system_emails, only: [:index] do get :view get :preview_pending + put :moderate_pending + put :send_pending end resources :emails_download, only: :index do diff --git a/spec/features/admin/system_emails_spec.rb b/spec/features/admin/system_emails_spec.rb index dd14e87bc..239e6183a 100644 --- a/spec/features/admin/system_emails_spec.rb +++ b/spec/features/admin/system_emails_spec.rb @@ -65,6 +65,49 @@ feature "System Emails" do expect(page).to have_link('Proposal B Title', href: proposal_url(proposal_b, anchor: 'tab-notifications')) end + + scenario "#moderate_pending" do + proposal1 = create(:proposal, title: 'Proposal A') + proposal2 = create(:proposal, title: 'Proposal B') + proposal_notification1 = create(:proposal_notification, proposal: proposal1, + title: 'Proposal A Title', + body: 'Proposal A Notification Body') + proposal_notification2 = create(:proposal_notification, proposal: proposal2) + create(:notification, notifiable: proposal_notification1, emailed_at: nil) + create(:notification, notifiable: proposal_notification2, emailed_at: nil) + + visit admin_system_email_preview_pending_path('proposal_notification_digest') + + within("#proposal_notification_#{proposal_notification1.id}") do + click_on "Moderate notification send" + end + + visit admin_system_email_preview_pending_path('proposal_notification_digest') + + expect(Notification.count).to equal(1) + expect(Activity.last.actionable_type).to eq('ProposalNotification') + expect(page).not_to have_content("Proposal A Title") + end + + scenario "#send_pending" do + proposal = create(:proposal) + proposal_notification = create(:proposal_notification, proposal: proposal, + title: 'Proposal A Title', + body: 'Proposal A Notification Body') + voter = create(:user, :level_two) + create(:notification, notifiable: proposal_notification, user: voter, emailed_at: nil) + create(:follow, user: voter, followable: proposal) + + visit admin_system_emails_path + + click_on "Send pending" + + email = open_last_email + expect(email).to deliver_to(voter) + expect(email).to have_body_text(proposal_notification.body) + + expect(page).to have_content("Pending notifications sent succesfully") + end end end From 3cc8b1d123516d189bccd16b3b37ad0a2cd2f470 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mar=C3=ADa=20Checa?= Date: Thu, 24 May 2018 11:31:48 +0200 Subject: [PATCH 08/12] Removes rake task to send proposal notifications This action will be performed manually by admin users from the `admin/system_emails` view. --- lib/tasks/emails.rake | 17 ----------------- 1 file changed, 17 deletions(-) delete mode 100644 lib/tasks/emails.rake diff --git a/lib/tasks/emails.rake b/lib/tasks/emails.rake deleted file mode 100644 index 3afa81449..000000000 --- a/lib/tasks/emails.rake +++ /dev/null @@ -1,17 +0,0 @@ -namespace :emails do - - desc "Sends email digest of proposal notifications to each user" - task digest: :environment do - User.email_digest.find_each do |user| - email_digest = EmailDigest.new(user) - begin - email_digest.deliver - email_digest.mark_as_emailed - rescue - user.increment_counter(:failed_email_digests_count) - user.save - end - end - end - -end From f1e7b634ba9f569724d80b4d22f05305fdbe7462 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mar=C3=ADa=20Checa?= Date: Tue, 15 May 2018 12:55:51 +0200 Subject: [PATCH 09/12] Adds moderate actions to proposal system notifications --- .../admin/system_emails_controller.rb | 2 +- app/models/proposal_notification.rb | 5 ++ .../_proposal_notification_digest.html.erb | 59 +++++++++++-------- spec/models/proposal_notification_spec.rb | 16 +++++ 4 files changed, 55 insertions(+), 27 deletions(-) diff --git a/app/controllers/admin/system_emails_controller.rb b/app/controllers/admin/system_emails_controller.rb index c7d1c8dd0..d079c171b 100644 --- a/app/controllers/admin/system_emails_controller.rb +++ b/app/controllers/admin/system_emails_controller.rb @@ -1,6 +1,6 @@ class Admin::SystemEmailsController < Admin::BaseController - before_action :load_system_email, only: [:view, :preview_pending] + before_action :load_system_email, only: [:view, :preview_pending, :moderate_pending] def index @system_emails = { diff --git a/app/models/proposal_notification.rb b/app/models/proposal_notification.rb index f0a6850f7..f057b9cb8 100644 --- a/app/models/proposal_notification.rb +++ b/app/models/proposal_notification.rb @@ -55,4 +55,9 @@ class ProposalNotification < ActiveRecord::Base self.update(author_id: self.proposal.author_id) if self.proposal end + def moderate_system_email(moderator) + Notification.where(notifiable_type: 'ProposalNotification', notifiable: self).destroy_all + Activity.log(moderator, :hide, self) + end + end diff --git a/app/views/admin/system_emails/preview_pending/_proposal_notification_digest.html.erb b/app/views/admin/system_emails/preview_pending/_proposal_notification_digest.html.erb index 7cdd4d66d..273dee0c5 100644 --- a/app/views/admin/system_emails/preview_pending/_proposal_notification_digest.html.erb +++ b/app/views/admin/system_emails/preview_pending/_proposal_notification_digest.html.erb @@ -1,33 +1,40 @@ <% if preview.proposal.present? %> -
-
-
- <%= t("admin.shared.proposal") %>
- <%= link_to preview.proposal.title, proposal_url(preview.proposal) %> -
-
- <%= t("admin.shared.title") %>
- <%= link_to preview.title, proposal_url(preview.proposal, anchor: "tab-notifications") %> +
+
+
+
+ <%= t("admin.shared.proposal") %>
+ <%= link_to preview.proposal.title, proposal_url(preview.proposal) %> +
+
+ <%= t("admin.shared.title") %>
+ <%= link_to preview.title, proposal_url(preview.proposal, anchor: "tab-notifications") %> +
+
+ <%= t("admin.shared.author") %>
+ <%= preview.proposal.author&.username || '-' %> +
+
+ <%= t("admin.shared.created_at") %>
+ <%= l(preview.created_at, format: :datetime) %> +
-
-
- <%= t("admin.shared.author") %>
- <%= preview.proposal.author&.username || '-' %> -
-
- <%= t("admin.shared.created_at") %>
- <%= l(preview.created_at, format: :datetime) %> -
-
-
-
-
- <%= t("admin.shared.content") %> -

- <%= preview.body %> -

+
+
+ <%= t("admin.shared.content") %> +

+ <%= preview.body %> +

+
+ + <%= link_to t("admin.system_emails.preview_pending.moderate_pending"), + admin_system_email_moderate_pending_path(system_email_id: 'proposal_notification_digest', + id: preview.id), + method: :put, + class: "button hollow float-right" %>
+
<% end %> diff --git a/spec/models/proposal_notification_spec.rb b/spec/models/proposal_notification_spec.rb index 60021d95c..d7ba529c4 100644 --- a/spec/models/proposal_notification_spec.rb +++ b/spec/models/proposal_notification_spec.rb @@ -152,5 +152,21 @@ describe ProposalNotification do end + describe "#moderate_system_email" do + let(:admin) { create(:administrator) } + let(:proposal) { create(:proposal) } + let(:proposal_notification) { build(:proposal_notification, proposal: proposal) } + let(:notification) { create(:notification, notifiable: proposal_notification) } + + it "removes all notifications related to the proposal notification" do + proposal_notification.moderate_system_email(admin.user) + expect(Notification.all.count).to be(0) + end + + it "records the moderation action in the Activity table" do + proposal_notification.moderate_system_email(admin.user) + expect(Activity.last.actionable_type).to eq('ProposalNotification') + end + end end end From 9d580e15f5cb4fc80286ce8047f0140e9e816c85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mar=C3=ADa=20Checa?= Date: Tue, 15 May 2018 13:19:47 +0200 Subject: [PATCH 10/12] Adds activity tab for system emails --- app/controllers/admin/activity_controller.rb | 2 +- app/models/activity.rb | 1 + app/models/proposal_notification.rb | 1 + config/locales/en/admin.yml | 1 + config/locales/es/admin.yml | 1 + spec/features/admin/activity_spec.rb | 18 ++++++++++++++++++ 6 files changed, 23 insertions(+), 1 deletion(-) diff --git a/app/controllers/admin/activity_controller.rb b/app/controllers/admin/activity_controller.rb index 1d763c09d..ce9c04183 100644 --- a/app/controllers/admin/activity_controller.rb +++ b/app/controllers/admin/activity_controller.rb @@ -1,5 +1,5 @@ class Admin::ActivityController < Admin::BaseController - has_filters %w{all on_users on_proposals on_debates on_comments} + has_filters %w{all on_users on_proposals on_debates on_comments on_system_emails} def show @activity = Activity.for_render.send(@current_filter) diff --git a/app/models/activity.rb b/app/models/activity.rb index 834f7721c..afe25bed3 100644 --- a/app/models/activity.rb +++ b/app/models/activity.rb @@ -11,6 +11,7 @@ class Activity < ActiveRecord::Base scope :on_users, -> { where(actionable_type: 'User') } scope :on_comments, -> { where(actionable_type: 'Comment') } scope :on_budget_investments, -> { where(actionable_type: 'Budget::Investment') } + scope :on_system_emails, -> { where(actionable_type: 'ProposalNotification') } scope :for_render, -> { includes(user: [:moderator, :administrator]).includes(:actionable) } def self.log(user, action, actionable) diff --git a/app/models/proposal_notification.rb b/app/models/proposal_notification.rb index f057b9cb8..2532f6c4c 100644 --- a/app/models/proposal_notification.rb +++ b/app/models/proposal_notification.rb @@ -10,6 +10,7 @@ class ProposalNotification < ActiveRecord::Base validates :proposal, presence: true validate :minimum_interval + scope :with_hidden, -> { all } scope :public_for_api, -> { where(proposal_id: Proposal.public_for_api.pluck(:id)) } scope :sort_by_created_at, -> { reorder(created_at: :desc) } scope :sort_by_moderated, -> { reorder(moderated: :desc) } diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index 52dafc762..259ea7c44 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -67,6 +67,7 @@ en: on_debates: Debates on_proposals: Proposals on_users: Users + on_system_emails: System emails title: Moderator activity type: Type no_activity: There are no moderators activity. diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index bae2c43c1..00cd907a3 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -67,6 +67,7 @@ es: on_debates: Debates on_proposals: Propuestas on_users: Usuarios + on_system_emails: Emails del sistema title: Actividad de los Moderadores type: Tipo no_activity: No hay actividad de moderadores. diff --git a/spec/features/admin/activity_spec.rb b/spec/features/admin/activity_spec.rb index 5529cb464..9cfd5f9b8 100644 --- a/spec/features/admin/activity_spec.rb +++ b/spec/features/admin/activity_spec.rb @@ -331,4 +331,22 @@ feature 'Admin activity' do end end + context "System emails" do + scenario "Shows moderation activity on system emails" do + proposal = create(:proposal, title: 'Proposal A') + proposal_notification = create(:proposal_notification, proposal: proposal, + title: 'Proposal A Title', + body: 'Proposal A Notification Body') + proposal_notification.moderate_system_email(@admin.user) + + visit admin_activity_path + + within("#activity_#{Activity.last.id}") do + expect(page).to have_content(proposal_notification.title) + expect(page).to have_content("Hidden") + expect(page).to have_content(@admin.user.username) + end + end + end + end From 786a3949fd2bcc987e3e691aa1c3e8817f89518c Mon Sep 17 00:00:00 2001 From: decabeza Date: Thu, 26 Jul 2018 17:13:45 +0200 Subject: [PATCH 11/12] Moves moderate_system_email from private method --- app/models/proposal_notification.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/proposal_notification.rb b/app/models/proposal_notification.rb index 2532f6c4c..37640b0e5 100644 --- a/app/models/proposal_notification.rb +++ b/app/models/proposal_notification.rb @@ -38,6 +38,11 @@ class ProposalNotification < ActiveRecord::Base proposal end + def moderate_system_email(moderator) + Notification.where(notifiable_type: 'ProposalNotification', notifiable: self).destroy_all + Activity.log(moderator, :hide, self) + end + def ignore_flag update(ignored_at: Time.current) end @@ -56,9 +61,4 @@ class ProposalNotification < ActiveRecord::Base self.update(author_id: self.proposal.author_id) if self.proposal end - def moderate_system_email(moderator) - Notification.where(notifiable_type: 'ProposalNotification', notifiable: self).destroy_all - Activity.log(moderator, :hide, self) - end - end From 69d67940d38c075b81ff234873707295bf952290 Mon Sep 17 00:00:00 2001 From: decabeza Date: Thu, 26 Jul 2018 17:53:26 +0200 Subject: [PATCH 12/12] Removes unnecesary scope --- app/models/proposal_notification.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models/proposal_notification.rb b/app/models/proposal_notification.rb index 37640b0e5..686fdc075 100644 --- a/app/models/proposal_notification.rb +++ b/app/models/proposal_notification.rb @@ -10,7 +10,6 @@ class ProposalNotification < ActiveRecord::Base validates :proposal, presence: true validate :minimum_interval - scope :with_hidden, -> { all } scope :public_for_api, -> { where(proposal_id: Proposal.public_for_api.pluck(:id)) } scope :sort_by_created_at, -> { reorder(created_at: :desc) } scope :sort_by_moderated, -> { reorder(moderated: :desc) }