diff --git a/app/controllers/api/v0/articles_controller.rb b/app/controllers/api/v0/articles_controller.rb index c7616160e..9450d652a 100644 --- a/app/controllers/api/v0/articles_controller.rb +++ b/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 diff --git a/app/controllers/articles_controller.rb b/app/controllers/articles_controller.rb index e79bffef1..dafe84989 100644 --- a/app/controllers/articles_controller.rb +++ b/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 diff --git a/app/services/articles/attributes.rb b/app/services/articles/attributes.rb new file mode 100644 index 000000000..a9ddf91f2 --- /dev/null +++ b/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 diff --git a/app/services/articles/updater.rb b/app/services/articles/updater.rb index c12968cbe..7db230509 100644 --- a/app/services/articles/updater.rb +++ b/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 diff --git a/spec/requests/api/v0/articles_spec.rb b/spec/requests/api/v0/articles_spec.rb index cc4c33f8e..1f15b624a 100644 --- a/spec/requests/api/v0/articles_spec.rb +++ b/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 diff --git a/spec/requests/articles/articles_update_spec.rb b/spec/requests/articles/articles_update_spec.rb index 2d67677a2..96cf095e0 100644 --- a/spec/requests/articles/articles_update_spec.rb +++ b/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 diff --git a/spec/services/articles/attributes_spec.rb b/spec/services/articles/attributes_spec.rb new file mode 100644 index 000000000..fe5cb3133 --- /dev/null +++ b/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 diff --git a/spec/services/articles/updater_spec.rb b/spec/services/articles/updater_spec.rb index c33df95e6..987c31f8c 100644 --- a/spec/services/articles/updater_spec.rb +++ b/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