Respond with 404 Not Found if project not found in Api::SchoolProjectsController actions

[floehopper]

Previously when a request was made to either of the actions in Api::SchoolProjectsController for a project that does not exist, the following exception was raised, because the call to Project.find_by returned nil:

NoMethodError: undefined method `school_project' for nil:NilClass

Unhandled exceptions are rescued by the default ActionDispatch::ShowExceptions middleware. In turn this uses the default exceptions app, ActionDispatch::PublicExceptions to render a static HTML error page from public/500.html in this case.

Since this is an API endpoint, ideally it should always respond with JSON in the response body, otherwise it makes writing client code a lot harder. Indeed my actual motivation for fixing this is, because we've been seeing JSON:ParseError exceptions raised in experience-cs.

I've fixed the problem by using Project.find_by! instead of Project.find_by. This means that if the project is not found, a ActiveRecord::RecordNotFound exception is raised. This in turn is rescued by the rescue_from block in ApiController and the action responds with a 404 Not Found head response, i.e. with no body.

This head response seems to be handled OK by the HTTParty code in experience-cs and thus it fixes the problem I was trying to address.

The equivalent client-side code in editor-standalone was already throwing an AxiosError exception due to the 500 response code (not being a 2XX code) and it will continue to throw the same exception, albeit with a payload reflecting the response code being 404 instead of 500 and the body being empty instead of being the Rails 500 HTML error page.

There is no specific error-handling code in editor-standalone catching this exception, so I assume it's just relying on the generic error handling for the React app. Thus I think the user-facing behaviour should be unchanged.

Addresses https://github.com/RaspberryPiFoundation/experience-cs/issues/828.

Postscript

While investigating this issue, I've uncovered a number of other problems with the Api::SchoolProjectsController code and its specs, but I've intentionally not addressed them here in order to keep the scope of this PR as small as possible and keep it focussed on fixing the problem at hand. However, in case it's useful, I'll list the issues here:

• The call to load_and_authorize_resource :project is not working as intended:
  • it's not even attempting to load the Project instance (even if it was, it would be attempting to call Project.find(params[:id) and not Project.find_by!(identifier: params[:id])
  • it's not authorizing the user can take the action against an instance of a Project (it seems to be authorizing the user can show the Project class, but it seems unlikely this is what was intended)
  • I can't tell from the git history why this call to load_and_authorize_resource was added and no tests fail if I remove it!
  • This issue is closely related to Fix authorization in Api::ProjectsController #553.
• Given that Api::ProjectsController uses a custom ProjectLoader class which takes locale into account, I would've thought that this Api::SchoolProjectsController should also use the same lookup mechanism, but it doesn't.
• There are no examples testing the presence of the before_action :authorize_user call.
• The call to ProfileApiMock#stub_profile_api_list_school_students in spec/requests/school_projects/show_finished_spec.rb & spec/requests/school_projects/set_finished_spec.rb seem unnecessary and are potentially confusing. This helper method stubs ProfileApiClient.list_school_students, but this method is never called by code exercised by those specs. This seems to be a wider problem where calls to ProfileApiMock#stub_profile_api_list_school_students have been added to specs that don't need it. Perhaps they never needed it or the need for them has gone away.
• This isn't specific to this controller, but I think it's not great that any unhandled exceptions in API controllers result in an HTML response. Exceptions that occur in the "operation" classes in lib/concepts/*/operations/ are usually rescued and the action responds with a JSON body. However, for exceptions that occur outside those operations (e.g. in the actions themselves) just result in the 500 error with HTML body.

[sebjacobs]

❤️ Thanks @floehopper. It's great that you've been able to wrap some simple specs around this bug. I'm also really impressed how you've managed to avoid scope creep here.

[floehopper]

Adrian has given me the go-ahead to merge this.