From 4cd0b2cbfe01472df3c5bdbffa96601d91141da4 Mon Sep 17 00:00:00 2001 From: evazion Date: Tue, 17 Mar 2020 05:52:38 -0500 Subject: [PATCH] pundit: convert user feedbacks to pundit. Allow users to delete feedbacks they've given to other users, not just mods. --- app/controllers/user_feedbacks_controller.rb | 32 +++-------- app/models/user_feedback.rb | 22 -------- app/policies/user_feedback_policy.rb | 25 +++++++++ .../user_feedbacks/_secondary_links.html.erb | 4 +- app/views/user_feedbacks/edit.html.erb | 4 +- app/views/user_feedbacks/index.html.erb | 18 +++--- app/views/user_feedbacks/show.html.erb | 2 +- .../user_feedbacks_controller_test.rb | 55 +++++++++++++++++-- test/unit/user_feedback_test.rb | 20 ------- 9 files changed, 95 insertions(+), 87 deletions(-) create mode 100644 app/policies/user_feedback_policy.rb diff --git a/app/controllers/user_feedbacks_controller.rb b/app/controllers/user_feedbacks_controller.rb index c283f6283..cfea4efd0 100644 --- a/app/controllers/user_feedbacks_controller.rb +++ b/app/controllers/user_feedbacks_controller.rb @@ -1,53 +1,37 @@ class UserFeedbacksController < ApplicationController - before_action :gold_only, :only => [:new, :edit, :create, :update] respond_to :html, :xml, :json, :js def new - @user_feedback = UserFeedback.new(user_feedback_params(:create)) + @user_feedback = authorize UserFeedback.new(permitted_attributes(UserFeedback)) respond_with(@user_feedback) end def edit - @user_feedback = UserFeedback.visible(CurrentUser.user).find(params[:id]) - check_privilege(@user_feedback) + @user_feedback = authorize UserFeedback.find(params[:id]) respond_with(@user_feedback) end def show - @user_feedback = UserFeedback.visible(CurrentUser.user).find(params[:id]) + @user_feedback = authorize UserFeedback.find(params[:id]) respond_with(@user_feedback) end def index - @user_feedbacks = UserFeedback.visible(CurrentUser.user).paginated_search(params, count_pages: true) + @user_feedbacks = authorize UserFeedback.visible(CurrentUser.user).paginated_search(params, count_pages: true) @user_feedbacks = @user_feedbacks.includes(:user, :creator) if request.format.html? respond_with(@user_feedbacks) end def create - @user_feedback = UserFeedback.create(user_feedback_params(:create).merge(creator: CurrentUser.user)) + @user_feedback = authorize UserFeedback.new(creator: CurrentUser.user, **permitted_attributes(UserFeedback)) + @user_feedback.save respond_with(@user_feedback) end def update - @user_feedback = UserFeedback.visible(CurrentUser.user).find(params[:id]) - check_privilege(@user_feedback) - @user_feedback.update(user_feedback_params(:update, @user_feedback)) + @user_feedback = authorize UserFeedback.find(params[:id]) + @user_feedback.update(permitted_attributes(@user_feedback)) respond_with(@user_feedback) end - - private - - def check_privilege(user_feedback) - raise User::PrivilegeError unless user_feedback.editable_by?(CurrentUser.user) - end - - def user_feedback_params(context, user_feedback = nil) - permitted_params = %i[body category] - permitted_params += %i[user_id user_name] if context == :create - permitted_params += %i[is_deleted] if context == :update && user_feedback.deletable_by?(CurrentUser.user) - - params.fetch(:user_feedback, {}).permit(permitted_params) - end end diff --git a/app/models/user_feedback.rb b/app/models/user_feedback.rb index 2a9ac785b..90690d56a 100644 --- a/app/models/user_feedback.rb +++ b/app/models/user_feedback.rb @@ -6,8 +6,6 @@ class UserFeedback < ApplicationRecord attr_accessor :disable_dmail_notification validates_presence_of :body, :category validates_inclusion_of :category, :in => %w(positive negative neutral) - validate :creator_is_gold - validate :user_is_not_creator after_create :create_dmail, unless: :disable_dmail_notification after_update(:if => ->(rec) { CurrentUser.id != rec.creator_id}) do |rec| ModAction.log(%{#{CurrentUser.name} updated user feedback for "#{rec.user.name}":/users/#{rec.user_id}}, :user_feedback_update) @@ -67,26 +65,6 @@ class UserFeedback < ApplicationRecord Dmail.create_automated(:to_id => user_id, :title => "Your user record has been updated", :body => body) end - def creator_is_gold - if !creator.is_gold? - errors[:creator] << "must be gold" - end - end - - def user_is_not_creator - if user_id == creator_id - errors[:creator] << "cannot submit feedback for yourself" - end - end - - def deletable_by?(deleter) - deleter.is_moderator? && deleter != user - end - - def editable_by?(editor) - (editor.is_moderator? && editor != user) || (creator == editor && !is_deleted?) - end - def self.available_includes [:creator, :user] end diff --git a/app/policies/user_feedback_policy.rb b/app/policies/user_feedback_policy.rb new file mode 100644 index 000000000..f7512e1c5 --- /dev/null +++ b/app/policies/user_feedback_policy.rb @@ -0,0 +1,25 @@ +class UserFeedbackPolicy < ApplicationPolicy + def create? + unbanned? && user.is_gold? && record.user_id != user.id + end + + def update? + create? && (user.is_moderator? || (record.creator_id == user.id && !record.is_deleted?)) + end + + def show? + !record.is_deleted? || can_view_deleted? + end + + def can_view_deleted? + user.is_moderator? + end + + def permitted_attributes_for_create + [:body, :category, :user_id, :user_name] + end + + def permitted_attributes_for_update + [:body, :category, :is_deleted] + end +end diff --git a/app/views/user_feedbacks/_secondary_links.html.erb b/app/views/user_feedbacks/_secondary_links.html.erb index 17c32ea87..6a52bfb9d 100644 --- a/app/views/user_feedbacks/_secondary_links.html.erb +++ b/app/views/user_feedbacks/_secondary_links.html.erb @@ -1,6 +1,6 @@ <% content_for(:secondary_links) do %> - <% if CurrentUser.is_gold? %> - <% if @user_feedback %> + <% if policy(UserFeedback.new).create? %> + <% if @user_feedback.present? && policy(@user_feedback).create? %> <%= subnav_link_to "New", new_user_feedback_path(:user_feedback => {:user_id => @user_feedback.user_id}) %> <% elsif params[:search] && params[:search][:user_id] %> <%= subnav_link_to "New", new_user_feedback_path(:user_feedback => {:user_id => params[:search][:user_id]}) %> diff --git a/app/views/user_feedbacks/edit.html.erb b/app/views/user_feedbacks/edit.html.erb index de6117083..d7571c462 100644 --- a/app/views/user_feedbacks/edit.html.erb +++ b/app/views/user_feedbacks/edit.html.erb @@ -10,9 +10,7 @@ <%= edit_form_for(@user_feedback) do |f| %> <%= f.input :category, :collection => ["positive", "neutral", "negative"], :include_blank => false %> <%= dtext_field "user_feedback", "body" %> - <% if @user_feedback.deletable_by?(CurrentUser.user) %> - <%= f.input :is_deleted, label: "Deleted" %> - <% end %> + <%= f.input :is_deleted, label: "Deleted" %> <%= f.button :submit, "Submit" %> <%= dtext_preview_button "user_feedback", "body" %> <% end %> diff --git a/app/views/user_feedbacks/index.html.erb b/app/views/user_feedbacks/index.html.erb index 4aeda7899..35a50e961 100644 --- a/app/views/user_feedbacks/index.html.erb +++ b/app/views/user_feedbacks/index.html.erb @@ -7,7 +7,7 @@ <%= f.input :creator_name, input_html: { value: params.dig(:search, :creator_name), "data-autocomplete": "user" } %> <%= f.input :body_matches, label: "Message", input_html: { value: params.dig(:search, :body_matches) } %> <%= f.input :category, collection: %w[positive negative neutral], include_blank: true, selected: params.dig(:search, :category) %> - <% if CurrentUser.is_moderator? %> + <% if policy(UserFeedback).can_view_deleted? %> <%= f.input :is_deleted, label: "Deleted", collection: %w[Yes No], include_blank: true, selected: params.dig(:search, :is_deleted) %> <% end %> <%= f.submit "Search" %> @@ -24,9 +24,9 @@ <%= render "application/update_notice", record: feedback, interval: 0.minutes %> <% end %> - <% if CurrentUser.is_moderator? %> + <% if policy(UserFeedback).can_view_deleted? %> <% t.column "Status" do |feedback| %> - <%= "deleted" if feedback.is_deleted? %> + <%= "Deleted" if feedback.is_deleted? %> <% end %> <% end %> <% t.column "Category" do |feedback| %> @@ -38,12 +38,12 @@
<%= time_ago_in_words_tagged(feedback.created_at) %>
<% end %> <% t.column column: "control" do |feedback| %> - <% if feedback.editable_by?(CurrentUser.user) %> - <%= link_to "edit", edit_user_feedback_path(feedback) %> - <% if feedback.deletable_by?(CurrentUser.user) && !feedback.is_deleted? %> - | <%= link_to "delete", user_feedback_path(feedback), method: :put, remote: true, "data-params": "user_feedback[is_deleted]=true", "data-confirm": "Are you sure you want to delete this user feedback?" %> - <% elsif feedback.deletable_by?(CurrentUser.user) && feedback.is_deleted? %> - | <%= link_to "undelete", user_feedback_path(feedback), method: :put, remote: true, "data-params": "user_feedback[is_deleted]=false" %> + <% if policy(feedback).update? %> + <%= link_to "Edit", edit_user_feedback_path(feedback) %> + <% if feedback.is_deleted? %> + | <%= link_to "Undelete", user_feedback_path(feedback), method: :put, remote: true, "data-params": "user_feedback[is_deleted]=false" %> + <% else %> + | <%= link_to "Delete", user_feedback_path(feedback), method: :put, remote: true, "data-params": "user_feedback[is_deleted]=true", "data-confirm": "Are you sure you want to delete this user feedback?" %> <% end %> <% end %> <% end %> diff --git a/app/views/user_feedbacks/show.html.erb b/app/views/user_feedbacks/show.html.erb index 05c3ce45d..83f682eae 100644 --- a/app/views/user_feedbacks/show.html.erb +++ b/app/views/user_feedbacks/show.html.erb @@ -17,7 +17,7 @@ - <% if @user_feedback.editable_by?(CurrentUser.user) %> + <% if policy(@user_feedback).update? %>

