Nullify published_at when unpublishing an article (#18298)

* Remove context_notifications when unpublishing all posts

* Nullify published_at when unpublishing an article

app/models/article.rb

@@ -824,6 +824,8 @@ class Article < ApplicationRecord
 
   def has_correct_published_at?
     return unless published_at_was && published
+    # nullifying published_at when unpublishing is valid
+    return if changes["published"] == [true, false] && !published_at
     # don't allow editing published_at if an article has already been published
     # allow changes within one minute in case of editing via frontmatter w/o specifying seconds
     return unless published_was && published_at_was < Time.current &&

app/services/articles/attributes.rb

@@ -11,7 +11,8 @@ module Articles
       @article_user = article_user
     end
 
-    def for_update(update_edited_at: false, update_published_at: false)
+    def for_update(update_edited_at: false, update_published_at: false,
+                   nullify_published_at: false)
       hash = attributes.slice(*ATTRIBUTES)
       # don't reset the collection when no series was passed
       hash[:collection] = collection if attributes.key?(:series)
@@ -19,6 +20,8 @@ module Articles
       hash[:edited_at] = Time.current if update_edited_at
       if update_published_at
         hash[:published_at] = Time.current
+      elsif nullify_published_at
+        hash[:published_at] = nil
       elsif hash[:published_at]
         hash[:published_at] = hash[:published_at].to_datetime
       end

app/services/articles/updater.rb

@@ -23,9 +23,15 @@ module Articles
       publishing = !article.published && article_params[:published]
       past_published_at = !article_params[:published_at] || article_params[:published_at] < Time.current
       update_published_at = publishing && !article.published_from_feed && past_published_at
+
+      # nullify published_at on unpublishing to avoid having drafts with various published_at values
+      # because it may be misleading
+      unpublishing = article.published && !article_params[:published]
+
       attrs = Articles::Attributes.new(article_params, article.user)
         .for_update(update_edited_at: update_edited_at,
-                    update_published_at: update_published_at)
+                    update_published_at: update_published_at,
+                    nullify_published_at: unpublishing)
 
       success = article.update(attrs)
       if success

app/workers/moderator/unpublish_all_articles_worker.rb

@@ -11,6 +11,9 @@ module Moderator
       user.articles.published.find_each do |article|
         if article.has_frontmatter?
           article.body_markdown.sub!(/^published:\s*true\s*$/, "published: false")
+        else
+          # nullify published_at when unpublishing for the articles published in a rich markdown editor
+          article.published_at = nil
         end
         article.published = false
         article.save(validate: false)

spec/services/articles/attributes_spec.rb

@@ -78,6 +78,12 @@ RSpec.describe Articles::Attributes, type: :service do
       expect(attrs[:published_at]).to be_falsey
     end
 
+    it "nullifies published_at if nullify_publlished_at is passed" do
+      attrs = described_class.new({ title: "title", published_at: Time.current }, user)
+        .for_update(nullify_published_at: true)
+      expect(attrs[:published_at]).to be_nil
+    end
+
     it "sets published_at correctly" do
       attrs = described_class.new({ title: "title", published_at: "2022-04-25" }, user).for_update
       expect(attrs[:published_at]).to eq(DateTime.new(2022, 4, 25))

spec/services/articles/updater_spec.rb

@@ -124,12 +124,12 @@ RSpec.describe Articles::Updater, type: :service do
     context "when an article is unpublished" do
       before { attributes[:published] = false }
 
-      it "doesn't update published_at" do
+      it "nullifies published_at" do
         published_at = 1.day.ago
         article.update_column(:published_at, published_at)
         described_class.call(user, article, attributes)
         article.reload
-        expect(article.published_at).to be_within(1.second).of(published_at)
+        expect(article.published_at).to be_nil
       end
 
       it "doesn't send any notifications" do
@@ -144,7 +144,7 @@ RSpec.describe Articles::Updater, type: :service do
         expect(Mentions::CreateAll).not_to have_received(:call).with(article)
       end
 
-      it "destroys the preexisting notifications" do
+      it "destroys the pre-existing notifications" do
         allow(Notification).to receive(:remove_all_by_action_without_delay).and_call_original
         described_class.call(user, article, attributes)
         attrs = { notifiable_ids: article.id, notifiable_type: "Article", action: "Published" }
@@ -152,7 +152,7 @@ RSpec.describe Articles::Updater, type: :service do
         # expect(ContextNotification).to have_received(:delete_all)
       end
 
-      it "destroys the prexexisting context notifications" do
+      it "destroys the pre-existing context notifications" do
         create(:context_notification, context: article, action: "Published")
         expect do
           described_class.call(user, article, attributes)
@@ -180,7 +180,7 @@ RSpec.describe Articles::Updater, type: :service do
         allow(Notification).to receive(:remove_all).and_call_original
       end
 
-      it "removes any preexisting comment notifications but does not delete the comment" do
+      it "removes any pre-existing comment notifications but does not delete the comment" do
         described_class.call(user, article, attributes)
 
         expect(Notification).to have_received(:remove_all).with(
@@ -201,7 +201,7 @@ RSpec.describe Articles::Updater, type: :service do
         allow(Notification).to receive(:remove_all).and_call_original
       end
 
-      it "removes any preexisting mention notifications but does not delete the mention" do
+      it "removes any pre-existing mention notifications but does not delete the mention" do
         described_class.call(user, article, attributes)
 
         expect(Notification).to have_received(:remove_all).with(