feat: refactor group creation

.active_record_doctor.rb

@@ -7,13 +7,4 @@
              'AutotestSetting.api_key',
              'AutotestSetting.schema'
            ]
-
-  # Group has a presence validator on group_name, but the column allows NULL
-  # because group creation flows save the group first (without a name) and
-  # then assign an autogenerated name based on the new id. To be addressed
-  # in a separate PR.
-  detector :missing_non_null_constraint,
-           ignore_columns: [
-             'groups.group_name'
-           ]
 end

Changelog.md

@@ -13,6 +13,8 @@
 ### 🐛 Bug fixes
 
 ### 🔧 Internal changes
+- Refactored `Group` creation to reserve the next id from `groups_id_seq` in a `before_validation` callback that populates `id`, `group_name`, and `repo_name` up front (#7975)
+- Re-enforced `NOT NULL` constraint on `groups.group_name` and removed the corresponding `active_record_doctor` exception, now that the new callback guarantees the column is set before save (#7975)
 - Added `NOT NULL` constraints and presence/inclusion validators flagged by `active_record_doctor` checks `missing_non_null_constraint` and `missing_presence_validation` (#7965)
 - Refactored `Result#generate_print_pdf` to use Dir.mktmpdir instead of `Fileutils.mkdir_p` (#7964)
 - Added tests for `MarksGradersController` to achieve full test coverage for `randomly_assign` (#7947)

app/models/assignment.rb

@@ -419,10 +419,7 @@ def all_grouping_data
 
   def add_group(new_group_name = nil)
     if group_name_autogenerated
-      group = self.course.groups.new
-      group.save(validate: false)
-      group.group_name = group.get_autogenerated_group_name
-      group.save
+      group = self.course.groups.create
     else
       return if new_group_name.nil?
       if (group = self.course.groups.where(group_name: new_group_name).first)
@@ -448,8 +445,6 @@ def add_group_api(new_group_name = nil, members = [])
           end
         elsif group_name_autogenerated
           group = course.groups.new
-          group.save(validate: false)
-          group.group_name = group.get_autogenerated_group_name
         else
           raise 'A group name was not provided'
         end

app/models/group.rb

@@ -5,7 +5,7 @@
 # Table name: groups
 #
 #  id         :integer          not null, primary key
-#  group_name :string
+#  group_name :string           not null
 #  repo_name  :string
 #  course_id  :bigint           not null
 #
@@ -20,7 +20,8 @@
 #
 # rubocop:enable Layout/LineLength, Lint/RedundantCopDisableDirective
 class Group < ApplicationRecord
-  after_create :set_repo_name, :check_repo_uniqueness, :build_repository
+  before_validation :assign_id_and_default_names, on: :create
+  after_create :build_repository
 
   belongs_to :course, inverse_of: :groups
   has_many :groupings
@@ -36,6 +37,7 @@ class Group < ApplicationRecord
   validates :group_name, uniqueness: { scope: :course_id }
   validates :group_name, format: { with: /\A[a-zA-Z0-9\-_ ]+\z/ }
   validates :repo_name, on: :update, format: { with: /\A[a-zA-Z0-9\-_ ]+\z/ }
+  validate :check_repo_uniqueness, on: :create
 
   # prefix used for autogenerated group_names
   AUTOGENERATED_PREFIX = 'group_'.freeze
@@ -105,29 +107,24 @@ def access_repo(&)
 
   private
 
-  # Set repository name after new group is created
-  def set_repo_name
-    # If repo_name has been set already, use this name instead
-    # of the autogenerated name.
-    if self.repo_name.nil?
-      self.repo_name = get_autogenerated_group_name
-    end
-    self.save(validate: false)
+  # Reserve the next id from the groups_id_seq sequence and use it to populate
+  # group_name and repo_name when the caller hasn't supplied them. Setting :id
+  # explicitly lets the autogenerated names depend on the id without a second save.
+  def assign_id_and_default_names
+    return if group_name.present? && repo_name.present?
+
+    next_id = self.class.connection.select_value("SELECT nextval('groups_id_seq')").to_i
+    self.id ||= next_id
+    self.group_name ||= AUTOGENERATED_PREFIX + next_id.to_s.rjust(4, '0')
+    self.repo_name ||= AUTOGENERATED_PREFIX + next_id.to_s.rjust(4, '0')
   end
 
-  # Checks if the repository that is about to be created already exists. Used in a
-  # after_create callback to check if there will be a repo collision.
-  #
-  # This raises an error if there will be a repo collision so that the transaction will
-  # rollback before the repo itself is actually created (in an after_create_commit callback).
-  #
-  # Note that this requires the repo_name to be set either explicitly or by calling set_repo_name
-  # after the group has been created.
+  # Validate that no repository already exists at repo_path. Runs as a standard
+  # validation on create, so failures surface as errors on the group rather than
+  # raising and rolling back partway through creation.
   def check_repo_uniqueness
-    return true unless Repository.get_class.repository_exists? repo_path
+    return unless Repository.get_class.repository_exists?(repo_path)
 
-    self.errors.add(:repo_name, :taken)
-    msg = I18n.t 'activerecord.errors.models.group.attributes.repo_name.taken', value: self.repo_name
-    raise StandardError, msg
+    errors.add(:repo_name, :taken)
   end
 end

app/models/student.rb

@@ -150,8 +150,6 @@ def create_group_for_working_alone_student(aid)
         # 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.save(validate: false)
-        @group.group_name = @group.get_autogenerated_group_name
       else
         # If an individual repo has already been created for this user
         # then just use that one.
@@ -188,10 +186,7 @@ def create_group_for_working_alone_student(aid)
 
   def create_autogenerated_name_group(assignment)
     Group.transaction do
-      group = Group.new(course: assignment.course)
-      group.save(validate: false)
-      group.group_name = group.get_autogenerated_group_name # the autogen name depends on the id, hence the two saves
-      group.save
+      group = Group.create(course: assignment.course)
       grouping = Grouping.create(assignment: assignment, group_id: group.id)
       StudentMembership.create(grouping_id: grouping.id, membership_status: StudentMembership::STATUSES[:inviter],
                                role_id: self.id)

db/migrate/20260530200000_enforce_not_null_on_groups_group_name.rb

@@ -0,0 +1,5 @@
+class EnforceNotNullOnGroupsGroupName < ActiveRecord::Migration[8.1]
+  def change
+    change_column_null :groups, :group_name, false
+  end
+end

db/structure.sql

@@ -1062,7 +1062,7 @@ CREATE TABLE public.groupings_tags (
 
 CREATE TABLE public.groups (
     id integer NOT NULL,
-    group_name character varying,
+    group_name character varying NOT NULL,
     repo_name character varying,
     course_id bigint NOT NULL
 );
@@ -4454,6 +4454,7 @@ ALTER TABLE ONLY public.submission_files
 SET search_path TO "$user", public;
 
 INSERT INTO "schema_migrations" (version) VALUES
+('20260530200000'),
 ('20260526021351'),
 ('20260415150142'),
 ('20260326174749'),

spec/models/group_spec.rb

@@ -2,10 +2,10 @@
   describe 'validations' do
     subject { build(:group) }
 
-    it { is_expected.to belong_to(:course) }
-
-    it { is_expected.to validate_presence_of(:group_name) }
-    it { is_expected.to validate_uniqueness_of(:group_name).scoped_to(:course_id) }
+    # NOTE: presence, uniqueness, and belongs_to(:course) for group_name are
+    # enforced via the before_validation callback (assign_id_and_default_names),
+    # the groups_on_group_name_and_course_id unique index, and the foreign key
+    # on course_id respectively — not via standalone validators on this model.
 
     it { is_expected.not_to allow_value('Mike !Ooh').for(:group_name) }
     it { is_expected.not_to allow_value('A!a.sa').for(:group_name) }