Merge pull request #1867 from codidact/0valt/1859/tag-drafts

Fix tag drafts not being deleted upon creation / update

Author: Oaphi

app/controllers/concerns/draft_management.rb

@@ -0,0 +1,56 @@
+module DraftManagement
+  extend ActiveSupport::Concern
+
+  DRAFTABLE_FIELDS = [:body, :comment, :excerpt, :license, :saved_at, :tags, :tag_name, :title].freeze
+  NESTED_DRAFTABLE_FIELDS = [:body, :saved_at].freeze
+  TOP_LEVEL_DRAFTABLE_FIELDS = [:body, :comment, :excerpt, :license, :tag_name, :tags, :title].freeze
+  DRAFT_MAX_AGE = 86_400 * 7
+
+  # saving by-field is kept for backwards compatibility with old drafts
+  # @param user [User] user to save the draft for
+  # @param path [String] draft path to save
+  # @return [String] top level field key
+  def do_save_draft(user, path)
+    base_key = "saved_post.#{user.id}.#{path}"
+
+    TOP_LEVEL_DRAFTABLE_FIELDS.each do |key|
+      next unless params.key?(key)
+
+      key_name = NESTED_DRAFTABLE_FIELDS.include?(key) ? base_key : "#{base_key}.#{key}"
+
+      if key == :tags
+        valid_tags = params[key]&.select(&:present?)
+
+        RequestContext.redis.del(key_name)
+
+        if valid_tags.present?
+          RequestContext.redis.sadd(key_name, valid_tags)
+        end
+      else
+        RequestContext.redis.set(key_name, params[key])
+      end
+
+      RequestContext.redis.expire(key_name, DRAFT_MAX_AGE)
+    end
+
+    saved_at_key = "saved_post_at.#{user.id}.#{path}"
+    RequestContext.redis.set(saved_at_key, DateTime.now.iso8601)
+    RequestContext.redis.expire(saved_at_key, DRAFT_MAX_AGE)
+
+    base_key
+  end
+
+  # Attempts to delete a draft for a given path
+  # @param user [User] user to delete the draft for
+  # @param path [String] draft path to delete
+  # @return [Boolean] status of the operation
+  def do_delete_draft(user, path)
+    keys = DRAFTABLE_FIELDS.map do |key|
+      pfx = key == :saved_at ? 'saved_post_at' : 'saved_post'
+      base = "#{pfx}.#{user.id}.#{path}"
+      NESTED_DRAFTABLE_FIELDS.include?(key) ? base : "#{base}.#{key}"
+    end
+
+    RequestContext.redis.del(*keys)
+  end
+end

app/controllers/posts_controller.rb

@@ -1,6 +1,8 @@
 # rubocop:disable Metrics/ClassLength
 # rubocop:disable Metrics/MethodLength
 class PostsController < ApplicationController
+  include DraftManagement
+
   before_action :authenticate_user!, except: [:document, :help_center, :show]
   before_action :set_post, only: [:toggle_comments, :feature, :lock, :unlock]
   before_action :set_scoped_post, only: [:change_category, :show, :edit, :update, :close, :reopen, :delete, :restore]
@@ -111,7 +113,7 @@ def create
         Rails.cache.delete "community_user/#{current_user.community_user.id}/metric/#{key}"
       end
 
-      do_draft_delete(URI(request.referer || '').path)
+      do_delete_draft(current_user, URI(request.referer || '').path)
 
       redirect_to helpers.generic_show_link(@post)
     else
@@ -261,7 +263,7 @@ def update
               PostHistory.redact(@post, current_user)
             end
             Rails.cache.delete "community_user/#{current_user.community_user.id}/metric/E"
-            do_draft_delete(URI(request.referer || '').path)
+            do_delete_draft(current_user, URI(request.referer || '').path)
             redirect_to post_path(@post)
           end
 
@@ -300,7 +302,7 @@ def update
             message += " on '#{@post.parent.title}'"
           end
           @post.user.create_notification message, suggested_edit_url(edit, host: @post.community.host)
-          do_draft_delete(URI(request.referer || '').path)
+          do_delete_draft(current_user, URI(request.referer || '').path)
           redirect_to post_path(@post)
         else
           @post.errors.copy!(edit.errors)
@@ -631,41 +633,13 @@ def feature
     render json: { status: 'success', success: true }
   end
 
-  # saving by-field is kept for backwards compatibility with old drafts
   def save_draft
