From e5ee2f9013be242b6623a40fd8cfa7658111c33e Mon Sep 17 00:00:00 2001 From: Spencer Date: Thu, 21 Nov 2019 13:32:32 -0800 Subject: [PATCH] Mod tooling: add tags via /mod (#4750) [deploy] * added tests to specs * added view logic * added controller/model functionality * minor tweak to tagadjustment view * adjusted mod view to reduce query * added instance variables to controller * show already adjusted tags in mod view * removed unnecessary ifs from mods controller * catch and display errors to tag adjustment forms * update specs and refactor --- app/controllers/moderations_controller.rb | 5 ++ app/controllers/tag_adjustments_controller.rb | 17 ++++- app/models/article.rb | 7 ++ app/models/tag_adjustment.rb | 2 +- .../tag_adjustment_creation_service.rb | 11 ++- app/views/moderations/mod.html.erb | 70 ++++++++++++++++-- .../notifications/_tagadjustment.html.erb | 11 ++- spec/requests/tag_adjustments_spec.rb | 73 ++++++++++++++++++- .../tag_adjustment_notification/send_spec.rb | 7 ++ .../tag_adjustment_creation_service_spec.rb | 54 ++++++++++---- .../tag_adjustment_update_service_spec.rb | 5 +- 11 files changed, 226 insertions(+), 36 deletions(-) diff --git a/app/controllers/moderations_controller.rb b/app/controllers/moderations_controller.rb index d5ece75eb..a40e1584f 100644 --- a/app/controllers/moderations_controller.rb +++ b/app/controllers/moderations_controller.rb @@ -18,6 +18,11 @@ class ModerationsController < ApplicationController def article authorize(User, :moderation_routes?) @moderatable = Article.find_by(slug: params[:slug]) + @adjustments = TagAdjustment.where(article_id: @moderatable.id) + @removed_adjustments = @adjustments.filter { |a| a.adjustment_type == "removal" } + @added_adjustments = @adjustments.filter { |a| a.adjustment_type == "addition" } + @already_adjusted_tags = @adjustments.map(&:tag_name).join(", ") + @allowed_to_add = @moderatable.class.name == "Article" && (current_user.has_role?(:super_admin) || current_user.has_role?(:tag_moderator, :any)) render template: "moderations/mod" end diff --git a/app/controllers/tag_adjustments_controller.rb b/app/controllers/tag_adjustments_controller.rb index f3d5dc2e5..8ed51359d 100644 --- a/app/controllers/tag_adjustments_controller.rb +++ b/app/controllers/tag_adjustments_controller.rb @@ -1,14 +1,22 @@ class TagAdjustmentsController < ApplicationController def create authorize(User, :moderation_routes?) - TagAdjustmentCreationService.new( + service = TagAdjustmentCreationService.new( current_user, - adjustment_type: "removal", + adjustment_type: params[:tag_adjustment][:adjustment_type], status: "committed", tag_name: params[:tag_adjustment][:tag_name], article_id: params[:tag_adjustment][:article_id], reason_for_adjustment: params[:tag_adjustment][:reason_for_adjustment], - ).create + ) + tag_adjustment = service.tag_adjustment + if tag_adjustment.save + service.update_tags_and_notify + else + errors = tag_adjustment.errors.full_messages.join(", ") + flash[:error_removal] = errors if tag_adjustment.adjustment_type == "removal" + flash[:error_addition] = errors if tag_adjustment.adjustment_type == "addition" + end @article = Article.find(params[:tag_adjustment][:article_id]) redirect_to "#{URI.parse(@article.path).path}/mod" end @@ -18,7 +26,8 @@ class TagAdjustmentsController < ApplicationController tag_adjustment = TagAdjustment.find(params[:id]) tag_adjustment.destroy @article = Article.find(tag_adjustment.article_id) - @article.update!(tag_list: @article.tag_list.add(tag_adjustment.tag_name)) + @article.update!(tag_list: @article.tag_list.add(tag_adjustment.tag_name)) if tag_adjustment.adjustment_type == "removal" + @article.update!(tag_list: @article.tag_list.remove(tag_adjustment.tag_name)) if tag_adjustment.adjustment_type == "addition" redirect_to "#{URI.parse(@article.path).path}/mod" end end diff --git a/app/models/article.rb b/app/models/article.rb index f8f805d38..964042d61 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -486,6 +486,7 @@ class Article < ApplicationRecord self.tag_list = [] # overwrite any existing tag with those from the front matter tag_list.add(front_matter["tags"], parser: ActsAsTaggableOn::TagParser) remove_tag_adjustments_from_tag_list + add_tag_adjustments_to_tag_list end self.published = front_matter["published"] if %w[true false].include?(front_matter["published"].to_s) self.published_at = parse_date(front_matter["date"]) if published @@ -505,6 +506,7 @@ class Article < ApplicationRecord def validate_tag # remove adjusted tags remove_tag_adjustments_from_tag_list + add_tag_adjustments_to_tag_list # check there are not too many tags return errors.add(:tag_list, "exceed the maximum of 4 tags") if tag_list.size > 4 @@ -520,6 +522,11 @@ class Article < ApplicationRecord tag_list.remove(tags_to_remove, parser: ActsAsTaggableOn::TagParser) if tags_to_remove end + def add_tag_adjustments_to_tag_list + tags_to_add = TagAdjustment.where(article_id: id, adjustment_type: "addition", status: "committed").pluck(:tag_name) + tag_list.add(tags_to_add, parser: ActsAsTaggableOn::TagParser) if tags_to_add + end + def validate_video return errors.add(:published, "cannot be set to true if video is still processing") if published && video_state == "PROGRESSING" return errors.add(:video, "cannot be added member without permission") if video.present? && user.created_at > 2.weeks.ago diff --git a/app/models/tag_adjustment.rb b/app/models/tag_adjustment.rb index 3386130fe..f028563a2 100644 --- a/app/models/tag_adjustment.rb +++ b/app/models/tag_adjustment.rb @@ -2,7 +2,7 @@ class TagAdjustment < ApplicationRecord validates :user_id, presence: true validates :article_id, presence: true validates :tag_id, presence: true - validates :tag_name, presence: true, uniqueness: { scope: :article_id } + validates :tag_name, presence: true, uniqueness: { scope: :article_id, message: "can't be an already adjusted tag" } validates :reason_for_adjustment, presence: true validates :adjustment_type, inclusion: { in: %w[removal addition] }, presence: true validates :status, inclusion: { in: %w[committed pending committed_and_resolvable resolved] }, presence: true diff --git a/app/services/tag_adjustment_creation_service.rb b/app/services/tag_adjustment_creation_service.rb index 39659aeb1..148ddc5d6 100644 --- a/app/services/tag_adjustment_creation_service.rb +++ b/app/services/tag_adjustment_creation_service.rb @@ -4,11 +4,13 @@ class TagAdjustmentCreationService @tag_adjustment_params = tag_adjustment_params end - def create - @tag_adjustment = TagAdjustment.create!(creation_args) + def tag_adjustment + @tag_adjustment ||= TagAdjustment.new(creation_args) + end + + def update_tags_and_notify update_article - Notification.send_tag_adjustment_notification(@tag_adjustment) - @tag_adjustment + Notification.send_tag_adjustment_notification(tag_adjustment) end private @@ -16,6 +18,7 @@ class TagAdjustmentCreationService def update_article article = Article.find(creation_args[:article_id]) article.update!(tag_list: article.tag_list.remove(@tag_adjustment.tag_name)) if @tag_adjustment.adjustment_type == "removal" + article.update!(tag_list: article.tag_list.add(@tag_adjustment.tag_name)) if @tag_adjustment.adjustment_type == "addition" end def creation_args diff --git a/app/views/moderations/mod.html.erb b/app/views/moderations/mod.html.erb index e59cf111a..1a630b3f0 100644 --- a/app/views/moderations/mod.html.erb +++ b/app/views/moderations/mod.html.erb @@ -91,6 +91,11 @@ input.undo { background: #ff0000; /* $red */ } + + .adjustment-error { + text-align:center; + color:red; + }
@@ -132,20 +137,27 @@
  • Only remove tags which are tagged innapropriately.
  • Use negative reactions if tag is appropriate but quality is low.
  • Always be friendly in your tag removal reason.
    • +
  • Only remove tags which haven't been adjusted already.
    • <% end %> - Current live tags: <%= @moderatable.tag_list %> + Current live tags: <%= @moderatable.tag_list %>
    + Already adjusted tags: <%= @already_adjusted_tags %>

    + <% if flash[:error_removal].present? %> +
    +

    Error: <%= flash[:error_removal] %>


    +
    + <% end %> <%= f.hidden_field :article_id, value: @moderatable.id %> - <%= f.text_field :tag_name, placeholder: "Tag Name" %> - <%= f.text_area :reason_for_adjustment, placeholder: "Reason for Removal (Be super kind) - Only the reason is needed, the notification will take care of the rest." %> + <%= f.hidden_field :adjustment_type, value: "removal" %> + <%= f.text_field :tag_name, placeholder: "Tag Name", required: true %> + <%= f.text_area :reason_for_adjustment, placeholder: "Reason for Removal (Be super kind) - Only the reason is needed, the notification will take care of the rest.", required: true %> <%= f.submit "Remove Tag" %> <% end %> - <% adjustments = TagAdjustment.where(article_id: @moderatable.id, adjustment_type: "removal") %> - <% if adjustments.any? %> + <% if @removed_adjustments.any? %>

    Removed tags: - <% adjustments.each do |adjustment| %> + <% @removed_adjustments.each do |adjustment| %> <% if current_user.any_admin? %> <%= form_for(adjustment, url: "/tag_adjustments/#{adjustment.id}", html: { method: :delete, onsubmit: "return confirm('Are you sure you want to undo the removal of the #{adjustment.tag_name} tag?')" }) do |f| %> <%= adjustment.tag_name %> @@ -156,6 +168,52 @@ <% end %>
    <% end %> + + <% if @allowed_to_add %> +
    +

    Add Appropriate Tags

    + <% if @moderatable.tag_list.count < 4 %> + <%= form_for(TagAdjustment.new) do |f| %> + <% unless current_user.has_role?(:super_admin) %> + + <% end %> + Current live tags: <%= @moderatable.tag_list %>
    + Already adjusted tags: <%= @already_adjusted_tags %> +

    + <% if flash[:error_addition].present? %> +
    +

    Error: <%= flash[:error_addition] %>


    +
    + <% end %> + <%= f.hidden_field :article_id, value: @moderatable.id %> + <%= f.hidden_field :adjustment_type, value: "addition" %> + <%= f.text_field :tag_name, placeholder: "Tag Name - only add one at a time", required: true %> + <%= f.text_area :reason_for_adjustment, placeholder: "Reason for addition (Be super kind) - Only the reason is needed, the notification will take care of the rest.", required: true %> + <%= f.submit "Add Tag" %> + <% end %> + <% else %> + Unable to add new tags. 4 tags max. + <% end %> + <% if @added_adjustments.any? %> +

    + Added tags: + <% @added_adjustments.each do |adjustment| %> + <% if current_user.any_admin? %> + <%= form_for(adjustment, url: "/tag_adjustments/#{adjustment.id}", html: { method: :delete, onsubmit: "return confirm('Are you sure you want to undo the addition of the #{adjustment.tag_name} tag?')" }) do |f| %> + <%= adjustment.tag_name %> + <%= f.submit "X", id: "undo" %> + <% end %> + <% end %> + <% end %> + <% end %> +
    + <% end %> + <% if @moderatable.class.name == "Article" %> <% if @moderatable.last_buffered.blank? %>
    diff --git a/app/views/notifications/_tagadjustment.html.erb b/app/views/notifications/_tagadjustment.html.erb index b46030a40..cd5346e87 100644 --- a/app/views/notifications/_tagadjustment.html.erb +++ b/app/views/notifications/_tagadjustment.html.erb @@ -4,20 +4,23 @@
    <% data = notification.json_data %> + <% removal = data["adjustment_type"] == "removal" %> Hey there 👋

    - Moderators removed the - " style="font-weight: 900">#<%= data["tag_name"] %> tag from your post + Moderators <%= removal ? "removed" : "added" %> the + " style="font-weight: 900">#<%= data["tag_name"] %> tag <%= removal ? "from" : "to" %> your post "><%= h(data["article"]["title"]) %>.

    - Certain tags have special submission guidelines, and mods need to adjust tags to best fit the community. + <%= removal ? "Certain tags have special submission guidelines, and mods need to adjust tags to best fit the community." : "Mods felt this tag represented your article." %>

    Reason for action
    <%= data["reason_for_adjustment"] %>

    - Check out other popular tags which may be more appropriate. + <% if removal %> + Check out other popular tags which may be more appropriate + <% end %>

    Thanks for being part of DEV! If you feel like this mod action was a mistake, feel free to contact "><%= ApplicationConfig["DEFAULT_SITE_EMAIL"] %>. 🤗 diff --git a/spec/requests/tag_adjustments_spec.rb b/spec/requests/tag_adjustments_spec.rb index 155fdefdc..80befa6c6 100644 --- a/spec/requests/tag_adjustments_spec.rb +++ b/spec/requests/tag_adjustments_spec.rb @@ -8,7 +8,8 @@ RSpec.describe "TagAdjustments", type: :request do { tag_name: tag.name, article_id: article.id, - reason_for_adjustment: "Test #{rand(100)}" + reason_for_adjustment: "Test #{rand(100)}", + adjustment_type: "removal" } end @@ -48,6 +49,43 @@ RSpec.describe "TagAdjustments", type: :request do end end + describe "POST /tag_adjustments with adjustment_type addition" do + before do + tag_adjustment_params[:adjustment_type] = "addition" + user.add_role(:tag_moderator, tag) + user.add_role(:trusted) + sign_in user + post "/tag_adjustments", params: { + tag_adjustment: tag_adjustment_params, + article_id: article.id + } + end + + context "when an article doesn't use front matter" do + let(:article) { Article.create(user: user, title: "something", body_markdown: "blah blah #{rand(100)}", tag_list: "yoyo, bobo", published: true) } + + it "adds the tag" do + expect(article.reload.tag_list.include?(tag.name)).to be true + end + + it "keeps the other tags" do + expect(article.reload.tag_list.include?("yoyo")).to be true + end + end + + context "when an article uses front matter" do + let(:article) { create(:article, user: user, body_markdown: "---\ntitle: Hellohnnnn#{rand(1000)}\npublished: true\ntags: heyheyhey\n---\n\nHello") } + + it "adds the tag" do + expect(article.reload.tag_list.include?(tag.name)).to be true + end + + it "keeps the other tags" do + expect(article.reload.tag_list.include?("heyheyhey")).to be true + end + end + end + describe "DELETE /tag_adjustments/:id" do let(:tag_adjustment) { create(:tag_adjustment, article_id: article.id, user: user, tag: tag) } @@ -80,4 +118,37 @@ RSpec.describe "TagAdjustments", type: :request do end end end + + describe "DELETE /tag_adjustments/:id with adjustment_type addition" do + let(:tag_adjustment) { create(:tag_adjustment, article_id: article.id, user: user, tag: tag, adjustment_type: "addition") } + + before do + user.add_role(:admin) + user.add_role(:trusted) + tag_adjustment + sign_in user + end + + context "when an article doesn't use front matter" do + let(:article) { Article.create(user: user, title: "something", body_markdown: "blah blah #{rand(100)}", tag_list: "yoyo, bobo", published: true) } + + it "removes the added tag" do + delete "/tag_adjustments/#{tag_adjustment.id}", params: { + id: tag_adjustment.id + } + expect(article.tag_list).not_to include tag.name + end + end + + context "when an article uses front matter" do + let(:article) { create(:article, user: user, body_markdown: "---\ntitle: Hellohnnnn#{rand(1000)}\npublished: true\ntags: heyheyhey\n---\n\nHello") } + + it "removes the added tag" do + delete "/tag_adjustments/#{tag_adjustment.id}", params: { + id: tag_adjustment.id + } + expect(article.tag_list).not_to include tag.name + end + end + end end diff --git a/spec/services/notifications/tag_adjustment_notification/send_spec.rb b/spec/services/notifications/tag_adjustment_notification/send_spec.rb index 1c1ce7d8a..d57c0a701 100644 --- a/spec/services/notifications/tag_adjustment_notification/send_spec.rb +++ b/spec/services/notifications/tag_adjustment_notification/send_spec.rb @@ -36,6 +36,13 @@ RSpec.describe Notifications::TagAdjustmentNotification::Send, type: :service do expect(json["adjustment_type"]). to eq "removal" end + it "tests JSON data for addition" do + tag_adjustment.adjustment_type = "addition" + json = notification.json_data + expect(json["article"]["title"]).to start_with("Hello") + expect(json["adjustment_type"]). to eq "addition" + end + specify "notification to be inserted on DB" do expect do described_class.call(tag_adjustment) diff --git a/spec/services/tag_adjustment_creation_service_spec.rb b/spec/services/tag_adjustment_creation_service_spec.rb index 7d16eac80..8a7a8547b 100644 --- a/spec/services/tag_adjustment_creation_service_spec.rb +++ b/spec/services/tag_adjustment_creation_service_spec.rb @@ -2,37 +2,63 @@ require "rails_helper" RSpec.describe TagAdjustmentCreationService, type: :service do let(:user) { create(:user) } - let(:article) { create(:article) } + let(:article) { create(:article, tags: tag.name) } let(:tag) { create(:tag) } - def create_tag_adjustment + def create_service(type) described_class.new( user, - adjustment_type: "removal", + adjustment_type: type, status: "committed", tag_name: tag.name, article_id: article.id, reason_for_adjustment: "Test", - ).create + ) end before do user.add_role(:tag_moderator, tag) end - it "creates tag adjustment" do - tag_adjustment = create_tag_adjustment + describe "creates tag adjustment" do + it "with adjustment_type removal" do + tag_adjustment = create_service("removal").tag_adjustment + tag_adjustment.save + expect(tag_adjustment).to be_valid + expect(tag_adjustment.tag_id).to eq(tag.id) + expect(tag_adjustment.status).to eq("committed") + end - expect(tag_adjustment).to be_valid - expect(tag_adjustment.tag_id).to eq(tag.id) - expect(tag_adjustment.status).to eq("committed") + it "with adjustment_type addition" do + tag_adjustment = create_service("addition").tag_adjustment + tag_adjustment.save + expect(tag_adjustment).to be_valid + expect(tag_adjustment.tag_id).to eq(tag.id) + expect(tag_adjustment.status).to eq("committed") + end end - it "creates notification" do - perform_enqueued_jobs do - tag_adjustment = create_tag_adjustment - expect(Notification.last.user_id).to eq(article.user_id) - expect(Notification.last.json_data["adjustment_type"]).to eq(tag_adjustment.adjustment_type) + describe "creates notification" do + it "with adjustment_type removal" do + perform_enqueued_jobs do + service = create_service("removal") + tag_adjustment = service.tag_adjustment + tag_adjustment.save + service.update_tags_and_notify + expect(Notification.last.user_id).to eq(article.user_id) + expect(Notification.last.json_data["adjustment_type"]).to eq(tag_adjustment.adjustment_type) + end + end + + it "with adjustment_type addition" do + perform_enqueued_jobs do + service = create_service("addition") + tag_adjustment = service.tag_adjustment + tag_adjustment.save + service.update_tags_and_notify + expect(Notification.last.user_id).to eq(article.user_id) + expect(Notification.last.json_data["adjustment_type"]).to eq(tag_adjustment.adjustment_type) + end end end end diff --git a/spec/services/tag_adjustment_update_service_spec.rb b/spec/services/tag_adjustment_update_service_spec.rb index 8869da1b3..05576b429 100644 --- a/spec/services/tag_adjustment_update_service_spec.rb +++ b/spec/services/tag_adjustment_update_service_spec.rb @@ -13,7 +13,7 @@ RSpec.describe TagAdjustmentUpdateService, type: :service do tag_name: tag.name, article_id: article.id, reason_for_adjustment: "reasons", - ).create + ) end before do @@ -21,7 +21,8 @@ RSpec.describe TagAdjustmentUpdateService, type: :service do end it "creates tag adjustment" do - tag_adjustment = create_tag_adjustment + tag_adjustment = create_tag_adjustment.tag_adjustment + tag_adjustment.save described_class.new(tag_adjustment, status: "resolved").update expect(tag_adjustment).to be_valid