pundit: convert artists to pundit.

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

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

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 @@
     <li>|</li>
     <%= 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 policy(@artist).unban? %>
-      <% 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?"} %>
-        <%= 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? %>
-      <% else %>
+      <%= subnav_link_to "Ban", ban_artist_path(@artist), :method => :put, :data => {:confirm => "Are you sure you want to ban this artist?"} %>
-        <%= subnav_link_to "Ban", ban_artist_path(@artist), :method => :put, :data => {:confirm => "Are you sure you want to ban this artist?"} %>
-      <% end %>
     <% end %>
   <% end %>
 <% end %>

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? %>

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
+      should "get the show page for a negated tag" do
-      get_auth new_artist_path, @user
+        @artist.update(name: "-aaa")
-      assert_response :success
+        get artist_path(@artist.id)
-    end
+        assert_response :success
-
-    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")
       end
     end
 
-    should "create an artist" do
+    context "new action" do
-      attributes = FactoryBot.attributes_for(:artist)
+      should "render" do
-      assert_difference("Artist.count", 1) do
+        get_auth new_artist_path, @user
-        attributes.delete(:is_deleted)
+        assert_response :success
-        post_auth artists_path, @user, params: {artist: attributes}
+      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
+    context "destroy action" do
-      @builder = create(:builder_user)
+      should "delete an artist" do
-      delete_auth artist_path(@artist.id), @builder
+        delete_auth artist_path(@artist.id), create(:builder_user)
-      assert_redirected_to(artist_path(@artist.id))
+        assert_redirected_to(artist_path(@artist.id))
-      @artist.reload
+        assert_equal(true, @artist.reload.is_deleted)
-      assert_equal(true, @artist.is_deleted)
+      end
     end
 
-    should "undelete an artist" do
+    context "update action" do
-      @builder = create(:builder_user)
+      should "undelete an artist" do
-      put_auth artist_path(@artist.id), @builder, params: {artist: {is_deleted: false}}
+        put_auth artist_path(@artist.id), create(:builder_user), params: {artist: {is_deleted: false}}
-      assert_redirected_to(artist_path(@artist.id))
+        assert_redirected_to(artist_path(@artist.id))
-      assert_equal(false, @artist.reload.is_deleted)
+        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