Merge pull request #1727 from codidact/0valt/pinned_links

Fixes for featured (pinned) links

Author: Oaphi

app/controllers/pinned_links_controller.rb

@@ -27,27 +27,42 @@ def new
   end
 
   def create
-    @link = PinnedLink.create pinned_link_params
+    @link = PinnedLink.new(pinned_link_params)
 
-    attr = @link.attributes_print
-    AuditLog.moderator_audit(event_type: 'pinned_link_create', related: @link, user: current_user,
-                             comment: "<<PinnedLink #{attr}>>")
+    if @link.save
+      attr = @link.attributes_print
 
-    flash[:success] = 'Your pinned link has been created. Due to caching, it may take some time until it is shown.'
-    redirect_to pinned_links_path
+      AuditLog.moderator_audit(event_type: 'pinned_link_create',
+                               related: @link,
+                               user: current_user,
+                               comment: "<<PinnedLink #{attr}>>")
+
+      flash[:success] =
+        'Your pinned link has been created. Due to caching, it may take some time until it is shown.'
+      redirect_to pinned_links_path
+    else
+      render 'pinned_links/new', status: :bad_request
+    end
   end
 
   def edit; end
 
   def update
     before = @link.attributes_print
-    @link.update pinned_link_params
-    after = @link.attributes_print
-    AuditLog.moderator_audit(event_type: 'pinned_link_update', related: @link, user: current_user,
-                             comment: "from <<PinnedLink #{before}>>\nto <<PinnedLink #{after}>>")
 
-    flash[:success] = 'The pinned link has been updated. Due to caching, it may take some time until it is shown.'
-    redirect_to pinned_links_path
+    if @link.update(pinned_link_params)
+      after = @link.attributes_print
+
+      AuditLog.moderator_audit(event_type: 'pinned_link_update',
+                               related: @link,
+                               user: current_user,
+                               comment: "from <<PinnedLink #{before}>>\nto <<PinnedLink #{after}>>")
+
+      flash[:success] = 'The pinned link has been updated. Due to caching, it may take some time until it is shown.'
+      redirect_to pinned_links_path
+    else
+      render 'pinned_links/edit', status: :bad_request
+    end
   end
 
   private

app/models/pinned_link.rb

@@ -1,10 +1,18 @@
 class PinnedLink < ApplicationRecord
   include MaybeCommunityRelated
-  belongs_to :post
+  belongs_to :post, optional: true
+
+  validate :check_post_or_url
 
   # Is the link time-constrained?
   # @return [Boolean] check result
   def timed?
     shown_before.present? || shown_after.present?
   end
+
+  def check_post_or_url
+    unless post_id.present? || link.present?
+      errors.add(:base, 'either a post or a URL must be set')
+    end
+  end
 end

test/controllers/pinned_links_controller_test.rb

@@ -3,31 +3,93 @@
 class PinnedLinksControllerTest < ActionController::TestCase
   include Devise::Test::ControllerHelpers
 
-  test 'edit should require moderator' do
-    sign_in users(:standard_user)
-    get :edit, params: { id: pinned_links(:active_with_label).id }
-    assert_response(:not_found)
+  test 'only mods or higher should be able to create pinned links' do
+    post = posts(:question_one)
+
+    users.each do |user|
+      sign_in user
+      try_create_pinned_link(post: post)
+      assert_response(user.at_least_moderator? ? :found : :not_found)
+    end
+  end
+
+  test 'only mods or higher should be able to edit pinned links' do
+    link = pinned_links(:active_with_label)
+
+    users.each do |user|
+      sign_in user
+      try_edit_pinned_link(link)
+      assert_response(user.at_least_moderator? ? :success : :not_found)
+    end
+  end
+
+  test 'only mods or higher should be able to update pinned links' do
+    link = pinned_links(:active_with_label)
+
+    users.each do |user|
+      sign_in user
+      try_update_pinned_link(link, label: 'updated label')
+      assert_response(user.at_least_moderator? ? :found : :not_found)
+    end
   end
 
-  test 'edit should work for moderators' do
+  test 'create should correctly create pinned links' do
     sign_in users(:moderator)
-    get :edit, params: { id: pinned_links(:active_with_label).id }
-    assert_response(:success)
+
+    try_create_pinned_link(post: posts(:question_one))
+
+    assert_response(:found)
+    assert_redirected_to pinned_links_path
     assert_not_nil assigns(:link)
   end
 
-  test 'update should require moderator' do
-    sign_in users(:standard_user)
-    post :update, params: { id: pinned_links(:active_with_label).id, pinned_link: { label: 'updated label' } }
-    assert_response(:not_found)
+  test 'create should correctly handle invalid pinned links' do
+    sign_in users(:moderator)
+    try_create_pinned_link
+    assert_response(:bad_request)
+    assert assigns(:link)&.errors&.any?
   end
 
-  test 'update should work for moderators' do
+  test 'update should correctly update pinned links' do
     sign_in users(:moderator)
-    post :update, params: { id: pinned_links(:active_with_label).id, pinned_link: { label: 'updated label' } }
+
+    try_update_pinned_link(pinned_links(:active_with_label), label: 'updated label')
+
     assert_response(:found)
     assert_redirected_to pinned_links_path
     assert_not_nil assigns(:link)
     assert_equal 'updated label', assigns(:link).label
   end
+
+  test 'update should correctly handle invlid pinned links' do
+    sign_in users(:moderator)
+    try_update_pinned_link(pinned_links(:active_with_label), link: nil)
+    assert_response(:bad_request)
+    assert assigns(:link)&.errors&.any?
+  end
+
+  private
+
+  def try_create_pinned_link(**opts)
+    community_id = opts.delete(:community)&.id
+    post_id = opts.delete(:post)&.id
+
+    post :create, params: {
+      pinned_link: {
+        community_id: community_id,
+        post_id: post_id
+      }.merge(opts)
+    }
+  end
+
+  def try_edit_pinned_link(link)
+    get :edit, params: { id: link.id }
+  end
+
+  def try_update_pinned_link(link, **opts)
+    post :update, params: {
+      id: link.id,
+      pinned_link: opts
+    }
+  end
 end