diff --git a/app/controllers/artists_controller.rb b/app/controllers/artists_controller.rb index 836be23c6..41f871aad 100644 --- a/app/controllers/artists_controller.rb +++ b/app/controllers/artists_controller.rb @@ -1,16 +1,14 @@ class ArtistsController < ApplicationController respond_to :html, :xml, :json, :js - before_action :member_only, :except => [:index, :show, :show_or_new, :banned] - before_action :admin_only, :only => [:ban, :unban] - before_action :load_artist, :only => [:ban, :unban, :show, :edit, :update, :destroy, :undelete] def new - @artist = Artist.new_with_defaults(artist_params(:new)) + @artist = authorize Artist.new_with_defaults(permitted_attributes(Artist)) @artist.build_wiki_page if @artist.wiki_page.nil? respond_with(@artist) end def edit + @artist = authorize Artist.find(params[:id]) @artist.build_wiki_page if @artist.wiki_page.nil? respond_with(@artist) end @@ -20,11 +18,13 @@ class ArtistsController < ApplicationController end def ban + @artist = authorize Artist.find(params[:id]) @artist.ban!(banner: CurrentUser.user) redirect_to(artist_path(@artist), :notice => "Artist was banned") end def unban + @artist = authorize Artist.find(params[:id]) @artist.unban! redirect_to(artist_path(@artist), :notice => "Artist was unbanned") end @@ -32,35 +32,38 @@ class ArtistsController < ApplicationController def index # XXX params[:search][:name] = params.delete(:name) if params[:name] - @artists = Artist.paginated_search(params) + @artists = authorize Artist.paginated_search(params) @artists = @artists.includes(:urls, :tag) if request.format.html? respond_with(@artists) end def show - @artist = Artist.find(params[:id]) + @artist = authorize Artist.find(params[:id]) respond_with(@artist) end def create - @artist = Artist.create(artist_params) + @artist = authorize Artist.new(permitted_attributes(Artist)) + @artist.save respond_with(@artist) end def update - @artist.update(artist_params) + @artist = authorize Artist.find(params[:id]) + @artist.update(permitted_attributes(@artist)) flash[:notice] = @artist.valid? ? "Artist updated" : @artist.errors.full_messages.join("; ") respond_with(@artist) end def destroy + @artist = authorize Artist.find(params[:id]) @artist.update_attribute(:is_deleted, true) redirect_to(artist_path(@artist), :notice => "Artist deleted") end def revert - @artist = Artist.find(params[:id]) + @artist = authorize Artist.find(params[:id]) @version = @artist.versions.find(params[:version_id]) @artist.revert_to!(@version) respond_with(@artist) @@ -88,16 +91,4 @@ class ArtistsController < ApplicationController true end end - - def load_artist - @artist = Artist.find(params[:id]) - end - - def artist_params(context = nil) - permitted_params = %i[name other_names other_names_string group_name url_string notes is_deleted] - permitted_params << { wiki_page_attributes: %i[id body] } - permitted_params << :source if context == :new - - params.fetch(:artist, {}).permit(permitted_params) - end end diff --git a/app/policies/artist_policy.rb b/app/policies/artist_policy.rb new file mode 100644 index 000000000..ded6d7a6c --- /dev/null +++ b/app/policies/artist_policy.rb @@ -0,0 +1,21 @@ +class ArtistPolicy < ApplicationPolicy + def ban? + user.is_admin? && !record.is_banned? + end + + def unban? + user.is_admin? && record.is_banned? + end + + def revert? + unbanned? + end + + def permitted_attributes + [:name, :other_names, :other_names_string, :group_name, :url_string, :is_deleted, { wiki_page_attributes: [:id, :body] }] + end + + def permitted_attributes_for_new + permitted_attributes + [:source] + end +end diff --git a/app/views/artists/_secondary_links.html.erb b/app/views/artists/_secondary_links.html.erb index 0bd6e7d80..8093e374e 100644 --- a/app/views/artists/_secondary_links.html.erb +++ b/app/views/artists/_secondary_links.html.erb @@ -2,7 +2,7 @@ <%= quick_search_form_for(:any_name_or_url_matches, artists_path, "artists", autocomplete: "artist", redirect: true) %> <%= subnav_link_to "Listing", artists_path %> <%= subnav_link_to "Banned", artists_path(search: { is_banned: "true", order: "updated_at" }) %> - <% if CurrentUser.is_member? %> + <% if policy(Artist).create? %> <%= subnav_link_to "New", new_artist_path %> <% end %> <%= subnav_link_to "Recent changes", artist_versions_path %> @@ -11,23 +11,21 @@
  • |
  • <%= subnav_link_to "Posts (#{@artist.tag.try(:post_count).to_i})", posts_path(:tags => @artist.name) %> <%= subnav_link_to "Show", artist_path(@artist) %> - <% if CurrentUser.is_member? %> + <% if policy(@artist).update? %> <%= subnav_link_to "Edit", edit_artist_path(@artist), :"data-shortcut" => "e" %> <% end %> <%= subnav_link_to "History", artist_versions_path(:search => {:artist_id => @artist.id}) %> - <% if CurrentUser.is_member? %> + <% if policy(@artist).update? %> <% if @artist.is_deleted? %> <%= subnav_link_to "Undelete", artist_path(@artist, format: "js"), method: :put, data: {confirm: "Are you sure you want to undelete this artist?", params: "artist[is_deleted]=false"}, remote: true %> <% else %> <%= subnav_link_to "Delete", artist_path(@artist), method: :delete, "data-shortcut": "shift+d", "data-confirm": "Are you sure you want to delete this artist?" %> <% end %> <% end %> - <% if CurrentUser.is_admin? %> - <% if @artist.is_banned? %> - <%= subnav_link_to "Unban", unban_artist_path(@artist), :method => :put, :data => {:confirm => "Are you sure you want to unban this artist?"} %> - <% else %> - <%= subnav_link_to "Ban", ban_artist_path(@artist), :method => :put, :data => {:confirm => "Are you sure you want to ban this artist?"} %> - <% end %> + <% if policy(@artist).unban? %> + <%= subnav_link_to "Unban", unban_artist_path(@artist), :method => :put, :data => {:confirm => "Are you sure you want to unban this artist?"} %> + <% elsif policy(@artist).ban? %> + <%= subnav_link_to "Ban", ban_artist_path(@artist), :method => :put, :data => {:confirm => "Are you sure you want to ban this artist?"} %> <% end %> <% end %> <% end %> diff --git a/app/views/artists/index.html.erb b/app/views/artists/index.html.erb index deb76e6ad..df32647bd 100644 --- a/app/views/artists/index.html.erb +++ b/app/views/artists/index.html.erb @@ -30,7 +30,7 @@ <%= time_ago_in_words_tagged(artist.updated_at) %> <% end %> <% t.column column: "control" do |artist| %> - <% if CurrentUser.is_member? %> + <% if policy(artist).update? %> <%= link_to "Edit", edit_artist_path(artist) %> <% if artist.is_deleted? %> diff --git a/test/functional/artists_controller_test.rb b/test/functional/artists_controller_test.rb index 83d11fcc9..386e9cb9a 100644 --- a/test/functional/artists_controller_test.rb +++ b/test/functional/artists_controller_test.rb @@ -39,100 +39,115 @@ class ArtistsControllerTest < ActionDispatch::IntegrationTest end context "show action" do + should "work for html responses" do + get artist_path(@masao.id) + assert_response :success + end + should "work for xml responses" do get artist_path(@masao.id), as: :xml assert_response :success end - end - should "get the new page" do - get_auth new_artist_path, @user - assert_response :success - end - - should "get the show_or_new page for an existing artist" do - get_auth show_or_new_artists_path(name: "masao"), @user - assert_redirected_to(@masao) - end - - should "get the show_or_new page for a nonexisting artist" do - get_auth show_or_new_artists_path(name: "nobody"), @user - assert_response :success - end - - should "get the edit page" do - get_auth edit_artist_path(@artist.id), @user - assert_response :success - end - - should "get the show page" do - get artist_path(@artist.id) - assert_response :success - end - - should "get the show page for a negated tag" do - @artist.update(name: "-aaa") - get artist_path(@artist.id) - assert_response :success - end - - should "get the banned page" do - get banned_artists_path - assert_redirected_to artists_path(search: { is_banned: true, order: "updated_at" }) - end - - should "ban an artist" do - put_auth ban_artist_path(@artist.id), @admin - assert_redirected_to(@artist) - @artist.reload - assert_equal(true, @artist.is_banned?) - assert_equal(true, TagImplication.exists?(antecedent_name: @artist.name, consequent_name: "banned_artist")) - end - - should "unban an artist" do - as_admin do - @artist.ban! - end - - put_auth unban_artist_path(@artist.id), @admin - assert_redirected_to(@artist) - @artist.reload - assert_equal(false, @artist.is_banned?) - assert_equal(false, TagImplication.exists?(antecedent_name: @artist.name, consequent_name: "banned_artist")) - end - - should "get the index page" do - get artists_path - assert_response :success - end - - context "when searching the index page" do - should "find artists by name" do - get artists_path(name: "masao", format: "json") - assert_artist_found("masao") - end - - should "find artists by image URL" do - get artists_path(search: { url_matches: "http://i2.pixiv.net/img04/img/syounen_no_uta/46170939_m.jpg" }, format: "json") - assert_artist_found("masao") - end - - should "find artists by page URL" do - url = "http://www.pixiv.net/member_illust.php?mode=medium&illust_id=46170939" - get artists_path(search: { url_matches: url }, format: "json") - assert_artist_found("masao") + should "get the show page for a negated tag" do + @artist.update(name: "-aaa") + get artist_path(@artist.id) + assert_response :success end end - should "create an artist" do - attributes = FactoryBot.attributes_for(:artist) - assert_difference("Artist.count", 1) do - attributes.delete(:is_deleted) - post_auth artists_path, @user, params: {artist: attributes} + context "new action" do + should "render" do + get_auth new_artist_path, @user + assert_response :success + end + end + + context "show_or_new action" do + should "get the show_or_new page for an existing artist" do + get_auth show_or_new_artists_path(name: "masao"), @user + assert_redirected_to(@masao) + end + + should "get the show_or_new page for a nonexisting artist" do + get_auth show_or_new_artists_path(name: "nobody"), @user + assert_response :success + end + end + + context "edit action" do + should "render" do + get_auth edit_artist_path(@artist.id), @user + assert_response :success + end + end + + context "banned action" do + should "redirect to a banned search" do + get banned_artists_path + assert_response :redirect + end + end + + context "ban action" do + should "ban an artist" do + put_auth ban_artist_path(@artist.id), @admin + assert_redirected_to(@artist) + assert_equal(true, @artist.reload.is_banned?) + assert_equal(true, TagImplication.exists?(antecedent_name: @artist.name, consequent_name: "banned_artist")) + end + + should "not allow non-admins to ban artists" do + put_auth ban_artist_path(@artist.id), @user + assert_response 403 + assert_equal(false, @artist.reload.is_banned?) + end + end + + context "unban action" do + should "unban an artist" do + @artist.ban!(banner: @admin) + put_auth unban_artist_path(@artist.id), @admin + + assert_redirected_to(@artist) + assert_equal(false, @artist.reload.is_banned?) + assert_equal(false, TagImplication.exists?(antecedent_name: @artist.name, consequent_name: "banned_artist")) + end + end + + context "index action" do + should "get the index page" do + get artists_path + assert_response :success + end + + context "when searching the index page" do + should "find artists by name" do + get artists_path(name: "masao", format: "json") + assert_artist_found("masao") + end + + should "find artists by image URL" do + get artists_path(search: { url_matches: "http://i2.pixiv.net/img04/img/syounen_no_uta/46170939_m.jpg" }, format: "json") + assert_artist_found("masao") + end + + should "find artists by page URL" do + url = "http://www.pixiv.net/member_illust.php?mode=medium&illust_id=46170939" + get artists_path(search: { url_matches: url }, format: "json") + assert_artist_found("masao") + end + end + end + + context "create action" do + should "create an artist" do + assert_difference("Artist.count", 1) do + post_auth artists_path, @user, params: { artist: { name: "test" }} + assert_response :redirect + assert_equal("test", Artist.last.name) + end end - artist = Artist.find_by_name(attributes[:name]) - assert_not_nil(artist) - assert_redirected_to(artist_path(artist.id)) end context "with an artist that has a wiki page" do @@ -166,22 +181,23 @@ class ArtistsControllerTest < ActionDispatch::IntegrationTest end end - should "delete an artist" do - @builder = create(:builder_user) - delete_auth artist_path(@artist.id), @builder - assert_redirected_to(artist_path(@artist.id)) - @artist.reload - assert_equal(true, @artist.is_deleted) + context "destroy action" do + should "delete an artist" do + delete_auth artist_path(@artist.id), create(:builder_user) + assert_redirected_to(artist_path(@artist.id)) + assert_equal(true, @artist.reload.is_deleted) + end end - should "undelete an artist" do - @builder = create(:builder_user) - put_auth artist_path(@artist.id), @builder, params: {artist: {is_deleted: false}} - assert_redirected_to(artist_path(@artist.id)) - assert_equal(false, @artist.reload.is_deleted) + context "update action" do + should "undelete an artist" do + put_auth artist_path(@artist.id), create(:builder_user), params: {artist: {is_deleted: false}} + assert_redirected_to(artist_path(@artist.id)) + assert_equal(false, @artist.reload.is_deleted) + end end - context "reverting an artist" do + context "revert action" do should "work" do as_user do @artist.update(name: "xyz") @@ -196,9 +212,8 @@ class ArtistsControllerTest < ActionDispatch::IntegrationTest @artist2 = create(:artist) end put_auth artist_path(@artist.id), @user, params: {version_id: @artist2.versions.first.id} - @artist.reload - assert_not_equal(@artist.name, @artist2.name) assert_redirected_to(artist_path(@artist.id)) + assert_not_equal(@artist.reload.name, @artist2.name) end end