From 882df38a7b2e92a3bbda7ea41ae0f10fd1f9ff11 Mon Sep 17 00:00:00 2001 From: Mysti Sadler Date: Mon, 11 Jun 2018 11:30:30 -0600 Subject: [PATCH] Do not create ErrorReport on files api 404 closes ADMIN-1130 Test plan - Run /api/v1/courses/:course_id/files/(id that doesn't exist, or is deleted) - Ensure you don't get a new error report for it Change-Id: I9df392d22ca0ecfae18a5f2576e99d907b9a77a3 Reviewed-on: https://gerrit.instructure.com/153301 Tested-by: Jenkins Reviewed-by: Ed Schiebel QA-Review: Luke Kingsley Product-Review: Mysti Sadler --- app/controllers/files_controller.rb | 7 +++++-- spec/apis/v1/files_controller_api_spec.rb | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/controllers/files_controller.rb b/app/controllers/files_controller.rb index d970cf5ca31..b8e68feeea5 100644 --- a/app/controllers/files_controller.rb +++ b/app/controllers/files_controller.rb @@ -444,8 +444,11 @@ class FilesController < ApplicationController # @returns File def api_show get_context - @attachment = @context ? @context.attachments.find(params[:id]) : Attachment.find(params[:id]) - raise ActiveRecord::RecordNotFound if @attachment.deleted? + @attachment = @context ? @context.attachments.not_deleted.find_by(id: params[:id]) : Attachment.not_deleted.find_by(id: params[:id]) + unless @attachment + render json: { errors: [{message: "The specified resource does not exist."}] }, status: 404 + return + end params[:include] = Array(params[:include]) if authorized_action(@attachment,@current_user,:read) render :json => attachment_json(@attachment, @current_user, {}, { include: params[:include], omit_verifier_in_app: !value_to_boolean(params[:use_verifiers]) }) diff --git a/spec/apis/v1/files_controller_api_spec.rb b/spec/apis/v1/files_controller_api_spec.rb index 9eb3640ffd4..bf4cd5416c8 100644 --- a/spec/apis/v1/files_controller_api_spec.rb +++ b/spec/apis/v1/files_controller_api_spec.rb @@ -927,7 +927,7 @@ describe "Files API", type: :request do end it "should return not found error" do - api_call(:get, "/api/v1/files/0", @file_path_options.merge(:id => '0'), {}, {}, :expected_status => 404) + expect{api_call(:get, "/api/v1/files/0", @file_path_options.merge(id: '0'), {}, {}, expected_status: 404)}.not_to change { ErrorReport.count } end it "should return not found for deleted attachment" do