Fix duplicate subject validation on secondary course selection #4224

inulty-dfe · 2024-05-22T08:52:33Z

Context

The primary concern of this PR is to validate the Second level courses' subject inputs.

Validate duplicate Secondary Subject inputs
Validate missing master subject, even when subordinate subject supplied

When choosing two subjects to associate with a Second level course, the validation preventing a provider from choosing the same subject twice was not working.

When the validation was fixed, it was noticed that the form fields do not maintain the values that the user entered.

Changes proposed in this pull request && Guidance to review

Add `subordinate_subject_id` attribute to Course

The form sends master_subject_id, subordinate_subject_id and subject_ids but does not process subordinate_subject_id.

This is because master_subject_id is a persisted column on the schema, and subordinate_subject_id is not even an attribute.

I've added subordinate_subject_id as an ActiveModel attribute on the Course so we can work with it explicitly.

This requires an after_initialize hook on the model to populate the property from the associated subjects.

To understand how this happens, you need to understand all about Subjects

But the long story short is that we can populate the form subordinate_subject_id with this attribute.

"master_subject_id"=>"24", "subjects_ids"=>["24", "24"]

Differences between New and Edit

The Course controllers work significantly differently between creating a new course and editing and existing course. The two process share some of the same modules however.

New Course

- SubjectsController#continue
- CourseBasicDetailConcern#build_new_course
- ::Courses::CreationService.call
- AssignSubjectsService

The problem here is that this service is called on every step of the course creation service with accumulating parameters from the view which need to be maintained over requests, including when using the back button. This makes it very messy and tricky to manage state.

The subject parameters are passed to AssignSubjectsService.
Not only is this service called on every step of the create path, it is also called during the editing path.

The params passed to it are an array of subject_ids

The subject_ids are built from the parameters in the URL for the new path.

How the subject_ids are built and managed is explained:

Select master dropdown and blank subordinate
#=> subject_ids [1]
Select subordinate dropdown and blank master
#=> subject_ids [2]

Click back button after selecting only the suboridinate input, and the form populates the master input with the values supplied to the subordinate input previously.

If modern languages is one of the subject ids, the next page is multiple select of languages. These language ids need to be manages across pages in the URL like the original subject ids.

The subject ids array can have more than 3 elements.

The current implementation PREPENDS the language ids to this list of subject ids in the URL.

This means the master id could be the first, second or third+ position in the subject_ids array.

Edit Course

- SubjectsController#continue
- CourseSubjectsForm#build_new_course
- AssignSubjectsService

AssignSubjectsService

This service receives an array of subject_ids

The subject_ids could contain

[master, subordinate]
[subordinate]
[languages_id_1, master, subordinate]
[languages_id_1, languages_id_2, master, subordinate]

It's hard to work with such a variable set of values. It's better if the position of the values is relatively consistent.

The Course Creation process unshifts language ids onto the subject_ids array. I've changed it so they are pushed onto the end now.

The secondary_subject_ids are passed to the AssignSubjectsService without compacting away the missing values.

[master, subordinate, languages_id_1]
[master, subordinate, languages_id_1, languages_id_2]

Errors

The Course creation / edit system depends heavily on errors. The errors on the object need to be managed carefully in order to show the right errors at the right time.

If we add custom, non-model errors to the object, and then call #valid? on the object, the custom errors are cleared and the model errors are applied.

c = Course.new
c.errors
#=> []
c.errors.add(:prop, :validation)
=> #<ActiveModel::Error attribute=prop, type=valdation, options={}>
c.valid?
#=> false
 c.errors
=> #<ActiveModel::Errors [#<ActiveModel::Error attribute=course_code, type=blank, options={:on=>[:create, :update]}>, #<ActiveModel::Error attribute=maths, type=inclusion, o.................
c.errors.key?(:course_code)
#=> true
c.errors.key?(:prop)
#=> false

We must avoid calling valid? on an object that we have added custom errors to.

`course_subjects`

When we do course.subjects = subjects for a new record, that tries to save the records and raises an exception.

There is an ActiveRecord Validation on dupliate subjects, and also a database unique constraint on the foreign keys. So it's impossible to duplicate a subject on a course.

When this happens, the course.subjects does not maintain the two assigned subjects.

 c = Course.last
 c.subjects
 #=> [#<SecondarySubject:0x00007f5be540b918 id: 10, type: "SecondarySubject", subject_code: "C1", subject_name: "Biology">,
 #<SecondarySubject:0x00007f5be540b7d8 id: 33, type: "SecondarySubject", subject_code: nil, su.......
