[CPDLP-3094] Address more PR comments

app/models/declaration.rb

@@ -46,7 +46,6 @@ class Declaration < ApplicationRecord
   }, _suffix: true
 
   validates :declaration_date, :declaration_type, presence: true
-  validate :declaration_date_not_in_the_future
 
   def billable_statement
     statement_items.find(&:billable?)&.statement
@@ -85,21 +84,16 @@ def ineligible_for_funding_reason
   def duplicate_declarations
     self
       .class
+      .billable_or_changeable
       .joins(application: %i[user course])
       .where(user: { trn: application.user.trn })
       .where.not(user: { trn: nil })
+      .where.not(user: { id: application.user_id })
       .where.not(id:)
-      .where(state: %w[submitted eligible payable paid])
       .where(
         declaration_type:,
         superseded_by_id: nil,
         application: { course: application.course.rebranded_alternative_courses },
       )
   end
-
-private
-
-  def declaration_date_not_in_the_future
-    errors.add(:declaration_date, :future_declaration_date) if declaration_date&.future?
-  end
 end

app/services/declarations/create.rb

@@ -26,6 +26,7 @@ class Create
     validate :validate_has_passed_field, if: :validate_has_passed?
     validate :validate_schedule_exists
     validate :validates_billable_slot_available
+    validate :declaration_date_not_in_the_future
 
     attr_reader :raw_declaration_date, :declaration
 
@@ -52,13 +53,13 @@ def application
         &.applications
         &.accepted
         &.includes(:course)
-        &.find_by(lead_provider:, course: { identifier: course_identifier })
+        &.find_by(lead_provider:, course: Course.find_by(identifier: course_identifier)&.rebranded_alternative_courses)
     end
 
     def participant
       @participant ||= begin
         Participants::Query.new(lead_provider:).participant(ecf_id: participant_id)
-      rescue StandardError
+      rescue ActiveRecord::RecordNotFound
         nil
       end
     end
@@ -93,7 +94,7 @@ def existing_declaration
     end
 
     def find_or_create_declaration!
-      @declaration ||= existing_declaration || Declaration.create!(declaration_parameters.merge(application:, cohort:))
+      @declaration = existing_declaration || Declaration.create!(declaration_parameters.merge(application:, cohort:))
     end
 
     def statement_attacher
@@ -105,7 +106,7 @@ 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?
+      return if lead_provider.next_output_fee_statement(cohort).present?
 
       errors.add(:cohort, :no_output_fee_statement, cohort: cohort.start_year)
     end
@@ -135,14 +136,18 @@ def validates_billable_slot_available
       return unless participant
 
       return unless Declaration
+                      .billable_or_changeable
                       .joins(application: %i[user course])
                       .where(
                         application: { user: participant, courses: { identifier: course_identifier } },
                         declaration_type:,
-                        state: %w[submitted eligible payable paid],
                       ).exists?
 
-      errors.add(:base, I18n.t("declaration.declaration_already_exists"))
+      errors.add(:base, :declaration_already_exists)
+    end
+
+    def declaration_date_not_in_the_future
+      errors.add(:declaration_date, :future_declaration_date) if declaration_date&.future?
     end
 
     def validate_has_passed_field

app/validators/course_for_participant_validator.rb

@@ -5,16 +5,14 @@ def validate(record)
     return if record.errors.any?
     return if has_accepted_application_for_course_given_course_identifier?(record)
 
-    record.errors.add(:course_identifier, :invalid_course)
+    record.errors.add(:course_identifier, :invalid)
   end
 
 private
 
   def has_accepted_application_for_course_given_course_identifier?(record)
     return if record.participant.blank?
 
-    record.participant.applications.joins(:course).accepted.active.map { |application|
-      application.course.rebranded_alternative_courses.map(&:identifier)
-    }.flatten.include?(record.course_identifier)
+    record.participant.applications.accepted.joins(:course).where(course: Course.find_by(identifier: record.course_identifier)&.rebranded_alternative_courses).any?
   end
 end

app/validators/declaration_date_validator.rb

@@ -25,13 +25,8 @@ def date_has_the_right_format(record)
 
   def declaration_within_schedule(record)
     return unless record.schedule && record.declaration_date.present?
+    return unless record.declaration_date < record.schedule.applies_from.beginning_of_day
 
-    if record.declaration_date < record.schedule.applies_from.beginning_of_day
-      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, :declaration_after_milestone_cutoff)
-    end
+    record.errors.add(:declaration_date, :declaration_before_schedule_start)
   end
 end

config/locales/en.yml

