Merge pull request #2802 from consul/newsletter-delivery

Newsletter delivery

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)

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|
+      @newsletter.delay.deliver
-        Mailer.newsletter(@newsletter, recipient_email).deliver_later
-      end
-
       @newsletter.update(sent_at: Time.current)
       flash[:notice] = t("admin.newsletters.send_success")
     else

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 = {
@@ -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.delay.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

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}
 
@@ -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)

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
@@ -20,9 +23,47 @@ 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|
+        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
+    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
+
+  def log_delivery(recipient_email)
+    user = User.where(email: recipient_email).first
+    Activity.log(user, :email, self)
+  end
 end

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

app/models/proposal_notification.rb

@@ -37,6 +37,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

app/views/admin/system_emails/index.html.erb

@@ -5,7 +5,7 @@
     <tr>
       <th><%= t("admin.shared.title") %></th>
       <th><%= t("admin.shared.description") %></th>
-      <th class="small-5 text-right"><%= t("admin.shared.actions") %></th>
+      <th class="small-7 text-right"><%= t("admin.shared.actions") %></th>
     </tr>
   </thead>
   <tbody>
@@ -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 %>
       </td>
     </tr>
   <% end %>

app/views/admin/system_emails/preview_pending/_proposal_notification_digest.html.erb

@@ -1,6 +1,7 @@
 <% if preview.proposal.present? %>
+  <div id="<%= dom_id(preview) %>">
     <div class="callout highlight">
-    <div class="row">
+      <div class="row expanded">
         <div class="small-12 medium-6 column">
           <strong><%= t("admin.shared.proposal") %></strong><br>
           <%= link_to preview.proposal.title, proposal_url(preview.proposal) %>
@@ -9,8 +10,6 @@
           <strong><%= t("admin.shared.title") %></strong><br>
           <%= link_to preview.title, proposal_url(preview.proposal, anchor: "tab-notifications") %>
         </div>
-    </div>
-    <div class="row">
         <div class="small-12 medium-6 column">
           <strong><%= t("admin.shared.author") %></strong><br>
           <%= preview.proposal.author&.username || '-' %>
@@ -25,9 +24,17 @@
     <div class="row">
       <div class="column">
         <strong><%= t("admin.shared.content") %></strong>
-      <p class="help-text" id="phase-description-help-text">
+        <p id="phase-description-help-text">
           <%= preview.body %>
         </p>
       </div>
     </div>
+
+    <%= 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" %>
+  </div>
+  <hr>
 <% end %>

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.
@@ -655,6 +656,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.

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.
@@ -656,6 +657,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.

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

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

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|

lib/email_digest.rb

@@ -14,9 +14,11 @@ class EmailDigest
     notifications.any?
   end
 
-  def deliver
+
-    if pending_notifications?
+  def deliver(run_at)
-      Mailer.proposal_notification_digest(user, notifications.to_a).deliver_later
+    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.present? && user.email.match(/\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/i)
+  end
+
 end

lib/tasks/emails.rake

@@ -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

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

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

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

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 CONSUL")
+    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

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,29 @@ 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
+
+    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

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
 

spec/models/newsletter_spec.rb

@@ -63,4 +63,95 @@ 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 "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
+
+      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

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