WIP: fix: missing null and presence checks

.active_record_doctor.rb

@@ -0,0 +1,10 @@
+ActiveRecordDoctor.configure do
+  # api_key and schema are NOT NULL but are populated by AutotestSetting's
+  # before_create callback (register_autotester), which runs after validation.
+  # A presence validator can never pass on create, so these are exempt.
+  detector :missing_presence_validation,
+           ignore_attributes: [
+             'AutotestSetting.api_key',
+             'AutotestSetting.schema'
+           ]
+end

Changelog.md

@@ -30,6 +30,7 @@
 - Fixed `(hidden)` assignment labeling for assignments with `visible_on` and/or `visible_until` set (#7944)
 
 ### 🔧 Internal changes
+- Added `NOT NULL` constraints and presence/inclusion validators flagged by `active_record_doctor` checks `missing_non_null_constraint` and `missing_presence_validation`
 - Added tests for `MarksGradersController` to achieve full test coverage for `randomly_assign` (#7947)
 - Refactored `AuthenticationHelper#sign_in` to set session values directly instead of going through `MainController#login` (#7962)
 - Updated `MainController` specs to dispatch `post :login` directly in tests that assert on login's response, instead of relying on `sign_in`'s internal request (#7962)

app/jobs/split_pdf_job.rb

@@ -145,6 +145,7 @@ def perform(exam_template, _path, split_pdf_log, _original_filename = nil, _role
           m_logger.log(status)
           split_page_updates << {
             id: split_page_id,
+            split_pdf_log_id: split_pdf_log.id,
             status: status,
             group_id: nil,
             exam_page_number: nil
@@ -156,6 +157,7 @@ def perform(exam_template, _path, split_pdf_log, _original_filename = nil, _role
           m_logger.log(status)
           split_page_updates << {
             id: split_page_id,
+            split_pdf_log_id: split_pdf_log.id,
             status: status,
             group_id: nil,
             exam_page_number: nil
@@ -166,14 +168,15 @@ def perform(exam_template, _path, split_pdf_log, _original_filename = nil, _role
           m_logger.log("#{m[:short_id]}: exam number #{m[:exam_num]}, page #{m[:page_num]}")
           split_page_updates << {
             id: split_page_id,
+            split_pdf_log_id: split_pdf_log.id,
             status: status,
             group_id: group_id,
             exam_page_number: m[:page_num].to_i
           }
         end
       end
       SplitPage.upsert_all(split_page_updates, returning: false)
-      num_complete = save_pages(exam_template, partial_exams, on_duplicate)
+      num_complete = save_pages(exam_template, partial_exams, split_pdf_log, on_duplicate)
       num_incomplete = partial_exams.length - num_complete
 
       split_pdf_log.update(
@@ -210,7 +213,7 @@ def perform(exam_template, _path, split_pdf_log, _original_filename = nil, _role
   end
 
   # Save the pages into groups for this assignment
-  def save_pages(exam_template, partial_exams, on_duplicate = nil)
+  def save_pages(exam_template, partial_exams, split_pdf_log, on_duplicate = nil)
     complete_dir = File.join(exam_template.base_path, 'complete')
     incomplete_dir = File.join(exam_template.base_path, 'incomplete')
     error_dir = File.join(exam_template.base_path, 'error')
@@ -258,7 +261,7 @@ def save_pages(exam_template, partial_exams, on_duplicate = nil)
           status = "ERROR: #{exam_template.name}: exam number #{exam_num}, page #{page_num} already exists"
         end
         # update status of page
-        split_page_updates << { id: split_page_id, status: status }
+        split_page_updates << { id: split_page_id, split_pdf_log_id: split_pdf_log.id, status: status }
       end
       SplitPage.upsert_all(split_page_updates, returning: false)
 

app/models/annotation.rb

@@ -40,7 +40,7 @@
 class Annotation < ApplicationRecord
   belongs_to :submission_file
   belongs_to :annotation_text
-  belongs_to :creator, polymorphic: true
+  belongs_to :creator, polymorphic: true, optional: true
   belongs_to :result
 
   has_one :course, through: :submission_file

app/models/annotation_text.rb

@@ -22,7 +22,7 @@
 #
 # rubocop:enable Layout/LineLength, Lint/RedundantCopDisableDirective
 class AnnotationText < ApplicationRecord
-  belongs_to :creator, class_name: 'Role'
+  belongs_to :creator, class_name: 'Role', optional: true
   belongs_to :last_editor, class_name: 'Role', optional: true
 
   has_one :course, through: :creator

app/models/assessment.rb

@@ -65,6 +65,9 @@ class Assessment < ApplicationRecord
   validate :visible_dates_are_valid
   validates :description, presence: true
   validates :is_hidden, inclusion: { in: [true, false] }
+  validates :show_total, inclusion: { in: [true, false] }
+  validates :type, presence: true
+  validates :message, exclusion: { in: [nil] }
   validates :short_identifier, format: { with: /\A[a-zA-Z0-9\-_]+\z/ }
 
   def self.type

app/models/assignment.rb

@@ -419,7 +419,7 @@ def all_grouping_data
 
   def add_group(new_group_name = nil)
     if group_name_autogenerated
-      group = self.course.groups.new
+      group = self.course.groups.new(group_name: SecureRandom.uuid)
       group.save(validate: false)
       group.group_name = group.get_autogenerated_group_name
       group.save
@@ -447,7 +447,7 @@ def add_group_api(new_group_name = nil, members = [])
             g.repo_name = student_user_name
           end
         elsif group_name_autogenerated
-          group = course.groups.new
+          group = course.groups.new(group_name: SecureRandom.uuid)
           group.save(validate: false)
           group.group_name = group.get_autogenerated_group_name
         else

app/models/assignment_properties.rb

@@ -110,6 +110,12 @@ class AssignmentProperties < ApplicationRecord
   end
 
   validates :scanned_exam, inclusion: { in: [true, false] }
+  validates :allow_remarks, inclusion: { in: [true, false] }
+  validates :anonymize_groups, inclusion: { in: [true, false] }
+  validates :hide_unassigned_criteria, inclusion: { in: [true, false] }
+  validates :is_timed, inclusion: { in: [true, false] }
+  validates :starter_files_after_due, inclusion: { in: [true, false] }
+  validates :release_with_urls, inclusion: { in: [true, false] }
 
   validate :minimum_number_of_groups
 

app/models/course.rb

@@ -48,7 +48,7 @@ class Course < ApplicationRecord
   # Note rails provides built-in sanitization via active record.
   validates :display_name, presence: true
   validates :is_hidden, inclusion: { in: [true, false] }
-  validates :max_file_size, numericality: { greater_than_or_equal_to: 0 }
+  validates :max_file_size, presence: true, numericality: { greater_than_or_equal_to: 0 }
   validate :start_at_before_or_equal_to_end_at
 
   after_save_commit :update_repo_max_file_size

app/models/criterion.rb

@@ -50,6 +50,11 @@ class Criterion < ApplicationRecord
   validates :name, uniqueness: { scope: :assessment_id }
 
   validates :bonus, inclusion: { in: [true, false] }
+  validates :type, presence: true
+  validates :position, presence: true
+  validates :ta_visible, inclusion: { in: [true, false] }
+  validates :peer_visible, inclusion: { in: [true, false] }
+  validates :description, exclusion: { in: [nil] }
 
   validates :max_mark, presence: true
   validates :max_mark, numericality: { greater_than: 0 }
@@ -79,6 +84,7 @@ def update_assigned_groups_count
   def self.randomly_assign_tas(criterion_ids, ta_ids, assignment)
     assign_tas(criterion_ids, ta_ids, assignment) do |c_ids, t_ids|
       # Assign TAs in a round-robin fashion to a list of random criteria.
+      next [] if t_ids.empty?
       shuffled_criterion_ids = c_ids.shuffle
       shuffled_criterion_ids.zip(t_ids.cycle).map(&:flatten)
     end

app/models/exam_template.rb

@@ -40,6 +40,8 @@ class ExamTemplate < ApplicationRecord
             length: { maximum: 20 }
   validates :num_pages, numericality: { greater_than_or_equal_to: 0,
                                         only_integer: true }
+  validates :automatic_parsing, inclusion: { in: [true, false] }
+  validates :cover_fields, exclusion: { in: [nil] }
 
   has_many :split_pdf_logs, dependent: :destroy
   has_many :template_divisions, dependent: :destroy

app/models/extension.rb

@@ -27,7 +27,8 @@ class Extension < ApplicationRecord
 
   attribute :time_delta, :interval
 
-  validates :time_delta, numericality: { greater_than: 0 }
+  validates :time_delta, presence: true, numericality: { greater_than: 0 }
+  validates :apply_penalty, inclusion: { in: [true, false] }
 
   after_create :validate_grouping
 

app/models/grade_entry_item.rb

@@ -38,6 +38,7 @@ class GradeEntryItem < ApplicationRecord
 
   validates :position, presence: true
   validates :position, numericality: { greater_than_or_equal_to: 0 }
+  validates :bonus, inclusion: { in: [true, false] }
 
   BLANK_MARK = ''.freeze
 

app/models/grade_entry_student.rb

@@ -74,6 +74,7 @@ def self.randomly_assign_tas(student_ids, ta_ids, form)
     assign_tas(student_ids, ta_ids, form) do |grade_entry_student_ids, tids|
       # Assign TAs in a round-robin fashion to a list of random grade entry
       # students.
+      next [] if tids.empty?
       grade_entry_student_ids.shuffle.zip(tids.cycle)
     end
   end

app/models/grader_permission.rb

@@ -22,4 +22,8 @@
 class GraderPermission < ApplicationRecord
   belongs_to :ta, class_name: 'Ta', foreign_key: :role_id, inverse_of: :grader_permission
   has_one :course, through: :ta
+
+  validates :manage_assessments, inclusion: { in: [true, false] }
+  validates :manage_submissions, inclusion: { in: [true, false] }
+  validates :run_tests, inclusion: { in: [true, false] }
 end

app/models/grouping.rb

@@ -124,6 +124,8 @@ class Grouping < ApplicationRecord
   has_one :course, through: :assignment
 
   validates :is_collected, inclusion: { in: [true, false] }
+  validates :instructor_approved, inclusion: { in: [true, false] }
+  validates :starter_file_changed, inclusion: { in: [true, false] }
 
   validates :test_tokens, presence: true
   validates :test_tokens, numericality: { greater_than_or_equal_to: 0, only_integer: true }

app/models/lti_client.rb

@@ -18,7 +18,8 @@
 class LtiClient < ApplicationRecord
   has_many :lti_deployments
   has_many :lti_users
-  validates :client_id, uniqueness: { scope: :host }
+  validates :client_id, presence: true, uniqueness: { scope: :host }
+  validates :host, presence: true
   include Rails.application.routes.url_helpers
 
   KEY_PATH = File.join(Settings.file_storage.lti || File.join(Settings.file_storage.default_root_path, 'lti',

app/models/lti_deployment.rb

@@ -26,7 +26,9 @@ class LtiDeployment < ApplicationRecord
   belongs_to :lti_client
   has_many :lti_services, dependent: :destroy
   has_many :lti_line_items, dependent: :destroy
-  validates :lms_course_id, uniqueness: { scope: :lti_client }, allow_nil: true
+  validates :external_deployment_id, presence: true
+  validates :lms_course_id, presence: true, uniqueness: { scope: :lti_client }
+  validates :lms_course_name, presence: true
   # See LTI documentation for full lists of scopes/claims/roles
   # https://www.imsglobal.org/spec/lti/v1p3
   LTI_SCOPES = { names_role: 'https://purl.imsglobal.org/spec/lti-nrps/scope/contextmembership.readonly',

app/models/lti_line_item.rb

@@ -26,4 +26,5 @@ class LtiLineItem < ApplicationRecord
   belongs_to :assessment
   belongs_to :lti_deployment
   validates :assessment, uniqueness: { scope: :lti_deployment_id }
+  validates :lti_line_item_id, presence: true
 end

app/models/lti_service.rb

@@ -28,4 +28,5 @@ class LtiService < ApplicationRecord
   belongs_to :lti_deployment
   validates :service_type, inclusion: { in: LTI_SERVICES.values }
   validates :service_type, uniqueness: { scope: :lti_deployment_id }
+  validates :url, presence: true
 end

app/models/lti_user.rb

@@ -20,5 +20,5 @@
 class LtiUser < ApplicationRecord
   belongs_to :lti_client
   belongs_to :user
-  validates :lti_user_id, uniqueness: { scope: :lti_client_id }
+  validates :lti_user_id, presence: true, uniqueness: { scope: :lti_client_id }
 end

app/models/result.rb

@@ -47,6 +47,7 @@ class Result < ApplicationRecord
   validates :marking_state, inclusion: { in: MARKING_STATES.values }
 
   validates :released_to_students, inclusion: { in: [true, false] }
+  validates :view_token, presence: true
 
   before_update :check_for_released
 

app/models/role.rb

@@ -52,7 +52,11 @@ class Role < ApplicationRecord
   has_many :last_updated_grades, class_name: 'Grade', foreign_key: 'last_updated_by_id', inverse_of: :last_updated_by,
                                  dependent: :nullify
 
-  validates :type, format: { with: /\AStudent|Instructor|Ta|AdminRole\z/ }
+  validates :type, presence: true, format: { with: /\AStudent|Instructor|Ta|AdminRole\z/ }
+  validates :grace_credits, presence: true
+  validates :hidden, inclusion: { in: [true, false] }
+  validates :receives_invite_emails, inclusion: { in: [true, false] }
+  validates :receives_results_emails, inclusion: { in: [true, false] }
   validates :user_id, uniqueness: { scope: :course_id }
   after_update_commit :update_repo_permissions
 

app/models/split_pdf_log.rb

@@ -39,6 +39,7 @@ class SplitPdfLog < ApplicationRecord
   validates :num_groups_in_complete, :num_groups_in_complete, :num_pages_qr_scan_error, :original_num_pages,
             numericality: { greater_than_or_equal_to: 0,
                             only_integer: true }
+  validates :qr_code_found, inclusion: { in: [true, false] }
 
   validate :courses_should_match
 

app/models/starter_file_entry.rb

@@ -20,6 +20,7 @@
 class StarterFileEntry < ApplicationRecord
   belongs_to :starter_file_group
   has_one :course, through: :starter_file_group
+  validates :path, presence: true
   validate :entry_exists, if: :starter_file_group
 
   has_many :grouping_starter_file_entries, dependent: :destroy

app/models/starter_file_group.rb

@@ -39,6 +39,7 @@ class StarterFileGroup < ApplicationRecord
 
   validates :entry_rename, exclusion: { in: %w[.. .] }
   validates :entry_rename, presence: { if: -> { self.use_rename } }
+  validates :use_rename, inclusion: { in: [true, false] }
 
   def path
     Pathname.new(assignment.starter_file_path) + id.to_s

app/models/student.rb

@@ -70,10 +70,6 @@ class Student < Role
 
   validates :section, presence: { unless: -> { section_id.nil? } }
 
-  validates :receives_invite_emails, inclusion: { in: [true, false] }
-
-  validates :receives_results_emails, inclusion: { in: [true, false] }
-
   validates :grace_credits,
             numericality: { only_integer: true,
                             greater_than_or_equal_to: 0 }
@@ -149,7 +145,7 @@ def create_group_for_working_alone_student(aid)
       if @assignment.is_timed
         # must use a new group for timed assignments so that repos are
         # not accessible before the student starts the timer
-        @group = Group.new(course: @assignment.course)
+        @group = Group.new(course: @assignment.course, group_name: SecureRandom.uuid)
         @group.save(validate: false)
         @group.group_name = @group.get_autogenerated_group_name
       else
@@ -188,9 +184,9 @@ def create_group_for_working_alone_student(aid)
 
   def create_autogenerated_name_group(assignment)
     Group.transaction do
-      group = Group.new(course: assignment.course)
+      group = Group.new(course: assignment.course, group_name: SecureRandom.uuid)
       group.save(validate: false)
-      group.group_name = group.get_autogenerated_group_name # the autogen name depends on the id, hence the two saves
+      group.group_name = group.get_autogenerated_group_name
       group.save
       grouping = Grouping.create(assignment: assignment, group_id: group.id)
       StudentMembership.create(grouping_id: grouping.id, membership_status: StudentMembership::STATUSES[:inviter],

app/models/submission.rb

@@ -29,6 +29,7 @@ class Submission < ApplicationRecord
 
   validates :submission_version_used, inclusion: { in: [true, false] }
   validates :submission_version, numericality: { only_integer: true }
+  validates :is_empty, inclusion: { in: [true, false] }
   validate :max_number_of_results
   belongs_to :grouping
 

app/models/submission_file.rb

@@ -33,6 +33,7 @@ class SubmissionFile < ApplicationRecord
   validates :path, presence: true
 
   validates :is_converted, inclusion: { in: [true, false] }
+  validates :error_converting, inclusion: { in: [true, false] }
 
   def is_supported_image?
     # Here you can add more image types to support

app/models/template_division.rb

@@ -31,14 +31,14 @@ class TemplateDivision < ApplicationRecord
   accepts_nested_attributes_for :assignment_file, allow_destroy: true
 
   validate :courses_should_match
-  validates :start, numericality: { greater_than_or_equal_to: 2,
-                                    less_than_or_equal_to: :end,
-                                    only_integer: true }
-  validates :end, numericality: { only_integer: true }
+  validates :start, presence: true, numericality: { greater_than_or_equal_to: 2,
+                                                    less_than_or_equal_to: :end,
+                                                    only_integer: true }
+  validates :end, presence: true, numericality: { only_integer: true }
   validate :end_should_be_less_than_or_equal_to_num_pages
-  validates :label,
-            uniqueness: { scope: :exam_template,
-                          allow_blank: false }
+  validates :label, presence: true,
+                    uniqueness: { scope: :exam_template,
+                                  allow_blank: false }
 
   after_save :set_defaults_for_assignment_file # when template division is created or updated
 

app/models/test_group.rb

@@ -35,6 +35,8 @@ class TestGroup < ApplicationRecord
 
   validates :name, presence: true
   validates :display_output, presence: true
+  validates :autotest_settings, exclusion: { in: [nil] }
+  validates :position, presence: true
   validate :courses_should_match
 
   def to_json(*_args)

app/models/test_result.rb

@@ -31,6 +31,7 @@ class TestResult < ApplicationRecord
   validates :name, presence: true, uniqueness: { scope: :test_group_result }
   validates :status, presence: true, inclusion: { in: %w[pass partial fail error error_all] }
   validates :marks_earned, :marks_total, presence: true, numericality: { greater_than_or_equal_to: 0 }
+  validates :position, presence: true
   validates :time, numericality: { greater_than_or_equal_to: 0, only_integer: true, allow_nil: true }
   # output could be empty in some situations
   validates :output, presence: true, if: ->(o) { o.output.nil? }

app/models/test_run.rb

@@ -41,6 +41,7 @@ class TestRun < ApplicationRecord
 
   has_one :course, through: :role
 
+  validates :status, presence: true
   validate :courses_should_match
   validate :autotest_test_id_uniqueness
   before_save :unset_autotest_test_id

app/models/user.rb

@@ -45,7 +45,7 @@ class User < ApplicationRecord
   has_many :lti_users, dependent: :destroy
   validates :type, format: { with: /\AEndUser|AutotestUser|AdminUser\z/ }
 
-  validates :user_name, :last_name, :first_name, :time_zone, :display_name, presence: true
+  validates :user_name, :last_name, :first_name, :time_zone, :display_name, :theme, presence: true
   validates :user_name, uniqueness: true
   validates :email, uniqueness: { allow_nil: true }
   validates :id_number, uniqueness: { allow_nil: true }