[CPDLP-3094] Address PR comments

app/controllers/api/v1/declarations_controller.rb

@@ -33,7 +33,7 @@ def void
       def create
         service = Declarations::Create.new(declaration_params)
 
-        if service.save
+        if service.create_declaration
           render json: to_json(service.declaration)
         else
           render json: API::Errors::Response.from(service), status: :unprocessable_entity

app/controllers/api/v2/declarations_controller.rb

@@ -33,7 +33,7 @@ def void
       def create
         service = Declarations::Create.new(declaration_params)
 
-        if service.save
+        if service.create_declaration
           render json: to_json(service.declaration)
         else
           render json: API::Errors::Response.from(service), status: :unprocessable_entity

app/controllers/api/v3/declarations_controller.rb

@@ -25,7 +25,7 @@ def void
       def create
         service = Declarations::Create.new(declaration_params)
 
-        if service.save
+        if service.create_declaration
           render json: to_json(service.declaration)
         else
           render json: API::Errors::Response.from(service), status: :unprocessable_entity

app/models/application.rb

@@ -70,12 +70,6 @@ class Application < ApplicationRecord
     withdrawn: "withdrawn",
   }
 
-  def funding_eligibility(with_funded_place:)
-    return eligible_for_funding unless with_funded_place
-
-    eligible_for_funding && (funded_place.nil? || funded_place)
-  end
-
   # `eligible_for_dfe_funding?`  takes into consideration what we know
   # about user eligibility plus if it has been previously funded. We need
   # to keep this method in place to keep consistency during the split between
@@ -174,4 +168,12 @@ def self.cut_off_date_for_expired_applications
   def fundable?
     eligible_for_dfe_funding?(with_funded_place: true)
   end
+
+private
+
+  def funding_eligibility(with_funded_place:)
+    return eligible_for_funding unless with_funded_place
+
+    eligible_for_funding && (funded_place.nil? || funded_place)
+  end
 end

app/models/declaration.rb

@@ -8,6 +8,7 @@ class Declaration < ApplicationRecord
   belongs_to :superseded_by, class_name: "Declaration", optional: true
   has_many :participant_outcomes, dependent: :destroy
   has_many :statement_items
+  has_many :statements, through: :statement_items
 
   UPLIFT_PAID_STATES = %w[paid awaiting_clawback clawed_back].freeze
   COURSE_IDENTIFIERS_INELIGIBLE_FOR_UPLIFT = %w[npq-additional-support-offer npq-early-headship-coaching-offer].freeze
@@ -99,6 +100,6 @@ def duplicate_declarations
 private
 
   def declaration_date_not_in_the_future
-    errors.add(:declaration_date, I18n.t("declaration.future_declaration_date")) if declaration_date && declaration_date > Time.zone.today
+    errors.add(:declaration_date, :future_declaration_date) if declaration_date&.future?
   end
 end

app/services/declarations/create.rb

@@ -13,26 +13,28 @@ class Create
     attribute :has_passed
 
     validates :lead_provider, presence: true
-    validates :participant_id, presence: { message: I18n.t("declaration.missing_participant_id") }
+    validates :participant_id, presence: true
     validates :participant, participant_presence: true, participant_not_withdrawn: true
-    validates :course_identifier, inclusion: { in: Course::IDENTIFIERS, message: I18n.t(:invalid_course) }, allow_blank: false, course_for_participant: true
-    validates :declaration_date, declaration_date: true, allow_blank: true
-    validates :declaration_date, presence: { message: I18n.t("declaration.missing_declaration_date") }
-    validates :declaration_type, presence: { message: I18n.t("declaration.missing_declaration_type") }
-    validates :cohort, contract_for_cohort_and_course: { message: I18n.t("declaration.missing_npq_contract_for_cohort_and_course") }
+    validates :course_identifier, inclusion: { in: Course::IDENTIFIERS }, allow_blank: false, course_for_participant: true
+    validates :declaration_date, declaration_date: true
+    validates :declaration_date, presence: true
+    validates :declaration_type, presence: true
+    # TODO we don't have NPQ Contract yet
+    validates :cohort, contract_for_cohort_and_course: true
 
     validate :output_fee_statement_available
     validate :validate_has_passed_field, if: :validate_has_passed?
     validate :validate_schedule_exists
     validate :validates_billable_slot_available
 