<%= link_to "Edit", edit_user_feedback_path(@user_feedback) %>

<% end %> diff --git a/test/functional/user_feedbacks_controller_test.rb b/test/functional/user_feedbacks_controller_test.rb index bfef3a94b..4c88f6f24 100644 --- a/test/functional/user_feedbacks_controller_test.rb +++ b/test/functional/user_feedbacks_controller_test.rb @@ -23,12 +23,33 @@ class UserFeedbacksControllerTest < ActionDispatch::IntegrationTest end end + context "show action" do + should "allow all users to see undeleted feedbacks" do + get user_feedback_path(@user_feedback) + assert_response :success + end + + should "allow moderators to see deleted feedbacks" do + as(@user) { @user_feedback.update!(is_deleted: true) } + get_auth user_feedback_path(@user_feedback), @mod + assert_response :success + end + end + context "index action" do should "render" do get_auth user_feedbacks_path, @user assert_response :success end + should "not allow members to see deleted feedbacks" do + as(@user) { @user_feedback.update!(is_deleted: true) } + get_auth user_feedbacks_path, @user + + assert_response :success + assert_select "tr#user-feedback-#{@user_feedback.id}", false + end + context "with search parameters" do should "render" do get_auth user_feedbacks_path, @critic, params: {:search => {:user_id => @user.id}} @@ -38,31 +59,53 @@ class UserFeedbacksControllerTest < ActionDispatch::IntegrationTest end context "create action" do - should "create a new feedback" do + should "allow gold users to create new feedbacks" do assert_difference("UserFeedback.count", 1) do post_auth user_feedbacks_path, @critic, params: {:user_feedback => {:category => "positive", :user_name => @user.name, :body => "xxx"}} + assert_response :redirect + end + end + + should "not allow users to create feedbacks for themselves" do + assert_no_difference("UserFeedback.count") do + post_auth user_feedbacks_path, @critic, params: { user_feedback: { user_id: @critic.id, category: "positive", body: "xxx" }} + assert_response 403 end end end context "update action" do - should "update the feedback" do + should "allow updating undeleted feedbacks" do put_auth user_feedback_path(@user_feedback), @critic, params: { user_feedback: { category: "positive" }} assert_redirected_to(@user_feedback) - assert("positive", @user_feedback.reload.category) + assert_equal("positive", @user_feedback.reload.category) + end + + should "not allow updating deleted feedbacks" do + as(@user) { @user_feedback.update!(is_deleted: true) } + put_auth user_feedback_path(@user_feedback), @critic, params: { user_feedback: { body: "test" }} + + assert_response 403 + end + + should "allow deleting feedbacks given to others" do + put_auth user_feedback_path(@user_feedback), @critic, params: { user_feedback: { is_deleted: true }} + + assert_response :redirect + assert_equal(true, @user_feedback.reload.is_deleted) end context "by a moderator" do - should "allow deleting feedbacks given to other users" do + should "allow updating feedbacks given to other users" do put_auth user_feedback_path(@user_feedback), @mod, params: { user_feedback: { is_deleted: "true" }} assert_redirected_to @user_feedback assert(@user_feedback.reload.is_deleted?) end - should "not allow deleting feedbacks given to themselves" do - @user_feedback = as(@critic) { create(:user_feedback, user: @mod) } + should "not allow updating feedbacks given to themselves" do + @user_feedback = create(:user_feedback, user: @mod, creator: @mod) put_auth user_feedback_path(@user_feedback), @mod, params: { id: @user_feedback.id, user_feedback: { is_deleted: "true" }} assert_response 403 diff --git a/test/unit/user_feedback_test.rb b/test/unit/user_feedback_test.rb index 85ea65b31..516a79fd2 100644 --- a/test/unit/user_feedback_test.rb +++ b/test/unit/user_feedback_test.rb @@ -17,25 +17,5 @@ class UserFeedbackTest < ActiveSupport::TestCase assert_equal(dmail, user.dmails.last.body) end end - - should "not validate if the creator is the user" do - user = FactoryBot.create(:gold_user) - feedback = build(:user_feedback, creator: user, user: user) - feedback.save - assert_equal(["You cannot submit feedback for yourself"], feedback.errors.full_messages) - end - - should "not validate if the creator is not gold" do - user = FactoryBot.create(:user) - gold = FactoryBot.create(:gold_user) - member = FactoryBot.create(:user) - - feedback = FactoryBot.create(:user_feedback, creator: gold, user: user) - assert(feedback.errors.empty?) - - feedback = build(:user_feedback, creator: member, user: user) - feedback.save - assert_equal(["You must be gold"], feedback.errors.full_messages) - end end end