Merge pull request #34 from alphagov/dynamic-json-hash-content

Fact Check: Accept and validate JSON hashes for content fields

Author: AlexRyanShep

app/controllers/api/requests_controller.rb

@@ -21,7 +21,7 @@ def create
       if fact_check_request.save
         render json: { id: fact_check_request.id, source_id: fact_check_request.source_id }, status: :created
       else
-        render json: { errors: fact_check_request.errors.full_messages }, status: :bad_request
+        render json: { errors: fact_check_request.errors.full_messages }, status: :unprocessable_entity
       end
     end
 
@@ -37,7 +37,7 @@ def request_params
         :requester_email,
         :current_content,
         :previous_content, # optional
-        :deadline, # optional
+        :deadline,
         recipients: [],
         # dynamic hash fields at the end
         current_content: {},

app/models/request.rb

@@ -3,8 +3,28 @@ class Request < ApplicationRecord
   has_many :users, through: :collaborations
   has_one :response
 
-  validates :source_id, :source_app, :requester_name, :requester_email, :status, :current_content, presence: true
+  validates :source_id, :source_app, :requester_name, :requester_email, :status, :current_content, :deadline, presence: true
+  validate :content_data_must_be_string_pairs
+
   scope :for_source, ->(source_id) { where(source_id: source_id) }
   scope :most_recent_first, -> { order(created_at: :desc) }
   scope :most_recent_for_source, ->(source_id) { for_source(source_id).most_recent_first.first }
+
+  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?
+
+      unless content_hash.is_a?(Hash)
+        errors.add(content_field, "#{content_field} is not a hash")
+        next
+      end
+
+      content_hash.each do |key, value|
+        unless value.is_a?(String)
+          errors.add(content_field, "value for #{key} must be a string")
+        end
+      end
+    end
+  end
 end

db/migrate/20260220094700_add_json_current_content_column_to_requests.rb

@@ -0,0 +1,18 @@
+class AddJsonCurrentContentColumnToRequests < ActiveRecord::Migration[8.0]
+  def up
+    add_column :requests, :json_current_content, :json
+    Request.reset_column_information
+
+    # Convert old data and move it to this column
+    Request.find_each do |request|
+      if request.current_content.present?
+        json_payload = { body: request.current_content }
+        request.update_columns(json_current_content: json_payload)
+      end
+    end
+  end
+
+  def down
+    remove_column :requests, :json_current_content
+  end
+end

db/migrate/20260220120922_add_json_previous_content_column_to_requests.rb

@@ -0,0 +1,18 @@
+class AddJsonPreviousContentColumnToRequests < ActiveRecord::Migration[8.0]
+  def up
+    add_column :requests, :json_previous_content, :json
+    Request.reset_column_information
+
+    # Convert old data and move it to this column
+    Request.find_each do |request|
+      if request.previous_content.present?
+        json_payload = { body: request.previous_content }
+        request.update_columns(json_previous_content: json_payload)
+      end
+    end
+  end
+
+  def down
+    remove_column :requests, :json_previous_content
+  end
+end

db/migrate/20260224092134_cleanup_requests_content_columns.rb

@@ -0,0 +1,11 @@
+class CleanupRequestsContentColumns < ActiveRecord::Migration[8.0]
+  change_table(:requests, bulk: true) do |t|
+    # Remove old text columns, now that data has been converted to json
+    t.remove :current_content, type: :text
+    t.remove :previous_content, type: :text
+
+    # Rename new columns to replace removed column names
+    t.rename :json_current_content, :current_content
+    t.rename :json_previous_content, :previous_content
+  end
+end

db/schema.rb

@@ -9,8 +9,8 @@
 # migrations use external dependencies or application code.
 #
 # It's strongly recommended that you check this file into your version control system.
-
 ActiveRecord::Schema[8.0].define(version: 2026_02_26_095141) do
+
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_catalog.plpgsql"
   enable_extension "pgcrypto"
@@ -31,14 +31,14 @@
     t.string "requester_name", null: false
     t.string "requester_email", null: false
     t.string "status", default: "new", null: false
-    t.json "previous_content"
-    t.json "current_content", null: false
-    t.datetime "deadline"
+    t.datetime "deadline", null: false
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
     t.string "source_app", null: false
     t.string "source_url"
     t.string "source_title"