-    attr_reader :raw_declaration_date
+    attr_reader :raw_declaration_date, :declaration
 
-    def save
+    def create_declaration
       return false unless valid?
 
       ApplicationRecord.transaction do
-        set_eligibility
+        find_or_create_declaration!
+        set_eligibility!
 
         statement_attacher.attach unless declaration.submitted_state?
       end
@@ -61,17 +63,39 @@ def participant
       end
     end
 
-    def declaration
-      @declaration ||= existing_declaration || Declaration.create!(declaration_parameters.merge(application:, cohort:))
-    end
-
   private
 
     attr_writer :raw_declaration_date
 
     delegate :schedule, to: :application, allow_nil: true
     delegate :cohort, to: :schedule
 
+    def declaration_parameters
+      {
+        declaration_date:,
+        declaration_type:,
+        lead_provider:,
+      }
+    end
+
+    def existing_declaration
+      @existing_declaration ||= participant
+        .declarations
+        .joins(application: :course)
+        .submitted_state
+        .or(
+          participant
+            .declarations
+            .joins(application: :course)
+            .billable,
+        )
+        .find_by(declaration_parameters.merge(application: { courses: { identifier: course_identifier } }))
+    end
+
+    def find_or_create_declaration!
+      @declaration ||= existing_declaration || Declaration.create!(declaration_parameters.merge(application:, cohort:))
+    end
+
     def statement_attacher
       @statement_attacher ||= StatementAttacher.new(declaration:)
     end
@@ -81,13 +105,14 @@ def output_fee_statement_available
       return if application.blank?
       return if existing_declaration&.submitted_state?
       return if existing_declaration.nil? && !application.fundable?
+      return unless lead_provider.next_output_fee_statement(cohort).blank?
 
-      errors.add(:cohort, I18n.t("statement.no_output_fee_statement", cohort: cohort.start_year)) if lead_provider.next_output_fee_statement(cohort).blank?
+      errors.add(:cohort, :no_output_fee_statement, cohort: cohort.start_year)
     end
 
-    def set_eligibility
+    def set_eligibility!
       if declaration.duplicate_declarations.any?
-        declaration.update!(superseded_by: original_participant_declaration)
+        declaration.update!(superseded_by: original_declaration)
         declaration.ineligible_state!
       elsif application.fundable?
         declaration.eligible_state!
@@ -96,30 +121,9 @@ def set_eligibility
 
     def validate_schedule_exists
       return if errors.any?
+      return if schedule&.allowed_declaration_types&.include?(declaration_type)
 
-      errors.add(:declaration_type, I18n.t("declaration.mismatch_declaration_type_for_schedule")) unless schedule&.allowed_declaration_types&.include?(declaration_type)
-    end
-
-    def existing_declaration
-      @existing_declaration ||= participant
-        .declarations
-        .joins(application: :course)
-        .submitted_state
-        .or(
-          participant
-            .declarations
-            .joins(application: :course)
-            .billable,
-        )
-        .find_by(declaration_parameters.merge(application: { courses: { identifier: course_identifier } }))
-    end
-
-    def declaration_parameters
-      {
-        declaration_date:,
-        declaration_type:,
-        lead_provider:,
-      }
+      errors.add(:declaration_type, :mismatch_declaration_type_for_schedule)
     end
 
     def original_declaration
@@ -144,11 +148,9 @@ def validates_billable_slot_available
     def validate_has_passed_field
       self.has_passed = has_passed.to_s
 
-      if has_passed.blank?
-        errors.add(:has_passed, I18n.t("declaration.missing_has_passed"))
-      elsif !%w[true false].include?(has_passed)
-        errors.add(:has_passed, I18n.t("declaration.invalid_has_passed"))
-      end
+      return unless has_passed.blank? || !%w[true false].include?(has_passed)
+
+      errors.add(:has_passed, :invalid)
     end
 
     def validate_has_passed?

app/validators/contract_for_cohort_and_course_validator.rb

@@ -5,7 +5,7 @@ def validate(record)
     return if record.errors.any?
     return unless contract_for_cohort_and_course_missing?(record)
 