-    expiration_time = 86_400 * 7
-
-    base_key = "saved_post.#{current_user.id}.#{params[:path]}"
-
-    [:body, :comment, :excerpt, :license, :tag_name, :tags, :title].each do |key|
-      next unless params.key?(key)
-
-      key_name = [:body, :saved_at].include?(key) ? base_key : "#{base_key}.#{key}"
-
-      if key == :tags
-        valid_tags = params[key]&.select(&:present?)
-
-        RequestContext.redis.del(key_name)
-
-        if valid_tags.present?
-          RequestContext.redis.sadd(key_name, valid_tags)
-        end
-      else
-        RequestContext.redis.set(key_name, params[key])
-      end
-
-      RequestContext.redis.expire(key_name, expiration_time)
-    end
-
-    saved_at_key = "saved_post_at.#{current_user.id}.#{params[:path]}"
-    RequestContext.redis.set(saved_at_key, DateTime.now.iso8601)
-    RequestContext.redis.expire(saved_at_key, expiration_time)
-
+    base_key = do_save_draft(current_user, params[:path])
     render json: { status: 'success', success: true, key: base_key }
   end
 
   def delete_draft
-    do_draft_delete(params[:path])
+    do_delete_draft(current_user, params[:path])
     render json: { status: 'success', success: true }
   end
 
@@ -726,19 +700,6 @@ def edit_checks
   def unless_locked
     check_if_locked(@post)
   end
-
-  # Attempts to actually delete a post draft
-  # @param path [String] draft path to delete
-  # @return [Boolean] status of the operation
-  def do_draft_delete(path)
-    keys = [:body, :comment, :excerpt, :license, :saved_at, :tags, :tag_name, :title].map do |key|
-      pfx = key == :saved_at ? 'saved_post_at' : 'saved_post'
-      base = "#{pfx}.#{current_user.id}.#{path}"
-      [:body, :saved_at].include?(key) ? base : "#{base}.#{key}"
-    end
-
-    RequestContext.redis.del(*keys)
-  end
 end
 # rubocop:enable Metrics/MethodLength
 # rubocop:enable Metrics/ClassLength

app/controllers/tags_controller.rb

@@ -1,4 +1,6 @@
 class TagsController < ApplicationController
+  include DraftManagement
+
   before_action :authenticate_user!, only: [:new, :create, :edit, :update, :rename, :merge, :select_merge]
   before_action :set_category, except: [:index]
   before_action :set_tag, only: [:show, :edit, :update, :children, :rename, :merge, :select_merge, :nuke, :nuke_warning]
@@ -92,8 +94,11 @@ def new
   end
 
   def create
-    @tag = Tag.new(tag_params.merge(tag_set_id: @category.tag_set.id))
+    create_params = tag_params.merge(tag_set_id: @category.tag_set.id)
+
+    @tag = Tag.new(create_params)
     if @tag.save
+      do_delete_draft(current_user, URI(request.referer || '').path)
       redirect_to tag_path(id: @category.id, tag_id: @tag.id)
     else
       render :new, status: :bad_request
@@ -109,7 +114,11 @@ def update
     return unless check_your_privilege('edit_tags', nil, true)
 
     wiki_md = params[:tag][:wiki_markdown]
-    if @tag.update(tag_params.merge(wiki: wiki_md.present? ? helpers.render_markdown(wiki_md) : nil).except(:name))
+    update_params = tag_params.merge(wiki: wiki_md.present? ? helpers.render_markdown(wiki_md) : nil)
+                              .except(:name)
+
+    if @tag.update(update_params)
+      do_delete_draft(current_user, URI(request.referer || '').path)
       redirect_to tag_path(id: @category.id, tag_id: @tag.id)
     else
       render :edit, status: :bad_request
@@ -263,8 +272,7 @@ def exec_sql(sql_array)
   end
 
   def verify_tag_editor
-    unless user_signed_in? && (current_user.privilege?(:edit_tags) ||
-      current_user.at_least_moderator?)
+    unless user_signed_in? && current_user.can_edit_tags?
       respond_to do |format|
         format.html do
           render 'errors/not_found', layout: 'without_sidebar', status: :not_found

app/models/user.rb

@@ -138,6 +138,12 @@ def can_delete?(target)
     privilege?('flag_curate') && !target.deleted?
   end
 
+  # Can the user edit tags?
+  # @return [Boolean] check result
+  def can_edit_tags?
+    privilege?('edit_tags') || false
+  end
+
   # Can the user handle flags?
   # @return [Boolean] check result
   def can_handle_flags?

test/controllers/tags_controller_test.rb

@@ -99,6 +99,54 @@ class TagsControllerTest < ActionController::TestCase
     assert_not_nil assigns(:posts)
   end
 