+    t.json "current_content", null: false
+    t.json "previous_content"
     t.index ["created_at"], name: "index_requests_on_created_at"
     t.index ["source_id"], name: "index_requests_on_source_id"
   end

spec/factories/request.rb

@@ -4,7 +4,21 @@
     source_app { "publisher" }
     requester_name { "Malcolm Tucker" }
     requester_email { "m.tucker@gov.uk" }
-    current_content { "<HTML> Changes go here <HTML>" }
+    current_content do
+      {
+        "body":
+          "Many lines of data for the content. Many changes that need fact checking",
+      }
+    end
     deadline { Time.zone.now + 1.week }
+
+    trait :with_more_complex_content_data do
+      multi_part_previous_content = { heading: "How to claim for intergalactic travel expenses",
+                                      body: "If you or your partner is travelling abroad for more than 7 months, you may be able to claim for expenses." }
+      multi_part_current_content = { heading: "How to claim for Inter-galactic Travel Expenses",
+                                     body: "If you or your partner are travelling abroad for more than 8 months, you may be able to claim for expenses." }
+      previous_content { multi_part_previous_content }
+      current_content { multi_part_current_content }
+    end
   end
 end

spec/models/request_spec.rb

@@ -1,23 +1,68 @@
 require "rails_helper"
 
+RSpec.shared_examples "test JSON content" do |content_field|
+  context "when #{content_field} contains non string values" do
+    it "is invalid" do
+      invalid_hash_content = { "illegal_boolean": false }
+      record = FactoryBot.build(:request, **{ content_field => invalid_hash_content })
+
+      expect(record).not_to be_valid
+      expect(record.errors.messages[content_field]).to include("value for illegal_boolean must be a string")
+    end
+
+    it "adds an error to #{content_field}" do
+      invalid_hash_content = "[\"apple\", \"banana\", \"kiwi\"]"
+      record = FactoryBot.build(:request, **{ content_field => invalid_hash_content })
+
+      expect(record).not_to be_valid
+      expect(record.errors.messages[content_field]).to include("#{content_field} is not a hash")
+    end
+  end
+end
+
 RSpec.describe Request, type: :model do
-  it "is not valid without required attributes" do
-    record = described_class.new
+  context "when missing required attributes" do
+    it "is invalid" do
+      record = described_class.new
+
+      expect(record).not_to be_valid
+    end
 
-    expect(record).not_to be_valid
+    it "includes errors for each missing required attribute" do
+      record = described_class.new
+
+      expect(record).not_to be_valid
+      expect(record.errors.attribute_names).to include(:source_id, :source_app, :requester_name, :requester_email, :current_content)
+    end
   end
 
-  it "includes errors for each missing mandatory attribute" do
-    record = described_class.new
-    record.valid?
+  context "when current_content is an empty hash" do
+    it "raises an error " do
+      record = FactoryBot.build(:request, current_content: {})
 
-    expect(record.errors.attribute_names).to include(:source_id, :source_app, :requester_name, :requester_email, :current_content)
+      expect(record).not_to be_valid
+      expect(record.errors.attribute_names).to include(:current_content)
+      expect(record.errors.messages[:current_content]).to include("can't be blank")
+    end
   end
 
-  it "is valid when all required attributes are set" do
-    record = FactoryBot.build(:request)
+  include_examples "test JSON content", :current_content
+  include_examples "test JSON content", :previous_content
 
-    expect(record).to be_valid
+  context "when all required attributes are set" do
+    it "is valid" do
+      record = FactoryBot.build(:request)
+
+      expect(record).to be_valid
+    end
+  end
+
+  context "when content hashes contain multiple key-value-pairs" do
+    it "is valid" do
+      record = FactoryBot.build(:request, :with_more_complex_content_data)
+
+      expect(record).to be_valid
+    end
   end
 
   describe "searching by source_id" do

spec/requests/api/requests_spec.rb

@@ -17,8 +17,11 @@
       source_title: "",
       requester_name: "GDS Content Designer",
       requester_email: "gds-content-designer@example.com",