-    record.errors.add(:cohort, options[:message] || I18n.t(:missing_npq_contract_for_cohort_and_course))
+    record.errors.add(:cohort, options[:message] || :missing_npq_contract_for_cohort_and_course)
   end
 
 private

app/validators/course_for_participant_validator.rb

@@ -5,7 +5,7 @@ def validate(record)
     return if record.errors.any?
     return if has_accepted_application_for_course_given_course_identifier?(record)
 
-    record.errors.add(:course_identifier, I18n.t(:invalid_course))
+    record.errors.add(:course_identifier, :invalid_course)
   end
 
 private

app/validators/declaration_date_validator.rb

@@ -20,18 +20,18 @@ def date_has_the_right_format(record)
       false
     end
 
-    record.errors.add(:declaration_date, I18n.t("declaration.invalid_declaration_date"))
+    record.errors.add(:declaration_date, :invalid)
   end
 
   def declaration_within_schedule(record)
     return unless record.schedule && record.declaration_date.present?
 
     if record.declaration_date < record.schedule.applies_from.beginning_of_day
-      record.errors.add(:declaration_date, I18n.t("declaration.declaration_before_milestone_start"))
+      record.errors.add(:declaration_date, :declaration_before_milestone_start)
     end
 
     if record.schedule.applies_to.end_of_day <= record.declaration_date
-      record.errors.add(:declaration_date, I18n.t("declaration.declaration_after_milestone_cutoff"))
+      record.errors.add(:declaration_date, :declaration_after_milestone_cutoff)
     end
   end
 end

app/validators/participant_not_withdrawn_validator.rb

@@ -15,7 +15,7 @@ def validate_withdrawals(record)
 
     record
       .errors
-      .add(:participant_id, I18n.t("declaration.declaration_must_be_before_withdrawal_date", withdrawal_date: latest_state.created_at.rfc3339))
+      .add(:participant_id, :declaration_must_be_before_withdrawal_date, withdrawal_date: latest_state.created_at.rfc3339)
   end
 
   def latest_participant_application_state(record)

app/validators/participant_presence_validator.rb

@@ -12,6 +12,6 @@ def validate(record)
   def has_participant?(record)
     return if record.participant.present?
 
-    record.errors.add(:participant_id, I18n.t(:invalid_participant))
+    record.errors.add(:participant_id, :invalid_participant)
   end
 end

config/locales/en.yml

@@ -9,9 +9,7 @@ en:
   invalid_updated_since_filter: "The filter '#/updated_since' must be a valid ISO 8601 date"
   invalid_page_parameters: "The '#/page[page]' and '#/page[per_page]' parameter values must be a valid positive number"
   invalid_data_structure: correct json data structure required. See API docs for reference
-  invalid_participant: "Your update cannot be made as the '#/participant_id' is not recognised. Check participant details and try again."
   schedule_invalid_for_course: Selected schedule is not valid for the course
-  missing_npq_contract_for_cohort_and_course: You cannot change a participant to this cohort as you do not have a contract for the cohort and course. Contact the DfE for assistance.
 
   errors:
     email:
@@ -34,18 +32,25 @@ en:
 
   declaration: &declaration
     future_declaration_date: "The '#/declaration_date' value cannot be a future date. Check the date and try again."
-    invalid_declaration_date: "Enter a valid RCF3339 '#/declaration_date'."
-    declaration_before_milestone_start: "Enter a '#/declaration_date' that's on or after the milestone start."
-    declaration_after_milestone_cutoff: "Enter a '#/declaration_date' that's before the milestone end date."
     declaration_already_exists: A declaration has already been submitted that will be, or has been, paid for this event
-    declaration_must_be_before_withdrawal_date: "This participant withdrew from this course on %{withdrawal_date}. Enter a '#/declaration_date' that's on or before the withdrawal date."
-    missing_declaration_date: "Enter a '#/declaration_date'."
-    missing_declaration_type: "Enter a '#/declaration_type'."
-    missing_npq_contract_for_cohort_and_course: You cannot submit a declaration for this participant as you do not have a contract for the cohort and course. Contact the DfE for assistance.
-    missing_has_passed: "Enter 'true' or 'false' in the '#/has_passed' field to indicate whether this participant has passed or failed their course."
     mismatch_declaration_type_for_schedule: "The property '#/declaration_type' does not exist for this schedule."
