From c871d8618f10532341cee55851a36b0111014d6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Leandro=20Alem=C3=A3o?= Date: Thu, 27 Jun 2024 19:07:57 +0100 Subject: [PATCH] [CPDLP-3094] Address more PR comments --- app/models/declaration.rb | 10 +--- app/services/declarations/create.rb | 17 ++++-- .../course_for_participant_validator.rb | 6 +- app/validators/declaration_date_validator.rb | 9 +-- config/locales/en.yml | 14 ++--- db/seeds.rb | 1 - public/api/docs/v1/swagger.yaml | 2 - public/api/docs/v2/swagger.yaml | 5 +- public/api/docs/v3/swagger.yaml | 3 - spec/models/application_spec.rb | 4 +- spec/models/declaration_spec.rb | 60 +++++++++++-------- spec/services/declarations/create_spec.rb | 27 +++++++++ .../api_index_csv_endpoint_support.rb | 2 +- .../api_index_endpoint_support.rb | 2 +- spec/swagger_schemas/models/participant.rb | 3 +- .../requests/application_accept_request.rb | 2 - ...application_change_funded_place_request.rb | 1 - .../course_for_participant_validator_spec.rb | 4 +- .../declaration_date_validator_spec.rb | 19 ++---- ...articipant_not_withdrawn_validator_spec.rb | 2 +- 20 files changed, 101 insertions(+), 92 deletions(-) diff --git a/app/models/declaration.rb b/app/models/declaration.rb index 98922b34e4..c5bdbdeacd 100644 --- a/app/models/declaration.rb +++ b/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 diff --git a/app/services/declarations/create.rb b/app/services/declarations/create.rb index 7d34f12e9f..d016c9a6bf 100644 --- a/app/services/declarations/create.rb +++ b/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 diff --git a/app/validators/course_for_participant_validator.rb b/app/validators/course_for_participant_validator.rb index 4ac952daef..b17e373b9b 100644 --- a/app/validators/course_for_participant_validator.rb +++ b/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, :invalid_course) + record.errors.add(:course_identifier, :invalid) end private @@ -13,8 +13,6 @@ def validate(record) 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 diff --git a/app/validators/declaration_date_validator.rb b/app/validators/declaration_date_validator.rb index 2829f3ef67..60eec1f09b 100644 --- a/app/validators/declaration_date_validator.rb +++ b/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 diff --git a/config/locales/en.yml b/config/locales/en.yml index f2a671269e..70132a9cd1 100644 --- a/config/locales/en.yml +++ b/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,9 +29,6 @@ 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." @@ -41,14 +36,13 @@ en: 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 diff --git a/db/seeds.rb b/db/seeds.rb index ade11f6aec..af93db619f 100644 --- a/db/seeds.rb +++ b/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", diff --git a/public/api/docs/v1/swagger.yaml b/public/api/docs/v1/swagger.yaml index c5df8fae8b..fc3f7b9c03 100644 --- a/public/api/docs/v1/swagger.yaml +++ b/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 ' diff --git a/public/api/docs/v2/swagger.yaml b/public/api/docs/v2/swagger.yaml index f7f539d8b1..314346ac92 100644 --- a/public/api/docs/v2/swagger.yaml +++ b/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 diff --git a/public/api/docs/v3/swagger.yaml b/public/api/docs/v3/swagger.yaml index dfd789a5e0..ab4d57384e 100644 --- a/public/api/docs/v3/swagger.yaml +++ b/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 diff --git a/spec/models/application_spec.rb b/spec/models/application_spec.rb index 16cd4ca421..7226e4a174 100644 --- a/spec/models/application_spec.rb +++ b/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 diff --git a/spec/models/declaration_spec.rb b/spec/models/declaration_spec.rb index 88b4ab5d8a..a514d340b1 100644 --- a/spec/models/declaration_spec.rb +++ b/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 diff --git a/spec/services/declarations/create_spec.rb b/spec/services/declarations/create_spec.rb index d9beabd64f..070ab9df60 100644 --- a/spec/services/declarations/create_spec.rb +++ b/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) diff --git a/spec/support/shared_examples/api_index_csv_endpoint_support.rb b/spec/support/shared_examples/api_index_csv_endpoint_support.rb index c158297b7d..f5e0514de6 100644 --- a/spec/support/shared_examples/api_index_csv_endpoint_support.rb +++ b/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 diff --git a/spec/support/shared_examples/api_index_endpoint_support.rb b/spec/support/shared_examples/api_index_endpoint_support.rb index e7dda26dd3..a80af3bcb7 100644 --- a/spec/support/shared_examples/api_index_endpoint_support.rb +++ b/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 diff --git a/spec/swagger_schemas/models/participant.rb b/spec/swagger_schemas/models/participant.rb index 169315bda2..ad071d183d 100644 --- a/spec/swagger_schemas/models/participant.rb +++ b/spec/swagger_schemas/models/participant.rb @@ -174,7 +174,8 @@ }, training_status: { description: "The training status of the NPQ participant", - type: Application.training_statuses.keys, + type: :string, + enum: Application.training_statuses.keys, example: Application.training_statuses.keys.first, }, school_urn: { diff --git a/spec/swagger_schemas/requests/application_accept_request.rb b/spec/swagger_schemas/requests/application_accept_request.rb index 23c4a711ad..d2fd9c97b4 100644 --- a/spec/swagger_schemas/requests/application_accept_request.rb +++ b/spec/swagger_schemas/requests/application_accept_request.rb @@ -24,7 +24,6 @@ description: "This field indicates whether the application is funded", nullable: false, type: :boolean, - required: false, example: true, }, }, @@ -40,7 +39,6 @@ description: "The new schedule of the participant", nullable: false, type: :string, - required: false, example: Schedule::IDENTIFIERS.first, enum: Schedule::IDENTIFIERS, } diff --git a/spec/swagger_schemas/requests/application_change_funded_place_request.rb b/spec/swagger_schemas/requests/application_change_funded_place_request.rb index 9b5ff47cc8..bf62567f74 100644 --- a/spec/swagger_schemas/requests/application_change_funded_place_request.rb +++ b/spec/swagger_schemas/requests/application_change_funded_place_request.rb @@ -23,7 +23,6 @@ description: "This field indicates whether the application is funded", nullable: false, type: :boolean, - required: true, example: true, }, }, diff --git a/spec/validators/course_for_participant_validator_spec.rb b/spec/validators/course_for_participant_validator_spec.rb index fb246f8553..990e035aca 100644 --- a/spec/validators/course_for_participant_validator_spec.rb +++ b/spec/validators/course_for_participant_validator_spec.rb @@ -46,7 +46,7 @@ def initialize(participant:, course_identifier:) it "has a meaningfull error", :aggregate_failures do expect(subject).to be_invalid - expect(subject.errors.first).to have_attributes(attribute: :course_identifier, type: :invalid_course) + expect(subject.errors.first).to have_attributes(attribute: :course_identifier, type: :invalid) end end @@ -61,7 +61,7 @@ def initialize(participant:, course_identifier:) it "has a meaningfull error", :aggregate_failures do expect(subject).to be_invalid - expect(subject.errors.first).to have_attributes(attribute: :course_identifier, type: :invalid_course) + expect(subject.errors.first).to have_attributes(attribute: :course_identifier, type: :invalid) end end end diff --git a/spec/validators/declaration_date_validator_spec.rb b/spec/validators/declaration_date_validator_spec.rb index c5af02a446..b5833f2279 100644 --- a/spec/validators/declaration_date_validator_spec.rb +++ b/spec/validators/declaration_date_validator_spec.rb @@ -78,38 +78,29 @@ def schedule end end - context "when declaration_date is before the milestone start" do + context "when declaration_date is before the schedule start" do let(:schedule_applies_from_date) { declaration_date + 1.day } it "has a meaningful error", :aggregate_failures do expect(subject).to be_invalid - expect(subject.errors.first).to have_attributes(attribute: :declaration_date, type: :declaration_before_milestone_start) + expect(subject.errors.first).to have_attributes(attribute: :declaration_date, type: :declaration_before_schedule_start) end end - context "when declaration_date is at the milestone start" do + context "when declaration_date is at the schedule start" do let(:schedule_applies_from_date) { declaration_date } it { is_expected.to be_valid } end - context "when declaration_date is in the middle of milestone" do + context "when declaration_date is in the middle of schedule" do it { is_expected.to be_valid } end - context "when declaration_date is at the milestone end" do + context "when declaration_date is at the schedule end" do let(:schedule_applies_to_date) { declaration_date } it { is_expected.to be_valid } end - - context "when declaration_date is after the milestone start" do - let(:schedule_applies_to_date) { declaration_date - 1.day } - - it "has a meaningfull error", :aggregate_failures do - expect(subject).to be_invalid - expect(subject.errors.first).to have_attributes(attribute: :declaration_date, type: :declaration_after_milestone_cutoff) - end - end end end diff --git a/spec/validators/participant_not_withdrawn_validator_spec.rb b/spec/validators/participant_not_withdrawn_validator_spec.rb index dbbd9eb07d..6787b909b6 100644 --- a/spec/validators/participant_not_withdrawn_validator_spec.rb +++ b/spec/validators/participant_not_withdrawn_validator_spec.rb @@ -70,7 +70,7 @@ def initialize(application:, lead_provider:, declaration_date:) context "when participant was withdrawn by another lead provider" do before do - application.application_states.create!(state: :withdrawn, lead_provider: create(:lead_provider, name: "Another Lead Provider")) + application.application_states.create!(state: :withdrawn, lead_provider: LeadProvider.where.not(id: lead_provider.id).first) application.withdrawn! end