-      current_content: "<p>Test HTML</p>",
-      previous_content: "",
+      current_content: {
+        "heading": "Some title words",
+        "body": "Many lines of data for the content. Many changes that need fact checking",
+      },
+      previous_content: {},
       deadline: 1.week.from_now.iso8601,
       recipients: ["recipient1@example.com", "recipient2@example.com"],
     }
@@ -29,7 +32,7 @@
       expect {
         post "/api/requests", params: valid_payload, as: :json
       }.to change(Request, :count).by(1)
-       .and change(Collaboration, :count).by(2)
+                                  .and change(Collaboration, :count).by(2)
 
       expect(response).to have_http_status(:created)
 
@@ -39,38 +42,92 @@
       request = Request.last
       expect(request.source_app).to eq("Mainstream")
       expect(request.source_id).to be_present
-      expect(request.current_content).to eq("<p>Test HTML</p>")
+      expect(request.current_content["body"]).to eq("Many lines of data for the content. Many changes that need fact checking")
       expect(request.status).to eq("new")
       expect(request.requester_name).to eq("GDS Content Designer")
       expect(request.requester_email).to eq("gds-content-designer@example.com")
     end
   end
 
-  context "with invalid payload" do
+  context "with an invalid payload" do
+    let(:base_payload) do
+      {
+        source_app: "Mainstream",
+        source_id: SecureRandom.uuid,
+        requester_name: "GDS Content Designer",
+        requester_email: "gds-content-designer@example.com",
+        current_content: dynamic_current_content,
+        previous_content: {},
+        deadline: 1.week.from_now.iso8601,
+        recipients: ["recipient1@example.com", "recipient2@example.com"],
+      }
+    end
+
     it "returns errors for missing required fields" do
-      invalid_payload = { requester_name: "Alice", recipients: ["recipient1@example.com", "recipient2@example.com"] }
+      payload_missing_required_fields = { requester_name: "Alice",
+                                          recipients: ["recipient1@example.com", "recipient2@example.com"] }
 
-      post "/api/requests", params: invalid_payload, as: :json
+      expect {
+        post "/api/requests", params: payload_missing_required_fields, as: :json
+      }.to change(Request, :count).by(0)
+                                  .and change(Collaboration, :count).by(0)
 
-      expect(response).to have_http_status(:bad_request)
+      expect(response).to have_http_status(:unprocessable_content)
       json = JSON.parse(response.body)
       expect(json["errors"]).to include(
         "Source can't be blank",
+        "Source app can't be blank",
         "Requester email can't be blank",
+        "Current content can't be blank",
+        "Deadline can't be blank",
       )
     end
-  end
 
-  context "without recipients" do
-    it "returns a 400 error" do
-      payload = valid_payload
-      payload.delete(:recipients)
+    context "if current_content value is not a string" do
+      let(:dynamic_current_content) do
+        {
+          "normal_field" => "This should pass",
+          "bad_number_field" => 123,
+        }
+      end
+
+      it "returns an error" do
+        post "/api/requests", params: base_payload, as: :json
+
+        expect(response).to have_http_status(:unprocessable_content)
+        json = JSON.parse(response.body)
+        expect(json["errors"]).to include("Current content value for bad_number_field must be a string")
+      end
+    end
+
+    context "if current_content contains nested data" do
+      let(:dynamic_current_content) do
+        {
+          "normal_field" => "This should pass",
+          "sneaky_nested_hash" => { "naughty" => "This should fail" },
+        }
+      end
+
+      it "returns an error" do
+        expect {
+          post "/api/requests", params: base_payload, as: :json
+        }.to change(Request, :count).by(0)
+                                    .and change(Collaboration, :count).by(0)
+        expect(response).to have_http_status(:unprocessable_content)
+      end
+    end
+
+    context "without recipients" do
+      it "returns a 400 error" do
+        payload = valid_payload
+        payload.delete(:recipients)
 
-      post "/api/requests", params: payload, as: :json
+        post "/api/requests", params: payload, as: :json
 
-      expect(response).to have_http_status(:bad_request)
-      expect(JSON.parse(response.body)["errors"])
-        .to include("At least one recipient email is required")
+        expect(response).to have_http_status(:bad_request)
+        expect(JSON.parse(response.body)["errors"])
+          .to include("At least one recipient email is required")
+      end
     end
   end
 end