AO3-7576 Fix comment emails for news posts posted from preview

app/controllers/admin_posts_controller.rb

@@ -153,7 +153,7 @@ def load_admin_posts
 
   def admin_post_params
     params.require(:admin_post).permit(
-      :admin_id, :title, :content, :translated_post_id, :language_id, :tag_list,
+      :title, :content, :translated_post_id, :language_id, :tag_list,
       :comment_permissions, :moderated_commenting_enabled
     )
   end

app/mailers/comment_mailer.rb

@@ -1,10 +1,10 @@
 class CommentMailer < ApplicationMailer
   # Sends email to an owner of the top-level commentable when a new comment is created
-  # This may be an admin, in which case we use the admin address instead
-  def comment_notification(user, comment)
+  # The recipient may be a User or a plain email address to send to
+  def comment_notification(recipient, comment)
     @comment = comment
     @owner = true
-    email = user.is_a?(Admin) ? ArchiveConfig.ADMIN_ADDRESS : user.email
+    email = recipient.is_a?(User) ? recipient.email : recipient
     mail(
       to: email,
       # i18n-tasks-use t("comment_mailer.comment_notification.subject.chapter")
@@ -15,11 +15,11 @@ def comment_notification(user, comment)
   end
 
   # Sends email to an owner of the top-level commentable when a comment is edited
-  # This may be an admin, in which case we use the admin address instead
-  def edited_comment_notification(user, comment)
+  # The recipient may be a User or a plain email address to send to
+  def edited_comment_notification(recipient, comment)
     @comment = comment
     @owner = true
-    email = user.is_a?(Admin) ? ArchiveConfig.ADMIN_ADDRESS : user.email
+    email = recipient.is_a?(User) ? recipient.email : recipient
     mail(
       to: email,
       # i18n-tasks-use t("comment_mailer.edited_comment_notification.subject.chapter")

app/models/admin_post.rb

@@ -46,6 +46,7 @@ class AdminPost < ApplicationRecord
   scope :unposted, -> { where(posted: false) }
   scope :posted, -> { where(posted: true) }
 
+  before_validation :set_admin_id
   before_validation :inherit_translated_post_attributes
   before_save :apply_tag_list
   before_save :set_published_at, if: :posted_changed?
@@ -59,11 +60,9 @@ def commentable_name
   end
 
   def commentable_owners
-    begin
-        [Admin.find(self.admin_id)]
-    rescue
-      []
-    end
+    [Admin.find(self.admin_id)]
+  rescue ActiveRecord::RecordNotFound
+    []
   end
 
   def draft?
@@ -135,6 +134,12 @@ def expire_cached_home_admin_posts
     end
   end
 
+  def set_admin_id
+    return unless User.current_user.is_a?(Admin)
+
+    self.admin_id = User.current_user.id
+  end
+
   def inherit_translated_post_attributes
     return if translated_post.blank?
 

app/models/comment.rb

@@ -217,6 +217,11 @@ def after_update
         end
       end
 
+      if self.ultimate_parent.is_a?(AdminPost)
+        CommentMailer.edited_comment_notification(ArchiveConfig.ADMIN_ADDRESS, self).deliver_after_commit
+        return
+      end
+
       # send notification to the owner(s) of the ultimate parent, who can be users or admins
       # at this point, users contains those who've already been notified
       if users.empty?
@@ -264,6 +269,11 @@ def after_create
       users << parent_comment_owner
     end
 
+    if self.ultimate_parent.is_a?(AdminPost)
+      CommentMailer.comment_notification(ArchiveConfig.ADMIN_ADDRESS, self).deliver_after_commit
+      return
+    end
+
     # send notification to the owner(s) of the ultimate parent, who can be users or admins
     # at this point, users contains those who've already been notified
     if users.empty?

app/views/admin_posts/_admin_post_form.html.erb

@@ -4,8 +4,7 @@
     <%= error_messages_for @admin_post %>
     <fieldset class="text">
       <legend><%= ts("Post News") %></legend>
-      <h4><input type="hidden" name="admin_post[admin_id]" value="<%= current_admin.id %>" />
-      <%= f.text_field :title, :title =>"title" %></h4>
+      <h4><%= f.text_field :title, title: "title" %></h4>
       <!--BACK END, can we DRY up these post forms? There's admin_post_form, archive_faq_form, _chapter_form, known_issues_form, standard form-->
       <ul class="hidden rtf-html-switch actions" role="menu">
         <li><a class="rtf-link" href="#"><%= ts('Rich Text') %></a></li>

features/admins/admin_post_news.feature

@@ -334,3 +334,18 @@ Scenario: Preview of an edit to an admin post does not immediately create tags
     Then "never seen before" should not be an option within "Tag"
       And "original1" should be an option within "Tag"
       And "original2" should be an option within "Tag"
+
+  Scenario: Comment emails are sent on an admin post posted from preview
+    Given I am logged in as a "communications" admin
+      And I start to make an admin post
+      And I press "Preview"
+      And I press "Post"
+      And all emails have been delivered
+    When I am logged in as "happyuser"
+      And I go to the admin-posts page
+      And I follow "Default Admin Post"
+      And I fill in "Comment" with "I love this!"
+      And I press "Comment"
+    Then 0 emails should be delivered to "testadmin-communications@example.org"
+      But 1 email should be delivered to "admin@example.org"
+      And the email should contain "I love this!"

spec/controllers/admin_posts_controller_spec.rb

@@ -282,7 +282,7 @@
 
           context "with invalid translated_post_id" do
             it "renders the edit template with error message" do
-              put :update, params: { id: admin_post.id, admin_post: { admin_id: admin.id, translated_post_id: 0 } }
+              put :update, params: { id: admin_post.id, admin_post: { translated_post_id: 0 } }
 
               expect(response).to render_template(:edit)
               expect(assigns[:admin_post].errors.full_messages).to include("Translated post does not exist")
@@ -298,7 +298,6 @@
                   put :update, params: {
                     id: translation.id,
                     admin_post: {
-                      admin_id: admin.id,
                       comment_permissions: :disable_all
                     }
                   }
@@ -311,7 +310,7 @@
 
             context "with invalid translated_post language" do
               it "renders the edit template with error message" do
-                put :update, params: { id: translation.id, admin_post: { admin_id: admin.id, language_id: admin_post.language_id } }
+                put :update, params: { id: translation.id, admin_post: { language_id: admin_post.language_id } }
                 expect(response).to render_template(:edit)
                 expect(assigns[:admin_post].errors.full_messages).to include("Translated post cannot be same language as original post")
               end
@@ -323,10 +322,11 @@
                 let(:translation) { create(:admin_post, :draft, translated_post_id: admin_post.id, language_id: create(:language).id) }
 
                 it "will remain a draft" do
-                  put :update, params: { post_button: "Post", id: translation.id, admin_post: { admin_id: admin.id } }
-  
+                  put :update, params: { post_button: "Post", id: translation.id, admin_post: { title: "New Title" } }
+
                   it_redirects_to_with_notice(admin_post_path(assigns[:admin_post]), "Admin Post was successfully updated.")
                   expect(translation.reload.posted?).to be_falsey
+                  expect(translation.title).to eq("New Title")
                 end
               end
             end

spec/mailers/comment_mailer_spec.rb

@@ -196,12 +196,8 @@ def queue_adapter_for_test
   end
 
   shared_examples "a notification email for admins" do
-    it "is not delivered to the admin who is the commentable owner" do
-      expect(email).not_to deliver_to(comment.ultimate_parent.commentable_owners.first.email)
-    end
-
     it "is delivered to the admin address" do
-      expect(email).to deliver_to(ArchiveConfig.ADMIN_ADDRESS)
+      expect(email.to).to eq([ArchiveConfig.ADMIN_ADDRESS])
     end
   end
 
@@ -434,7 +430,7 @@ def queue_adapter_for_test
     end
 
     context "when the comment is on an admin post" do
-      let(:user) { comment.ultimate_parent.commentable_owners.first }
+      let(:user) { ArchiveConfig.ADMIN_ADDRESS }
       let(:comment) { create(:comment, :on_admin_post) }
 
       it_behaves_like "a notification email for admins"
@@ -450,6 +446,14 @@ def queue_adapter_for_test
 
         it_behaves_like "a notification email to someone who can review comments"
       end
+
+      context "when the admin post has no admin_id set" do
+        let!(:admin) { create(:admin) }
+        let(:admin_post) { create(:admin_post, admin_id: nil) }
+        let(:comment) { create(:comment, commentable: admin_post) }
+
+        it_behaves_like "a notification email for admins"
+      end
     end
 
     context "when the comment is a reply to another comment" do
@@ -559,7 +563,7 @@ def queue_adapter_for_test
     end
 
     context "when the comment is on an admin post" do
-      let(:user) { comment.ultimate_parent.commentable_owners.first }
+      let(:user) { ArchiveConfig.ADMIN_ADDRESS }
       let(:comment) { create(:comment, :on_admin_post) }
 
       it_behaves_like "a notification email for admins"

spec/models/admin_post_spec.rb

@@ -97,4 +97,38 @@
       expect(parent_post.reload.translations).to eq([language1_post, language2_post])
     end
   end
+
+  describe "#set_admin_id" do
+    let(:admin) { create(:superadmin) }
+
+    context "on a new admin post" do
+      let!(:admin_post) { build(:admin_post, admin_id: nil) }
+
+      it "sets the admin_id to the admin currently logged in" do
+        User.current_user = admin
+
+        expect(admin_post.save).to be_truthy
+        expect(admin_post.reload.admin_id).to eq(admin.id)
+      end
+
+      it "does nothing if no admin is logged in" do
+        User.current_user = nil
+
+        expect(admin_post.save).to be_truthy
+        expect(admin_post.reload.admin_id).to be_nil
+      end
+    end
+
+    context "on an existing admin post" do
+      let!(:admin_post) { create(:admin_post) }
+
+      it "replaces the admin_id if already set" do
+        User.current_user = admin
+
+        expect { admin_post.save }
+          .to change { admin_post.reload.admin_id }
+          .to(admin.id)
+      end
+    end
+  end
 end