-    invalid_has_passed: "Enter 'true' or 'false' in the '#/has_passed' field to indicate whether this participant has passed or failed their course."
-    missing_participant_id: "The property '#/participant_id' must be present"
+    participant_id:
+      blank: "The property '#/participant_id' must be present"
+      declaration_must_be_before_withdrawal_date: "This participant withdrew from this course on %{withdrawal_date}. Enter a '#/declaration_date' that's on or before the withdrawal date."
+      invalid_participant: "Your update cannot be made as the '#/participant_id' is not recognised. Check participant details and try again."
+    declaration_date:
+      blank: "Enter a '#/declaration_date'."
+      invalid: "Enter a valid RCF3339 '#/declaration_date'."
+      declaration_before_milestone_start: "Enter a '#/declaration_date' that's on or after the milestone start."
+      declaration_after_milestone_cutoff: "Enter a '#/declaration_date' that's before the milestone end date."
+    declaration_type:
+      blank: "Enter a '#/declaration_type'."
+    has_passed:
+      invalid: "Enter 'true' or 'false' in the '#/has_passed' field to indicate whether this participant has passed or failed their course."
+    cohort:
+      missing_npq_contract_for_cohort_and_course: You cannot change a participant to this cohort as you do not have a contract for the cohort and course. Contact the DfE for assistance.
+    course_identifier:
+      invalid_course: "The entered '#/course_identifier' is not recognised for the given participant. Check details and try again."
 
   statement: &statement
     no_output_fee_statement: You cannot submit or void declarations for the %{cohort} cohort. The funding contract for this cohort has ended. Get in touch if you need to discuss this with us.
@@ -293,6 +298,12 @@ en:
           attributes:
             completion_date:
               future_date: The '#/%{attribute}' value cannot be a future date. Check the date and try again.
+        declarations/create:
+          <<: *statement
+          attributes:
+            <<: *declaration
+            course_identifier:
+              <<: *participant_course_identifier
 
   omniauth_providers:
     tra_openid_connect: "Get an Identity"

spec/models/application_spec.rb

@@ -344,7 +344,7 @@ def subject.transient_previously_funded
     end
 
     context "when it is not eligible_for_funding but and has a funded place" do
-      subject { create(:application, eligible_for_funding: false, funded_place: false) }
+      subject { create(:application, eligible_for_funding: false, funded_place: true) }
 
       it { is_expected.not_to be_fundable }
     end

spec/models/declaration_spec.rb

@@ -10,6 +10,7 @@
     it { is_expected.to belong_to(:superseded_by).optional }
     it { is_expected.to have_many(:participant_outcomes).dependent(:destroy) }
     it { is_expected.to have_many(:statement_items) }
+    it { is_expected.to have_many(:statements).through(:statement_items) }
   end
 
   describe "validations" do
@@ -20,8 +21,8 @@
       context "when the declaration_date is in the future" do
         it "is not valid" do
           subject.declaration_date = 1.day.from_now
-          expect(subject).not_to be_valid
-          expect(subject.errors[:declaration_date]).to include("The '#/declaration_date' value cannot be a future date. Check the date and try again.")
+          expect(subject).to be_invalid
+          expect(subject.errors.first).to have_attributes(attribute: :declaration_date, type: :future_declaration_date)
         end
       end
 
@@ -231,7 +232,7 @@
     end
   end
 
-  describe "#duplication_declarations" do
+  describe "#duplicate_declarations" do
     let(:cohort) { create(:cohort, :current) }
     let(:course_group) { CourseGroup.find_by(name: "leadership") || create(:course_group, name: "leadership") }
     let(:course) { create(:course, :sl, course_group:) }
@@ -255,7 +256,8 @@
 
         context "when declarations have been made for a different course" do
           before do
-            other_application = create(:application, :accepted, cohort:, user: other_user)
+            course = create(:course, :ehco, course_group:)
+            other_application = create(:application, :accepted, course:, cohort:, user: other_user)
             create(:declaration, application: other_application)
           end