subject = SecondarySubject.first
c.subjects = [subject, subject]
#=> ActiveRecord::RecordInvalid: Validation failed: Subject has already been taken

c.errors
#=> []

c.subjects
=> [#<SecondarySubject:0x00007f5be5400a18 id: 8, type: "SecondarySubject", subject_code: "W1", subject_name: "Art and design">]

It would be great to be able to use the assigned values to repopulate the form directly.

If we begin to use the course.course_subjects association throughout the application, we have more control over the assignment. We can build the association without creating objects.

subject_ids.each do |subject_id|
  course.course_subjects.build(subject_id:)
end

c = Course.new

c.course_subjects.build(subject_id: subject.id)
=> #<CourseSubject:0x00007f5be53758c8 id: nil, course_id: nil, subject_id: 8, created_at: nil, updated_at: nil, position: 0>

c.course_subjects.build(subject_id: subject.id)
=> #<CourseSubject:0x00007f5be5373208 id: nil, course_id: nil, subject_id: 8, created_at: nil, updated_at: nil, position: 1>

c.course_subjects
=> [#<CourseSubject:0x00007f5be53758c8 id: nil, course_id: nil, subject_id: 8, created_at: nil, updated_at: nil, position: 0>,
 #<CourseSubject:0x00007f5be5373208 id: nil, course_id: nil, subject_id: 8, created_at: nil, updated_at: nil, position: 1>]

We can use this to populate the form.

The main benefit of always using course.course_subjects over course.subjects is consistency between new and persisted records.

Videos

New

Validating Primary, Further Education and Secondary Subject inputs

Screencast.from.30-05-24.14.53.28.webm

Edit

Validating Secondary Subject inputs

Screencast.from.30-05-24.14.57.16.webm

Checklist

Make sure all information from the Trello card is in here
Attach to Trello card
Rebased main
Cleaned commit history
Tested by running locally
Inform data insights team due to database changes

This seems like it can only be a vestige of an old implementation

we are adding errors explicitly to the course in the process of updating or creating it. When custom errors are on the course, calling `valid?` will clear the errors unless they are defined on the model

A course has many subjects. When the course is new and we assign subjects like: `course = some_subjects` if there are errors, then we can still access the subjects via: `course.subjects` When the course has been persisted, if there is an error in that assignment, course.subjects returns the value previous to assignment. More importantly, assigning subjects to a persisted course will make a commit to the database. It will also call save on the course and trigger any errors. This clears existing errors on the course. It is far better to consistently "soft assign" the subjects to avoid these pitfalls.

When the params `subject_id=[2,1,3]` reach the ModernLanguagesController we check the ids against a list of other ids. The array returned from this check is in a different order than the array that went in for checking. This PR ensures the subject_ids array is the same order as passed in

This is not a good idea, keeping the empty subject id allows us to determine the source of the subject_id based on it's position in the subjects array. The code is too complicated and brittle to change this now

By passing the empty subordinate param we can destructure the array of subjects assigning the master and subordinate accurately.

Run AssignSubjectService for FurtherEducation and when subject_ids are present

It's easier to separate the parameter validation from the course validation in the Subjects controller. This commit covers all subject types including physics, modern languages, primary, secondary and further education

inulty-dfe

I've left some notes on this PR because it's difficult to read

inulty-dfe · 2024-05-30T14:41:30Z

spec/features/publish/courses/new_engineers_teach_physics_spec.rb

-    "course%5Bis_send%5D=0&course%5Blevel%5D=#{level}&course%5Bmaster_subject_id%5D=#{course_subject.id}&course%5Bsubjects_ids%5D%5B%5D=#{course_subject.id}"
+    [
+      'course%5Bis_send%5D=0',
+      "course%5Blevel%5D=#{level}",
+      "course%5Bmaster_subject_id%5D=#{course_subject.id}",
+      "course%5Bsubjects_ids%5D%5B%5D=#{course_subject.id}",
+      'course%5Bsubordinate_subject_id%5D='
+    ].join('&')


I think this makes the string more readable

inulty-dfe · 2024-05-30T14:43:45Z

spec/features/publish/courses/new_primary_subject_spec.rb

+
+require 'rails_helper'
+
+feature 'selecting a subject', { can_edit_current_and_next_cycles: false } do


I deleted the new_subject_spec and split it into new_primary_subject_spec and new_secondary_subject_spec

I added more specs to support the changes and validation.

inulty-dfe · 2024-05-30T14:45:18Z

spec/models/course_spec.rb

+    context 'when course has two subjects' do
+      it 'set the subordinate subject id to the second subjects_id' do
+        subordinate_subject = find_or_create(:secondary_subject, :physics)
+        course.subjects << subordinate_subject
+        expect(course.reload).to have_attributes({ subordinate_subject_id: subordinate_subject.id })
+      end
+    end
+  end


This tests the Course.after_initialize assignment of the second Secondary subject to the subordinate_subject_id attribute

inulty-dfe · 2024-05-30T14:46:08Z

spec/services/courses/assign_subjects_service_spec.rb

+  describe 'subordinate subject present but master missing' do
+    let(:subject_ids) { [] }
+
+    context 'with a new course' do
+      let(:course) { Course.new }
+
+      it 'raises missing subject error' do
+        expect(subject.errors.full_messages).to include('Select a subject')
+      end
+    end
+
+    context 'with a persisted course' do
+      let(:course) { create(:course) }
+
+      it 'raises missing subject error' do
+        expect(subject.errors.full_messages).to include('Select a subject')
+      end
+    end
+  end


This only gets excercised in the "new course flow" becuase we short circuit the validation in the SubjectsController for update.

inulty-dfe · 2024-05-30T14:47:32Z

app/controllers/publish/courses/modern_languages_controller.rb

-        @updated_subject_list ||= selected_language_subjects_ids.concat(selected_non_language_subjects_ids)
+        @updated_subject_list ||= selected_non_language_subjects_ids.concat(selected_language_subjects_ids)


This contributes to the new ordering of subject ids where the master and subordinate subject ids are at the front and not unshited back when languages are added

inulty-dfe · 2024-05-30T14:59:29Z

app/forms/publish/course_subjects_form.rb

-      super && assign_subjects_service.valid?
+      super && assign_subjects_service.errors.none?


The course is returned from assign_subjects_service.

Calling .valid? on the course will clear out any validations we've added that do not appear on the Course model as validations.

inulty-dfe · 2024-05-30T15:00:45Z

app/models/course.rb

+    if is_primary?
+      self.master_subject_id ||= course_subjects.first&.subject_id
+    else
+      sub = course_subjects.select(&:position)
+
+      self.master_subject_id ||= sub.first&.subject&.id
+
+      self.subordinate_subject_id ||= sub&.second&.subject&.id
+    end


This does not account for Further Education, but there is no form inputs for further Education level courses. The master and subordinate properties are only used for input and validation.

inulty-dfe · 2024-05-30T15:02:13Z

app/services/courses/assign_program_type_service.rb

-      # NOTE: This looks like unwarranted side effects
-      course.save
    end


This is a hangover from a previous version of the application.

This service gets called during the building of a new course. When course.save is called, validate is called on the course, and then all custom errors added to the model are cleared and only the validations defined on the model are run.

This is not what we want when incrementally building up errors in the new course flow.

inulty-dfe · 2024-05-30T15:06:03Z

app/services/courses/creation_service.rb

+      if course_attributes[:master_subject_id].blank? && course_attributes[:subordinate_subject_id].present?
+        course.errors.add(:subjects, :course_creation)
+      elsif subject_ids.present? || course_attributes[:level] == 'further_education'
+        AssignSubjectsService.call(course:, subject_ids:)
+      end
+


This is here so we can avoid validating the subjects unless under specific circumstances.

We don't want to call AssignSubjectsService.call(course:, subject_ids:) when we have not reached the subjects form (choosing primary / secondary level)
We don't want to call it if there are no subject ids either, except in the case when the level is further education because no subject ids are passed for this level type and the Service is responsible for assigning the FurtherEducation subject to that course.

inulty-dfe · 2024-05-30T15:06:55Z

app/services/courses/creation_service.rb

-      AssignSubjectsService.call(course:, subject_ids:)
+      course.valid?(:new) if course.errors.blank?


Avoid calling valid? on a model when you've added custom errors.

Calling valid? will clear your custom errors unless they are defined on the model as validations.

elceebee

There are a few little things, but no blocks. You have navigated this nightmare terrain as well as possible.

elceebee · 2024-05-30T15:12:50Z