Merge pull request #47 from alphagov/refactor-requests-controller-params

Refactor requests controller params

Author: young-rob

app/controllers/api/requests_controller.rb

@@ -1,11 +1,11 @@
 module Api
   class RequestsController < Api::BaseController
     wrap_parameters include: Request.attribute_names + [:recipients]
+    before_action :set_request_record, only: %i[update resend_emails]
 
     def create
-      if request_params[:recipients].blank?
-        return render json: { errors: ["At least one recipient email is required"] }, status: :bad_request
-      end
+      errors = validate_create_params
+      return render json: { errors: errors }, status: :bad_request if errors.any?
 
       fact_check_request = Request.new(request_params.except(:recipients))
 
@@ -26,30 +26,22 @@ def create
     end
 
     def update
-      request_record = Request.find_by(source_app: params[:source_app], source_id: params[:source_id])
-
-      if request_record.nil?
-        return render json: { errors: "Request with ID #{params[:source_id]} not found for app #{params[:source_app]}" }, status: :bad_request
+      if params.dig(:request, :current_content).present? && update_params[:current_content].blank?
+        return render json: { errors: ["current_content must be a hash"] }, status: :bad_request
       end
 
-      if request_record.update(update_params)
-        render json: { id: request_record.id, source_id: request_record.source_id, source_app: request_record.source_app }, status: :ok
+      if @request_record.update(update_params)
+        render json: { id: @request_record.id, source_id: @request_record.source_id, source_app: @request_record.source_app }, status: :ok
       else
-        render json: { errors: request_record.errors.full_messages }, status: :unprocessable_entity
+        render json: { errors: @request_record.errors.full_messages }, status: :unprocessable_entity
       end
     end
 
     def resend_emails
-      request_record = Request.find_by(source_app: params[:source_app], source_id: params[:source_id])
-
-      unless request_record
-        return render json: { errors: "Request with ID #{params[:source_id]} not found for app #{params[:source_app]}" }, status: :bad_request
-      end
-
-      if NotifyService.resend_emails(request_record)
-        render json: { id: request_record.id, source_id: request_record.source_id, source_app: request_record.source_app }, status: :ok
+      if NotifyService.resend_emails(@request_record)
+        render json: { id: @request_record.id, source_id: @request_record.source_id, source_app: @request_record.source_app }, status: :ok
       else
-        render json: { errors: request_record.errors.full_messages }, status: :unprocessable_entity
+        render json: { errors: @request_record.errors.full_messages }, status: :unprocessable_entity
       end
     end
 
@@ -77,5 +69,27 @@ def update_params
         current_content: {},
       )
     end
+
+    def validate_create_params
+      errors = []
+
+      errors << "At least one recipient email is required" if request_params[:recipients].blank?
+
+      %i[current_content previous_content].each do |content_hash|
+        if params.dig(:request, content_hash).present? && request_params[content_hash].blank?
+          errors << "#{content_hash} must be a hash"
+        end
+      end
+
+      errors
+    end
+
+    def set_request_record
+      @request_record = Request.find_by(source_app: params[:source_app], source_id: params[:source_id])
+
+      unless @request_record
+        render json: { errors: ["Request with ID #{params[:source_id]} not found for app #{params[:source_app]}"] }, status: :not_found
+      end
+    end
   end
 end

app/models/request.rb

@@ -10,10 +10,12 @@ class Request < ApplicationRecord
   scope :most_recent_first, -> { order(created_at: :desc) }
   scope :most_recent_for_source, ->(source_id) { for_source(source_id).most_recent_first.first }
 
+private
+
   def content_data_must_be_string_pairs
     %i[current_content previous_content].each do |content_field|
       content_hash = public_send(content_field)
-      next if content_hash.blank?
+      next if content_hash.nil?
 
       unless content_hash.is_a?(Hash)
         errors.add(content_field, "#{content_field} is not a hash")

spec/requests/api/requests_spec.rb

@@ -118,6 +118,28 @@
         end
       end
 
+      context "if current_content is not a hash" do
+        let(:dynamic_current_content) { "not a hash" }
+
+        it "returns an error" do
+          post "/api/requests", params: base_payload, as: :json
+
+          expect(response).to have_http_status(:bad_request)
+          json = JSON.parse(response.body)
+          expect(json["errors"]).to include("current_content must be a hash")
+        end
+      end
+
+      context "if previous_content is not a hash" do
+        it "returns an error" do
+          post "/api/requests", params: valid_payload.merge(previous_content: "not a hash"), as: :json
+
+          expect(response).to have_http_status(:bad_request)
+          json = JSON.parse(response.body)
+          expect(json["errors"]).to include("previous_content must be a hash")
+        end
+      end
+
       context "without recipients" do
         it "returns a 400 error" do
           payload = valid_payload
