feat: refactor group creation#7975
Conversation
Coverage Report for CI Build 27021988134Coverage increased (+0.005%) to 90.195%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats💛 - Coveralls |
|
|
||
| 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') |
There was a problem hiding this comment.
use self.get_autogenerated_group_name for this and the one below
| ### 🐛 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) |
There was a problem hiding this comment.
"up front" -> "before validation"
|
|
||
| ### 🔧 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) |
There was a problem hiding this comment.
The implications of time are not great for the Changelog, as this historical context distracts from the actual change itself.
I would reword to just "Added NOT NULL constraint on groups.group_name". The PR description can provide additional context
|
|
||
| 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 |
There was a problem hiding this comment.
The Group model still has all three of these validations (which is good). It is true that there are callbacks that ensure these validations pass, but that doesn't mean that the validations themselves aren't checked. So it's appropriate to keep the tests here.
fcb0aaa to
22250e7
Compare
david-yz-liu
left a comment
There was a problem hiding this comment.
Nice work, @YheChen!
Proposed Changes
(Describe your changes here. Also describe the motivation for your changes: what problem do they solve, or how do they improve the application or codebase? If this pull request fixes an open issue, use a keyword to link this pull request to the issue.)
Refactors
Groupcreation to assign the group'sid,group_name, andrepo_nameup front in abefore_validationcallback, eliminating the previous "save with nil name → assign name from id → save again" pattern.What changed:
before_validation :assign_id_and_default_names, on: :createtoGroup. The callback reserves the next value fromgroups_id_seqvianextval, then uses that id to populateid/group_name/repo_nameonly when the caller hasn't supplied them.set_repo_nameafter_createcallback. Its job is now done by the newbefore_validationcallback.check_repo_uniquenessfrom anafter_createcallback that raised on conflict into a standardvalidate :check_repo_uniqueness, on: :createthat adds an error. This avoids partway-through-creation exceptions.Assignment#add_group,Assignment#add_group_api,Student#create_group_for_working_alone_student, andStudent#create_autogenerated_name_group— each collapses from a four-line save/assign/save dance into a singleGroup.create/Group.newcall.NOT NULLconstraint ongroups.group_namein a new migration, now that the model guarantees the column is set before save. Removed the correspondingmissing_non_null_constraintexception from.active_record_doctor.rb.group_spec.rb(validate_presence_of,validate_uniqueness_of,belong_to(:course)forgroup_name) that are no longer testable on:createcontext as the callback fills nil values before validation runs. Added a comment noting that those invariants are now enforced by the callback, the unique index, and the foreign key respectively.Screenshots of your changes (if applicable)
Associated documentation repository pull request (if applicable)
Type of Change
(Write an
Xor a brief description next to the type or types that best describe your changes.)Checklist
(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the
[ ]into a[x]in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)Before opening your pull request:
After opening your pull request:
Questions and Comments
(Include any questions or comments you have regarding your changes.)