+  test ':create should require authentication' do
+    try_create_tag(categories(:main), tag_sets(:main))
+
+    assert_redirected_to_sign_in
+  end
+
+  test 'only users who can edit tags should be able to create them' do
+    category = categories(:main)
+    tag_set = tag_sets(:main)
+
+    users.each do |user|
+      sign_in(user)
+
+      draft_path = create_tag_path(id: category.id)
+      tag_name = "test-tag-#{user.id}"
+
+      @controller.stub('params', { tag_name: tag_name }) do
+        @controller.do_save_draft(user, draft_path)
+      end
+
+      try_create_tag(category, tag_set, name: tag_name)
+
+      if !@controller.helpers.user_signed_in?
+        assert_redirected_to_sign_in
+      elsif user.can_edit_tags?
+        assert_response(:found, "Expected user '#{user.name}' to be able to create tags")
+        assert_draft_deleted(user, draft_path, :tag_name)
+      else
+        assert_response(:not_found, "Expected user '#{user.name}' to not be able to create tags")
+      end
+    end
+  end
+
+  test ':create should correctly create tags' do
+    sign_in users(:tags_editor)
+
+    category = categories(:main)
+    tag_set = tag_sets(:main)
+    tag_name = 'test-tag'
+
+    try_create_tag(category, tag_set, name: tag_name)
+
+    @tag = assigns(:tag)
+
+    assert_not_nil @tag
+    assert_redirected_to(tag_path(id: category.id, tag_id: @tag.id))
+  end
+
   test 'should deny edit to anonymous user' do
     get :edit, params: { id: categories(:main).id, tag_id: tags(:topic).id }
 
@@ -234,6 +282,23 @@ class TagsControllerTest < ActionController::TestCase
 
   private
 
+  # @param category [Category] category to create the tag in
+  # @param tag_set [TagSet] tag set the tag belongs to
+  def try_create_tag(tag_set, category, **opts)
+    # this is needed for draft deletion until we switch from request.referer
+    @request.set_header('HTTP_REFERER', create_tag_path(id: category.id))
+
+    post :create, params: {
+      id: category.id,
+      tag: {
+        excerpt: 'Usage guidance goes here',
+        name: 'test-tag',
+        tag_set_id: tag_set.id,
+        wiki_markdown: 'Extended tag description goes here'
+      }.merge(opts)
+    }
+  end
+
   # @param category [Category] category to rename the tag in
   # @param tag [Tag] tag to rename
   # @param name [String] new tag name

test/fixtures/community_users.yml

@@ -26,6 +26,13 @@ sample_editor:
   is_moderator: false
   reputation: 501
 
+sample_tags_editor:
+  user: tags_editor
+  community: sample
+  is_admin: false
+  is_moderator: false
+  reputation: 42
+
 sample_deleter:
   user: deleter
   community: sample

test/fixtures/user_abilities.yml

@@ -10,6 +10,10 @@ e_eo:
   community_user: sample_editor
   ability: everyone
 
+et_eo:
+  community_user: sample_tags_editor
+  ability: everyone
+
 d_eo:
   community_user: sample_deleter
   ability: everyone
@@ -38,6 +42,10 @@ e_ur:
   community_user: sample_editor
   ability: unrestricted
 
+et_ur:
+  community_user: sample_tags_editor
+  ability: unrestricted
+
 d_ur:
   community_user: sample_deleter
   ability: unrestricted
@@ -78,6 +86,10 @@ d_et:
   community_user: sample_deleter
   ability: edit_tags
 
+te_et:
+  community_user: sample_tags_editor
+  ability: edit_tags
+
 b_eo:
   community_user: sample_basic_user
   ability: everyone

test/fixtures/users.yml

@@ -40,6 +40,15 @@ editor:
   login_token_expires_at: 2000-01-01T00:00:00.000000Z
   confirmed_at: 2020-01-01T00:00:00.000000Z
 
+tags_editor:
+  email: editor@example.com
+  encrypted_password: '$2a$11$roUHXKxecjyQ72Qn7DWs3.9eRCCoRn176kX/UNb/xiue3aGqf7xEW'
+  sign_in_count: 1337
+  username: tags-editor
+  is_global_admin: false
+  is_global_moderator: false
+  confirmed_at: 2020-01-01T00:00:00.000000Z
+
 deleter:
   email: delete@qpixel-test.net
   encrypted_password: '$2a$11$roUHXKxecjyQ72Qn7DWs3.9eRCCoRn176kX/UNb/xiue3aGqf7xEW'

test/test_helper.rb

@@ -129,6 +129,16 @@ def copy_abilities(community_id)
     end
   end
 
+  def assert_draft_deleted(user, path, *fields)
+    base_key = "saved_post.#{user.id}.#{path}"
+
+    fields.each do |key|
+      key_name = DraftManagement::NESTED_DRAFTABLE_FIELDS.include?(key) ? base_key : "#{base_key}.#{key}"
+      assert_not(RequestContext.redis.exists?(key_name),
+                 "Expected '#{key_name}' draft to be deleted")
+    end
+  end
+
   def assert_valid_json_response
     assert_nothing_raised do
       parsed = JSON.parse(response.body)