@@ -158,7 +180,7 @@
         it "returns a 400 error" do
           post "/api/requests/#{invalid_source_app}/#{invalid_source_id}/resend-emails", as: :json
 
-          expect(response).to have_http_status(:bad_request)
+          expect(response).to have_http_status(:not_found)
           json = JSON.parse(response.body)
           expect(json["errors"]).to include(
             "Request with ID #{invalid_source_id} not found for app #{invalid_source_app}",
@@ -170,7 +192,7 @@
         it "returns a 400 error" do
           post "/api/requests/#{existing_request.source_app}/#{invalid_source_id}/resend-emails", as: :json
 
-          expect(response).to have_http_status(:bad_request)
+          expect(response).to have_http_status(:not_found)
           json = JSON.parse(response.body)
           expect(json["errors"]).to include(
             "Request with ID #{invalid_source_id} not found for app #{existing_request.source_app}",
@@ -182,7 +204,7 @@
         it "returns a 400 error" do
           post "/api/requests/#{invalid_source_app}/#{existing_request.source_id}/resend-emails", as: :json
 
-          expect(response).to have_http_status(:bad_request)
+          expect(response).to have_http_status(:not_found)
           json = JSON.parse(response.body)
           expect(json["errors"]).to include(
             "Request with ID #{existing_request.source_id} not found for app #{invalid_source_app}",
@@ -196,7 +218,7 @@
         it "returns a 400 error" do
           post "/api/requests/#{existing_request_whitehall.source_app}/#{existing_request.source_id}/resend-emails", as: :json
 
-          expect(response).to have_http_status(:bad_request)
+          expect(response).to have_http_status(:not_found)
           json = JSON.parse(response.body)
           expect(json["errors"]).to include(
             "Request with ID #{existing_request.source_id} not found for app #{existing_request_whitehall.source_app}",
@@ -253,7 +275,7 @@
         it "returns a 400 error" do
           patch "/api/requests/#{invalid_source_app}/#{invalid_source_id}", params: update_payload, as: :json
 
-          expect(response).to have_http_status(:bad_request)
+          expect(response).to have_http_status(:not_found)
           json = JSON.parse(response.body)
           expect(json["errors"]).to include(
             "Request with ID #{invalid_source_id} not found for app #{invalid_source_app}",
@@ -265,7 +287,7 @@
         it "returns a 400 error" do
           patch "/api/requests/#{update_payload[:source_app]}/#{invalid_source_id}", params: update_payload, as: :json
 
-          expect(response).to have_http_status(:bad_request)
+          expect(response).to have_http_status(:not_found)
           json = JSON.parse(response.body)
           expect(json["errors"]).to include(
             "Request with ID #{invalid_source_id} not found for app #{update_payload[:source_app]}",
@@ -277,7 +299,7 @@
         it "returns a 400 error" do
           patch "/api/requests/#{invalid_source_app}/#{update_payload[:source_id]}", params: update_payload, as: :json
 
-          expect(response).to have_http_status(:bad_request)
+          expect(response).to have_http_status(:not_found)
           json = JSON.parse(response.body)
           expect(json["errors"]).to include(
             "Request with ID #{update_payload[:source_id]} not found for app #{invalid_source_app}",
@@ -290,7 +312,7 @@
 
         it "returns a 400 error" do
           patch "/api/requests/#{second_request[:source_app]}/#{update_payload[:source_id]}", params: update_payload, as: :json
-          expect(response).to have_http_status(:bad_request)
+          expect(response).to have_http_status(:not_found)
           json = JSON.parse(response.body)
           expect(json["errors"]).to include(
             "Request with ID #{update_payload[:source_id]} not found for app #{second_request[:source_app]}",
@@ -313,5 +335,20 @@
         )
       end
     end
+
+    context "if current_content is not a hash" do
+      let(:invalid_content_payload) { { source_app: existing_request.source_app, source_id: existing_request.source_id, current_content: "not a hash" } }
+
+      it "returns errors for current_content format" do
+        patch "/api/requests/#{existing_request.source_app}/#{existing_request.source_id}", params: invalid_content_payload, as: :json
+
+        expect(response).to have_http_status(:bad_request)
+
+        json = JSON.parse(response.body)
+        expect(json["errors"]).to include(
+          "current_content must be a hash",
+        )
+      end
+    end
   end
 end