From c22d5c21ca585c8f5fe25f277e598a393cd64cbe Mon Sep 17 00:00:00 2001 From: Kelly Croswell Date: Tue, 13 Sep 2016 10:09:59 -0400 Subject: [PATCH] changed how not found objects get handled with pundit --- .../work_item_states_controller.rb | 9 +-- app/controllers/work_items_controller.rb | 55 ++++++++++--------- app/policies/institution_policy.rb | 2 +- app/policies/work_item_policy.rb | 10 +--- config/routes.rb | 1 + .../work_item_states_controller_spec.rb | 7 ++- .../controllers/work_items_controller_spec.rb | 12 ++-- 7 files changed, 51 insertions(+), 45 deletions(-) diff --git a/app/controllers/work_item_states_controller.rb b/app/controllers/work_item_states_controller.rb index 35cbdf7f..bb981c42 100644 --- a/app/controllers/work_item_states_controller.rb +++ b/app/controllers/work_item_states_controller.rb @@ -30,10 +30,11 @@ def update def show if @state_item.nil? render nothing: true, status: :not_found and return - end - authorize @state_item - respond_to do |format| - format.json { render json: @state_item.serializable_hash } + else + authorize @state_item + respond_to do |format| + format.json { render json: @state_item.serializable_hash } + end end end diff --git a/app/controllers/work_items_controller.rb b/app/controllers/work_items_controller.rb index 02d8bc09..03f06d2e 100644 --- a/app/controllers/work_items_controller.rb +++ b/app/controllers/work_items_controller.rb @@ -10,12 +10,17 @@ def index filter unless request.format == :json sort page_results(@items) - respond_to do |format| - format.json { - json_list = @paged_results.map { |item| item.serializable_hash(except: [:node, :pid]) } - render json: {count: @count, next: @next, previous: @previous, results: json_list} - } - format.html + if @items.nil? || @items.empty? + render nothing: true, status: :not_found and return + else + authorize @items + respond_to do |format| + format.json { + json_list = @paged_results.map { |item| item.serializable_hash(except: [:node, :pid]) } + render json: {count: @count, next: @next, previous: @previous, results: json_list} + } + format.html + end end end @@ -31,15 +36,19 @@ def create end def show - authorize @work_item - respond_to do |format| - if current_user.admin? - item = @work_item.serializable_hash - else - item = @work_item.serializable_hash(except: [:node, :pid]) + if @work_item + authorize @work_item + respond_to do |format| + if current_user.admin? + item = @work_item.serializable_hash + else + item = @work_item.serializable_hash(except: [:node, :pid]) + end + format.json { render json: item } + format.html end - format.json { render json: item } - format.html + else + render nothing: true, status: :not_found and return end end @@ -111,7 +120,11 @@ def set_restoration_status restore = Pharos::Application::PHAROS_ACTIONS['restore'] @item = WorkItem.where(object_identifier: params[:object_identifier], action: restore).order(created_at: :desc).first - authorize (@item || WorkItem.new), :set_restoration_status? + if @item.nil? + render nothing: true, status: :not_found and return + else + authorize @item + end if @item succeeded = @item.update_attributes(params_for_status_update) end @@ -277,7 +290,6 @@ def set_items end params[:institution].present? ? @institution = Institution.find(params[:institution]) : @institution = current_user.institution params[:sort] = 'date' if params[:sort].nil? - authorize @items @items = @items .created_before(params[:created_before]) .created_after(params[:created_after]) @@ -320,17 +332,6 @@ def set_item bag_date: params[:bag_date]).first end end - if @work_item - params[:id] = @work_item.id - else - # API callers **DEPEND** on getting a 404 if the record does - # not exist. This is how they know that an item has not started - # the ingestion process. So if @work_item is nil, return - # 404 now. Otherwise, the call to authorize below will result - # in a 500 error from pundit. - raise ActiveRecord::RecordNotFound - end - authorize @work_item, :show? end def rewrite_params_for_sqlite diff --git a/app/policies/institution_policy.rb b/app/policies/institution_policy.rb index b0402a1e..c87ee2c9 100644 --- a/app/policies/institution_policy.rb +++ b/app/policies/institution_policy.rb @@ -22,7 +22,7 @@ def index_through_institution? end def show? - record.nil? || user.admin? || (user.institution_id == record.id) + user.admin? || (user.institution_id == record.id) end def edit? diff --git a/app/policies/work_item_policy.rb b/app/policies/work_item_policy.rb index 8b61a721..099ad560 100644 --- a/app/policies/work_item_policy.rb +++ b/app/policies/work_item_policy.rb @@ -9,11 +9,7 @@ def new? end def index? - record.first.nil? || user.admin? || (user.institution.id == record.first.institution_id) - end - - def search? - record.first.nil? || user.admin? || (user.institution.id == record.first.institution_id) + user.admin? || (user.institution.id == record.first.institution_id) end def admin_api? @@ -25,7 +21,7 @@ def admin_show? end def show? - record.nil? || user.admin? || (user.institution.id == record.institution_id) + user.admin? || (user.institution.id == record.institution_id) end def update? @@ -42,7 +38,7 @@ def destroy? end def set_restoration_status? - record.nil? || user.admin? + user.admin? end def items_for_delete? diff --git a/config/routes.rb b/config/routes.rb index beccaafb..e7e51ba8 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -55,6 +55,7 @@ post '/api/v2/item_state', to: 'work_item_states#create', format: :json put '/api/v2/item_state/:work_item_id', to: 'work_item_states#update', format: :json get '/api/v2/item_state/:work_item_id', to: 'work_item_states#show', format: :json + get '/api/v2/item_state/:id', to: 'work_item_states#show', format: :json # CATALOG ROUTES post 'search/', to: 'catalog#search', format: [:json, :html], as: :search diff --git a/spec/controllers/work_item_states_controller_spec.rb b/spec/controllers/work_item_states_controller_spec.rb index 74c6704e..09c5dbe6 100644 --- a/spec/controllers/work_item_states_controller_spec.rb +++ b/spec/controllers/work_item_states_controller_spec.rb @@ -6,6 +6,7 @@ let!(:object) { FactoryGirl.create(:intellectual_object, institution: institution, access: 'institution') } let!(:item) { FactoryGirl.create(:work_item, institution: institution, intellectual_object: object, object_identifier: object.identifier, action: Pharos::Application::PHAROS_ACTIONS['fixity'], status: Pharos::Application::PHAROS_STATUSES['success']) } let!(:other_item) { FactoryGirl.create(:work_item, institution: institution, intellectual_object: object, object_identifier: object.identifier) } + let!(:lonely_item) { FactoryGirl.create(:work_item, institution: institution, intellectual_object: object, object_identifier: object.identifier) } let!(:state_item) { FactoryGirl.create(:work_item_state, work_item: item, state: '{JSON data}') } describe 'POST #create' do @@ -17,7 +18,6 @@ post :create, work_item_state: { state: '{JSON data}', work_item_id: other_item.id, action: 'Success' }, format: :json expect(response).to be_success assigns(:state_item).action.should eq('Success') - # State '{JSON data}' should be compressed assigns(:state_item).state.bytes.should eq("x\x9C\xAB\xF6\n\xF6\xF7SHI,I\xAC\x05\x00\x16\x90\x03\xED".bytes) assigns(:state_item).unzipped_state.should eq('{JSON data}') assigns(:state_item).work_item_id.should eq(other_item.id) @@ -56,6 +56,11 @@ assigns(:state_item).state.bytes.should eq("x\x9C\xAB\xF6\n\xF6\xF7SHI,I\xAC\x05\x00\x16\x90\x03\xED".bytes) assigns(:state_item).unzipped_state.should eq('{JSON data}') end + + it 'responds with a 404 error if the state item does not exist' do + get :show, work_item_id: lonely_item.id, format: :json + expect(response.status).to eq(404) + end end end diff --git a/spec/controllers/work_items_controller_spec.rb b/spec/controllers/work_items_controller_spec.rb index 110c7195..616f44ab 100644 --- a/spec/controllers/work_items_controller_spec.rb +++ b/spec/controllers/work_items_controller_spec.rb @@ -37,6 +37,11 @@ get :index assigns(:institution).should eq( admin_user.institution) end + + it 'responds with a 404 when no results are found' do + get :index, name: 'name_of_something_not_found' + expect(response.status).to eq(404) + end end describe 'for institutional admin' do @@ -84,11 +89,8 @@ end it 'returns 404, not 500, for item not found' do - expect { - get :show, - etag: 'does not exist', - name: 'duznot igzist', - bag_date: '1901-01-01' }.to raise_error(ActiveRecord::RecordNotFound) + get :show, etag: 'does not exist', name: 'duznot igzist', bag_date: '1901-01-01' + expect(response.status).to eq(404) end end