@@ -5,8 +5,6 @@ en:
   bad_request: "Bad request"
   not_found: "Not found"
   invalid_date_filter: "The filter '#/%{attribute}' must be a valid ISO 8601 date"
-  invalid_course: "The entered '#/course_identifier' is not recognised for the given participant. Check details and try again."
-  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
   schedule_invalid_for_course: Selected schedule is not valid for the course
@@ -31,24 +29,20 @@ en:
     cannot_change_funded_place: "You must void or claw back your declarations for this participant before being able to set '#/funded_place' to false"
 
   declaration: &declaration
-    future_declaration_date: "The '#/declaration_date' value cannot be a future date. Check the date and try again."
-    declaration_already_exists: A declaration has already been submitted that will be, or has been, paid for this event
-    mismatch_declaration_type_for_schedule: "The property '#/declaration_type' does not exist for this schedule."
     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_before_schedule_start: "Enter a '#/declaration_date' that's on or after the schedule start."
+      future_declaration_date: "The '#/declaration_date' value cannot be a future date. Check the date and try again."
     declaration_type:
       blank: "Enter a '#/declaration_type'."
+      mismatch_declaration_type_for_schedule: "The property '#/declaration_type' does not exist for this schedule."
     has_passed:
       invalid: "Enter 'true' or 'false' in the '#/has_passed' field to indicate whether this participant has passed or failed their course."
-    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.
@@ -64,6 +58,7 @@ en:
   participant_course_identifier: &participant_course_identifier
     blank: Enter a '#/course_identifier' value for this participant.
     inclusion: The entered '#/course_identifier' is not recognised for the given participant. Check details and try again.
+    invalid: "The entered '#/course_identifier' is not recognised for the given participant. Check details and try again."
 
   participant_reason: &participant_reason
     blank: The property '#/reason' must be present
@@ -297,6 +292,7 @@ en:
             completion_date:
               future_date: The '#/%{attribute}' value cannot be a future date. Check the date and try again.
         declarations/create:
+          declaration_already_exists: A declaration has already been submitted that will be, or has been, paid for this event
           <<: *statement
           attributes:
             <<: *declaration

db/seeds.rb

@@ -24,7 +24,6 @@ def load_base_file(file)
   "add_schedules.rb",
   "add_lead_providers.rb",
   "add_itt_providers.rb",
-  "add_statements.rb",
   "add_users.rb",
   "add_applications.rb",
   "add_settings.rb",

public/api/docs/v1/swagger.yaml

@@ -957,7 +957,6 @@ components:
                   description: This field indicates whether the application is funded
                   nullable: false
                   type: boolean
-                  required: false
                   example: true
     ApplicationChangeFundedPlaceRequest:
       description: A NPQ application change funded place request
@@ -987,7 +986,6 @@ components:
                   description: This field indicates whether the application is funded
                   nullable: false
                   type: boolean
-                  required: true
                   example: true
     DeclarationsCsvResponse:
       description: 'A list of NPQ enrolments in the Comma Separated Value (CSV) format  '

public/api/docs/v2/swagger.yaml

@@ -1058,7 +1058,6 @@ components:
                   description: This field indicates whether the application is funded
                   nullable: false
                   type: boolean
-                  required: false
                   example: true
     ApplicationChangeFundedPlaceRequest:
       description: A NPQ application change funded place request
@@ -1088,7 +1087,6 @@ components:
                   description: This field indicates whether the application is funded
                   nullable: false
                   type: boolean
-                  required: true
                   example: true
     EnrolmentsCsvResponse:
       description: 'A list of NPQ enrolments in the Comma Separated Value (CSV) format  '
@@ -1631,7 +1629,8 @@ components:
                     example: db3a7848-7308-4879-942a-c4a70ced400a
                   training_status:
                     description: The training status of the NPQ participant
-                    type:
+                    type: string
+                    enum:
                     - active
                     - deferred
                     - withdrawn

public/api/docs/v3/swagger.yaml

@@ -1139,13 +1139,11 @@ components:
                   description: This field indicates whether the application is funded
                   nullable: false
                   type: boolean
-                  required: false
                   example: true
                 schedule_identifier:
                   description: The new schedule of the participant
                   nullable: false
                   type: string
-                  required: false
                   example: npq-aso-march
                   enum:
                   - npq-aso-march
@@ -1188,7 +1186,6 @@ components:
                   description: This field indicates whether the application is funded
                   nullable: false
                   type: boolean
-                  required: true
                   example: true
     ParticipantResponse:
       description: A single NPQ participant

spec/models/application_spec.rb

