Articles update refactoring (#12913)

* Start refactoring articles update

* Articles update refactoring

* Added a couple of tests for Articles::Updater

* Reorganized code in Articles::Updater and ::Attributes a bit

* A couple of more tests for Articles::Attributes

* Reverted changed in the articles_controller

* Edited_at in Articles::Attributes

* Use Articles::Updater in ArticlesController

* Tests for notifications when published/unpublished an article

* Removed old code

* Return result object from Articles::Updater

* Don't reset articles collection when no series was passed

* Fixed sending notifications when published an article the first time

* Fix articles collection when updating

* Fix setting attributes for articles update

* Spec for articles update via api failure

* More tests for Articles::Updater

* More specs for articles updating

* Pass article instead of article_id to articles updater to avoid loading an article twice

* Remove the redundant check in the articles_controller

* Articles::Attributes refactoring

app/controllers/api/v0/articles_controller.rb

@@ -71,9 +71,19 @@ module Api
       end
 
       def update
-        @article = Articles::Updater.call(@user, params[:id], article_params)
+        articles_relation = @user.has_role?(:super_admin) ? Article.includes(:user) : @user.articles
+        article = articles_relation.find(params[:id])
 
-        render "show", status: :ok
+        result = Articles::Updater.call(@user, article, article_params)
+
+        @article = result.article
+
+        if result.success
+          render "show", status: :ok
+        else
+          message = @article.errors_as_sentence
+          render json: { error: message, status: 422 }, status: :unprocessable_entity
+        end
       end
 
       def me

app/controllers/articles_controller.rb

@@ -127,16 +127,8 @@ class ArticlesController < ApplicationController
     authorize @article
     @user = @article.user || current_user
 
-    not_found if @article.user_id != @user.id && !@user.has_role?(:super_admin)
+    updated = Articles::Updater.call(@user, @article, article_params_json)
 
-    edited_at_date = if @article.user == current_user && @article.published
-                       Time.current
-                     else
-                       @article.edited_at
-                     end
-    updated = @article.update(article_params_json.merge(edited_at: edited_at_date))
-    handle_notifications(updated)
-    Webhook::DispatchEvent.call("article_updated", @article) if updated
     respond_to do |format|
       format.html do
         # TODO: JSON should probably not be returned in the format.html section
@@ -156,7 +148,7 @@ class ArticlesController < ApplicationController
       end
 
       format.json do
-        render json: if updated
+        render json: if updated.success
                        @article.to_json(only: [:id], methods: [:current_state_path])
                      else
                        @article.errors.to_json
@@ -240,14 +232,6 @@ class ArticlesController < ApplicationController
     Honeycomb.add_field("article_id", @article.id)
   end
 
-  def article_params
-    params[:article][:published] = true if params[:submit_button] == "PUBLISH"
-    modified_params = policy(Article).permitted_attributes
-    modified_params << :user_id if org_admin_user_change_privilege
-    modified_params << :comment_template if current_user.has_role?(:admin)
-    params.require(:article).permit(modified_params)
-  end
-
   # TODO: refactor all of this update logic into the Articles::Updater possibly,
   # ideally there should only be one place to handle the update logic
   def article_params_json
@@ -255,14 +239,6 @@ class ArticlesController < ApplicationController
 
     params["article"].transform_keys!(&:underscore)
 
-    # handle series/collections
-    if params["article"]["series"].present?
-      collection = Collection.find_series(params["article"]["series"], @user)
-      params["article"]["collection_id"] = collection.id
-    elsif params["article"]["series"] == "" # reset collection?
-      params["article"]["collection_id"] = nil
-    end
-
     allowed_params = if params["article"]["version"] == "v1"
                        %i[body_markdown]
                      else
@@ -284,19 +260,6 @@ class ArticlesController < ApplicationController
     params.require(:article).permit(allowed_params)
   end
 
-  def handle_notifications(updated)
-    if updated && @article.published && @article.saved_changes["published"] == [false, true]
-      Notification.send_to_followers(@article, "Published")
-    elsif @article.saved_changes["published"] == [true, false]
-      Notification.remove_all_by_action_without_delay(notifiable_ids: @article.id, notifiable_type: "Article",
-                                                      action: "Published")
-      if @article.comments.exists?
-        Notification.remove_all(notifiable_ids: @article.comments.ids,
-                                notifiable_type: "Comment")
-      end
-    end
-  end
-
   def allowed_to_change_org_id?
     potential_user = @article&.user || current_user
     potential_org_id = params["article"]["organization_id"].presence || @article&.organization_id

app/services/articles/attributes.rb

@@ -0,0 +1,37 @@
+module Articles
+  class Attributes
+    ATTRIBUTES = %i[archived body_markdown canonical_url description
+                    edited_at main_image organization_id user_id published
+                    title video_thumbnail_url].freeze
+
+    attr_reader :attributes, :article_user
+
+    def initialize(attributes, article_user)
+      @attributes = attributes
+      @article_user = article_user
+    end
+
+    def for_update(update_edited_at: false)
+      hash = attributes.slice(*ATTRIBUTES)
+      # don't reset the collection when no series was passed
+      hash[:collection] = collection if attributes.key?(:series)
+      hash[:tag_list] = tag_list
+      hash[:edited_at] = Time.current if update_edited_at
+      hash
+    end
+
+    private
+
+    def collection
+      Collection.find_series(attributes[:series], article_user) if attributes[:series].present?
+    end
+
+    def tag_list
+      if attributes[:tag_list]
+        attributes[:tag_list]
+      elsif attributes[:tags]
+        attributes[:tags].join(", ")
+      end
+    end
+  end
+end

app/services/articles/updater.rb

@@ -1,8 +1,10 @@
 module Articles
   class Updater
-    def initialize(user, article_id, article_params, event_dispatcher = Webhook::DispatchEvent)
+    Result = Struct.new(:success, :article, keyword_init: true)
+
+    def initialize(user, article, article_params, event_dispatcher = Webhook::DispatchEvent)
       @user = user
-      @article_id = article_id
+      @article = article
       @article_params = article_params
       @event_dispatcher = event_dispatcher
     end
@@ -14,60 +16,42 @@ module Articles
     def call
       user.rate_limiter.check_limit!(:article_update)
 
-      article = load_article
       was_published = article.published
 
-      # the client can change the series the article belongs to
-      if article_params.key?(:series)
-        series = article_params[:series]
-        article.collection = Collection.find_series(series, article.user) if series.present?
-        article.collection = nil if series.nil?
-      end
-
-      # convert tags from array to a string
-      tags = article_params[:tags]
-      if tags.present?
-        article_params[:tag_list] = tags.join(", ")
-        article_params.delete(:tags)
-      end
-
       # updated edited time only if already published and not edited by an admin
       update_edited_at = article.user == user && article.published
-      article_params[:edited_at] = Time.current if update_edited_at
+      attrs = Articles::Attributes.new(article_params, article.user).for_update(update_edited_at: update_edited_at)
 
-      article.update!(article_params)
-      user.rate_limiter.track_limit_by_action(:article_update)
+      success = article.update(attrs)
 
-      # send notification only the first time an article is published
-      send_notification = article.published && article.saved_change_to_published_at.present?
-      Notification.send_to_followers(article, "Published") if send_notification
+      if success
+        user.rate_limiter.track_limit_by_action(:article_update)
 
-      # remove related notifications if unpublished
-      if article.saved_changes["published"] == [true, false]
-        Notification.remove_all_by_action_without_delay(notifiable_ids: article.id, notifiable_type: "Article",
-                                                        action: "Published")
-        if article.comments.exists?
-          Notification.remove_all(notifiable_ids: article.comments.ids,
-                                  notifiable_type: "Comment")
+        # send notification only the first time an article is published
+        send_notification = article.published && article.saved_change_to_published_at.present?
+        Notification.send_to_followers(article, "Published") if send_notification
+
+        # remove related notifications if unpublished
+        if article.saved_changes["published"] == [true, false]
+          Notification.remove_all_by_action_without_delay(notifiable_ids: article.id, notifiable_type: "Article",
+                                                          action: "Published")
+          if article.comments.exists?
+            Notification.remove_all(notifiable_ids: article.comments.ids,
+                                    notifiable_type: "Comment")
+          end
         end
+        # don't send only if article keeps being unpublished
+        dispatch_event(article) if article.published || was_published
       end
-      # don't send only if article keeps being unpublished
-      dispatch_event(article) if article.published || was_published
-
-      article.decorate
+      Result.new(success: success, article: article.decorate)
     end
 
     private
 
-    attr_reader :user, :article_id, :article_params, :event_dispatcher
+    attr_reader :user, :article, :article_params, :event_dispatcher
 
     def dispatch_event(article)
       event_dispatcher.call("article_updated", article)
     end
-
-    def load_article
-      relation = user.has_role?(:super_admin) ? Article.includes(:user) : user.articles
-      relation.find(article_id)
-    end
   end
 end

spec/requests/api/v0/articles_spec.rb

@@ -1180,6 +1180,12 @@ RSpec.describe "Api::V0::Articles", type: :request do
         expect(response).to have_http_status(:unprocessable_entity)
         expect(response.parsed_body["error"]).to be_present
       end
+
+      it "fails when article is not saved" do
+        put_article(title: nil, body_markdown: nil)
+        expect(response).to have_http_status(:unprocessable_entity)
+        expect(response.parsed_body["error"]).to be_present
+      end
     end
   end
 end

spec/requests/articles/articles_update_spec.rb

@@ -10,6 +10,7 @@ RSpec.describe "ArticlesUpdate", type: :request do
     user
   end
   let(:article) { create(:article, user_id: user.id) }
+  let(:other_article) { create(:article, user: user2) }
   let(:collection) { create(:collection, user: user) }
 
   before do
@@ -81,6 +82,18 @@ RSpec.describe "ArticlesUpdate", type: :request do
     expect(article.organization_id).to eq(admin_org_id)
   end
 
+  it "allows super_admin to edit an article" do
+    user.add_role(:super_admin)
+    put "/articles/#{other_article.id}", params: { article: { title: "new", body_markdown: "hello" } }
+    expect(other_article.reload.title).to eq("new")
+  end
+
+  it "doesn't allow other user to edit an article" do
+    expect do
+      put "/articles/#{other_article.id}", params: { article: { body_markdown: "hello" } }
+    end.to raise_error(Pundit::NotAuthorizedError)
+  end
+
   it "archives" do
     put "/articles/#{article.id}", params: {
       article: { archived: true }
@@ -116,11 +129,20 @@ RSpec.describe "ArticlesUpdate", type: :request do
     expect(article.collection).to eq(nil)
   end
 
-  it "creates a notification job if published" do
-    article.update_column(:published, false)
+  it "creates a notification job if published the first time" do
+    draft = create(:article, published: false, user_id: user.id)
     sidekiq_assert_enqueued_with(job: Notifications::NotifiableActionWorker) do
+      put "/articles/#{draft.id}", params: {
+        article: { published: true, body_markdown: "blah"  }
+      }
+    end
+  end
+
+  it "does not create a notification job if published the second time" do
+    article.update_column(:published, false)
+    sidekiq_assert_not_enqueued_with(job: Notifications::NotifiableActionWorker) do
       put "/articles/#{article.id}", params: {
-        article: { published: true }
+        article: { published: true, body_markdown: "blah"  }
       }
     end
   end

spec/services/articles/attributes_spec.rb

@@ -0,0 +1,71 @@
+require "rails_helper"
+
+RSpec.describe Articles::Attributes, type: :service do
+  describe "#for_update" do
+    let(:user) { create(:user) }
+
+    context "when few attributes" do
+      let(:few_attributes) do
+        {
+          body_markdown: "---\ntitle: Title\npublished: false\ndescription:\ntags: hey\n---\n\nHey this is the article",
+          organization_id: 2
+        }
+      end
+      let(:attrs_for_update) { described_class.new(few_attributes, user).for_update }
+
+      it "has attributes that were passed as nils" do
+        few_attributes[:title] = nil
+        attrs = described_class.new(few_attributes, user).for_update
+        expect(attrs.key?(:title)).to be true
+        expect(attrs[:title]).to be nil
+      end
+
+      it "doesn't have attributes that were not passed" do
+        expect(attrs_for_update.key?(:title)).to be false
+        expect(attrs_for_update.key?(:video_thumbnail_url)).to be false
+      end
+
+      it "has passed attributes" do
+        expect(attrs_for_update[:body_markdown]).to include("Hey this is the article")
+        expect(attrs_for_update[:organization_id]).to eq(2)
+      end
+    end
+
+    it "sets a collection when :series was passed" do
+      series_attrs = described_class.new({ series: "slug", title: "title" }, user).for_update
+      expect(series_attrs[:collection]).to be_a(Collection)
+      expect(series_attrs[:series]).to be nil
+    end
+
+    it "resets the collection when empty :series was passed" do
+      no_series_attrs = described_class.new({ series: "" }, user).for_update
+      expect(no_series_attrs[:collection]).to be nil
+      expect(no_series_attrs[:series]).to be nil
+    end
+
+    it "does not reset the collection when no :series was passed" do
+      no_series_attrs = described_class.new({ title: "hello" }, user).for_update
+      expect(no_series_attrs.key?(:collection)).to be false
+    end
+
+    it "sets tag_list when tags were passed" do
+      tags_attrs = described_class.new({ tags: %w[ruby cpp], title: "title" }, user).for_update
+      expect(tags_attrs[:tag_list]).to eq("ruby, cpp")
+    end
+
+    it "sets tag_list when tag_list was passed" do
+      tags_attrs = described_class.new({ tag_list: "ruby, cpp", title: "title" }, user).for_update
+      expect(tags_attrs[:tag_list]).to eq("ruby, cpp")
+    end
+
+    it "sets edited_at if update_edited_at is true" do
+      attrs = described_class.new({ title: "title" }, user).for_update(update_edited_at: true)
+      expect(attrs[:edited_at]).to be_truthy
+    end
+
+    it "doesn't set edited_at if update_edited_at is false" do
+      attrs = described_class.new({ title: "title" }, user).for_update(update_edited_at: false)
+      expect(attrs[:edited_at]).to be_falsey
+    end
+  end
+end

spec/services/articles/updater_spec.rb

@@ -7,11 +7,70 @@ RSpec.describe Articles::Updater, type: :service do
   let(:draft) { create(:article, user: user, published: false) }
 
   it "updates an article" do
-    described_class.call(user, article.id, attributes)
+    described_class.call(user, article, attributes)
     article.reload
     expect(article.body_markdown).to eq("sample")
   end
 
+  it "sets a collection" do
+    attributes[:series] = "collection-slug"
+    described_class.call(user, article, attributes)
+    article.reload
+    expect(article.collection).to be_a(Collection)
+  end
+
+  it "creates a collection for the user, not admin when updated by admin" do
+    admin = create(:user, :super_admin)
+    attributes[:series] = "new-slug"
+    described_class.call(admin, article, attributes)
+    expect(article.reload.collection.user).to eq(article.user)
+  end
+
+  it "sets tags" do
+    attributes[:tags] = %w[ruby productivity]
+    described_class.call(user, article, attributes)
+    article.reload
+    expect(article.tags.pluck(:name).sort).to eq(%w[productivity ruby])
+  end
+
+  describe "result" do
+    it "returns success when saved" do
+      result = described_class.call(user, article, attributes)
+      expect(result.success).to be true
+      expect(result.article).to be_a(ArticleDecorator)
+    end
+
+    it "returns not success when not saved" do
+      invalid_attributes = { body_markdown: nil }
+      result = described_class.call(user, article, invalid_attributes)
+      expect(result.success).to be false
+      expect(result.article.errors.any?).to be true
+    end
+  end
+
+  describe "notifications" do
+    it "sends notifications when an article was published" do
+      attributes[:published] = true
+      sidekiq_assert_enqueued_with(job: Notifications::NotifiableActionWorker) do
+        described_class.call(user, draft, attributes)
+      end
+    end
+
+    it "doesn't send when an article was unpublished" do
+      attributes[:published] = false
+      sidekiq_assert_not_enqueued_with(job: Notifications::NotifiableActionWorker) do
+        described_class.call(user, article, attributes)
+      end
+    end
+
+    it "doesn't send when an article went from published to published" do
+      attributes[:published] = true
+      sidekiq_assert_not_enqueued_with(job: Notifications::NotifiableActionWorker) do
+        described_class.call(user, article, attributes)
+      end
+    end
+  end
+
   describe "events dispatcher" do
     let(:event_dispatcher) { double }
 
@@ -20,24 +79,24 @@ RSpec.describe Articles::Updater, type: :service do
     end
 
     it "calls the dispatcher" do
-      described_class.call(user, article.id, attributes, event_dispatcher)
+      described_class.call(user, article, attributes, event_dispatcher)
       expect(event_dispatcher).to have_received(:call).with("article_updated", article)
     end
 
     it "doesn't call the dispatcher when unpublished => unpublished" do
-      described_class.call(user, draft.id, attributes, event_dispatcher)
+      described_class.call(user, draft, attributes, event_dispatcher)
       expect(event_dispatcher).not_to have_received(:call)
     end
 
     it "calls the dispatcher when unpublished => published" do
       attributes[:published] = true
-      described_class.call(user, draft.id, attributes, event_dispatcher)
+      described_class.call(user, draft, attributes, event_dispatcher)
       expect(event_dispatcher).to have_received(:call).with("article_updated", draft)
     end
 
     it "calls the dispatcher when published => unpublished" do
       attributes[:published] = false
-      described_class.call(user, article.id, attributes, event_dispatcher)
+      described_class.call(user, article, attributes, event_dispatcher)
       expect(event_dispatcher).to have_received(:call).with("article_updated", article)
     end
   end