@@ -343,8 +343,8 @@ def subject.transient_previously_funded
       it { is_expected.to be_fundable }
     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: true) }
+    context "when it is not eligible_for_funding but and has a funded false" do
+      subject { create(:application, eligible_for_funding: false, funded_place: false) }
 
       it { is_expected.not_to be_fundable }
     end

spec/models/declaration_spec.rb

@@ -16,30 +16,6 @@
   describe "validations" do
     it { is_expected.to validate_presence_of(:declaration_type) }
     it { is_expected.to validate_presence_of(:declaration_date) }
-
-    describe "declaration_date" do
-      context "when the declaration_date is in the future" do
-        it "is not valid" do
-          subject.declaration_date = 1.day.from_now
-          expect(subject).to be_invalid
-          expect(subject.errors.first).to have_attributes(attribute: :declaration_date, type: :future_declaration_date)
-        end
-      end
-
-      context "when the declaration_date is today" do
-        it "is valid" do
-          subject.declaration_date = Time.zone.today
-          expect(subject).to be_valid
-        end
-      end
-
-      context "when the declaration_date is in the past" do
-        it "is valid" do
-          subject.declaration_date = 1.day.ago
-          expect(subject).to be_valid
-        end
-      end
-    end
   end
 
   describe "delegations" do
@@ -273,5 +249,41 @@
         end
       end
     end
+
+    context "when a declaration has been superseded by another" do
+      before { create(:declaration, application:, superseded_by: declaration) }
+
+      it "returns no declarations" do
+        expect(declaration.duplicate_declarations).to be_empty
+      end
+    end
+
+    context "when a declaration has a different type" do
+      before { create(:declaration, application:, declaration_type: :completed) }
+
+      it "returns no declarations" do
+        expect(declaration.duplicate_declarations).to be_empty
+      end
+    end
+
+    context "when a declaration has a not billable/submitted state" do
+      before { create(:declaration, application:, state: :clawed_back) }
+
+      it "returns no declarations" do
+        expect(declaration.duplicate_declarations).to be_empty
+      end
+    end
+
+    context "when declarations have been made for a different course" do
+      before do
+        course = create(:course, :ehco, course_group:)
+        other_application = create(:application, :accepted, course:, cohort:, user: participant)
+        create(:declaration, application: other_application)
+      end
+
+      it "returns no declarations" do
+        expect(declaration.duplicate_declarations).to be_empty
+      end
+    end
   end
 end

spec/services/declarations/create_spec.rb

@@ -79,6 +79,27 @@
       end
     end
 
+    context "when the declaration_date is in the future" do
+      before { params[:declaration_date] = 1.day.from_now.rfc3339 }
+
+      it "has a meaningful error", :aggregate_failures do
+        expect(service).to be_invalid
+        expect(service.errors.first).to have_attributes(attribute: :declaration_date, type: :future_declaration_date)
+      end
+    end
+
+    context "when the declaration_date is today" do
+      before { params[:declaration_date] = Time.zone.today.rfc3339 }
+
+      it { is_expected.to be_valid }
+    end
+
+    context "when the declaration_date is in the past" do
+      before { params[:declaration_date] = 1.day.ago.rfc3339 }
+
+      it { is_expected.to be_valid }
+    end
+
     context "when a participant has been withdrawn" do
       before do
         travel_to(withdrawal_time) do
@@ -101,6 +122,12 @@
     context "when an existing declaration already exists" do
       before { service.create_declaration }
 
+      it "has a meaningful error" do
+        expect(subject).to be_invalid
+
+        expect(service.errors.first).to have_attributes(attribute: :base, type: :declaration_already_exists)
+      end
+
       context "when the state submitted" do
         it "does not create duplicates" do
           expect { service.create_declaration }.not_to change(Declaration, :count)

spec/support/shared_examples/api_index_csv_endpoint_support.rb

@@ -7,7 +7,7 @@
       let!(:resource2) { create_resource(lead_provider: current_lead_provider) }
 
       before do
-        create_resource(lead_provider: create(:lead_provider, name: "Another lead provider"))
+        create_resource(lead_provider: LeadProvider.where.not(id: current_lead_provider.id).first)
       end
 
       it "returns a header row and 2 resources" do

spec/support/shared_examples/api_index_endpoint_support.rb

@@ -7,7 +7,7 @@
       let!(:resource2) { create_resource(lead_provider: current_lead_provider) }
 
       before do
-        create_resource(lead_provider: create(:lead_provider, name: "Another lead provider"))
+        create_resource(lead_provider: LeadProvider.where.not(id: current_lead_provider.id).first)
       end
 
       it "returns 2 resources" do