Use username as group name when creating individual groups via API #7481

Raine-Yang-UofT · 2025-04-04T17:24:49Z

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.)

...

Add members parameter in /api/courses/:course_id/assignments/:assignment_id/groups endpoint to create a group with members.
For an individual assignment (group_max = 1), the group name created via API with one member is set to the member name.

Screenshots of your changes (if applicable)

Associated documentation repository pull request (if applicable)

MarkUsProject/Wiki/pull/231

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)	X
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

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:

I have performed a self-review of my changes.
- Check that all changed files included in this pull request are intentional changes.
- Check that all changes are relevant to the purpose of this pull request, as described above.
I have added tests for my changes, if applicable.
- This is required for all bug fixes and new features.
I have updated the project documentation, if applicable.
- This is required for new features.
If this is my first contribution, I have added myself to the list of contributors.

After opening your pull request:

I have updated the project Changelog (this is required for all changes).
I have verified that the pre-commit.ci checks have passed.
I have verified that the CI tests have passed.
I have reviewed the test coverage changes reported by Coveralls.
I have requested a review from a project maintainer.

Questions and Comments

(Include any questions or comments you have regarding your changes.)

Resolve N+1 query issue in creating members Wrap group creation in transaction block

… group-name-generation

coveralls · 2025-04-04T17:53:02Z

Pull Request Test Coverage Report for Build 14286000465

Details

95 of 97 (97.94%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.01%) to 91.887%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
app/models/assignment.rb	27	29	93.1%

Totals
Change from base Build 14283696351:	0.01%
Covered Lines:	41547
Relevant Lines:	44532

💛 - Coveralls

david-yz-liu

@Raine-Yang-UofT nice work!

david-yz-liu · 2025-04-05T14:09:06Z

app/models/assignment.rb

+    group = nil
+
+    Group.transaction do
+      group = if new_group_name.nil?


In general I'd like to avoid this style of variable assignment. Instead, keep the if statement at the outer level and initialize group in each branch. This also allows you to delete the group = nil line above.

david-yz-liu · 2025-04-05T14:14:19Z

app/models/assignment.rb

+      group = if new_group_name.nil?
+                if members.length == 1 && self.group_max == 1
+                  student_user_name = members.first
+                  if self.is_timed


This is a great condition to check. Putting it here is a larger divergence from the original logic (which didn't check is_timed at all).

So instead of putting it here, add it to the condition on line 401 (&& !self.is_timed), so that for timed assessments the previous logic will execute. This also helps remove some of the code duplication.

david-yz-liu · 2025-04-05T14:21:07Z

app/models/assignment.rb

+                  raise 'A group name was not provided'
+                end
+              else
+                existing_group = self.course.groups.find_by(group_name: new_group_name)


you don't need to change this code from the original version

david-yz-liu · 2025-04-05T14:22:50Z

app/models/assignment.rb

+
+      group.save!
+
+      if groupings.exists?(group_id: group.id)


In general, prefer using explicit self for all attributes/methods

david-yz-liu · 2025-04-05T14:23:21Z

app/models/assignment.rb

+      grouping = Grouping.create!(group: group, assignment: self)
+
+      unless members.empty?
+        students = Student.joins(:user)


use self.course.students (need to ensure only students from the current course are queried)

david-yz-liu · 2025-04-05T14:26:22Z

app/models/assignment.rb

+      unless members.empty?
+        students = Student.joins(:user)
+                          .where(users: { user_name: members })
+                          .index_by { |s| s.user.user_name }


This is a good time to use pluck, as you really only need the user name and id here

david-yz-liu · 2025-04-05T14:27:08Z

app/models/assignment.rb

+
+          grouping.student_memberships.create!(
+            role_id: student.id,
+            membership_status: if index.zero?


I have the same stylistic comment as above. However, in this case you should be able to fit this on one line if you apply the ternary expression to just :inviter or :accepted, and not the other parts

david-yz-liu · 2025-04-05T14:28:21Z

spec/models/assignment_spec.rb

+      end
+    end
+
+    context 'when group already exists in assignment' do


reword to "when group already has a grouping for assignment"

david-yz-liu · 2025-04-05T14:30:22Z

spec/models/assignment_spec.rb

+    let(:student2) { create(:student, user: user2, course: course) }
+
+    shared_examples 'group persistence and members' do |expected_member_count: 0|
+      it 'creates persistent group and grouping' do


I would split this into two assertions, one for the first assertion and one for the third assertion (the second is just testing an association so I don't think it's necessary). Also for the third assertion I would use find_by rather than where

david-yz-liu · 2025-04-05T14:31:03Z

spec/models/assignment_spec.rb

+        if expected_member_count > 0
+          expect(grouping.student_memberships.first.membership_status).to eq(StudentMembership::STATUSES[:inviter])
+          if expected_member_count > 1
+            expect(grouping.student_memberships.last.membership_status).to eq(StudentMembership::STATUSES[:accepted])


Testing just last is a bit strange; it's more precise to use a loop for the rest.

I also recommend adding an additional test case for having more than two members.

…-UofT/Markus into group-name-generation

david-yz-liu · 2025-04-06T22:38:24Z

Thank you, @Raine-Yang-UofT!

Raine-Yang-UofT added 4 commits March 23, 2025 23:03

feat: initial implementation of add_group_api

d16551d

refactor: optimize add_group_api

c6b7698

Resolve N+1 query issue in creating members Wrap group creation in transaction block

Merge branch 'master' of https://github.com/MarkUsProject/Markus into…

fef3a53

… group-name-generation

test: implement test cases

59e864c

Raine-Yang-UofT mentioned this pull request Apr 4, 2025

Add attribute members to add group API MarkUsProject/Wiki#231

Merged

docs: update change log

3c727be

Raine-Yang-UofT requested a review from david-yz-liu April 4, 2025 17:39

Merge branch 'master' of https://github.com/MarkUsProject/Markus into…

8bb8c7a

… group-name-generation

Merge branch 'master' into group-name-generation

24ad790

david-yz-liu reviewed Apr 5, 2025

View reviewed changes

Raine-Yang-UofT added 3 commits April 5, 2025 17:00

refactor: refactor code implementation

3a90318

test: update test cases

282dd98

Merge branch 'group-name-generation' of https://github.com/Raine-Yang…

da11793

…-UofT/Markus into group-name-generation

Raine-Yang-UofT requested a review from david-yz-liu April 5, 2025 21:33

Merge branch 'master' into group-name-generation

5448333

david-yz-liu approved these changes Apr 6, 2025

View reviewed changes

david-yz-liu merged commit fe3d3c6 into MarkUsProject:master Apr 6, 2025
6 checks passed

Use username as group name when creating individual groups via API #7481

Use username as group name when creating individual groups via API #7481

Uh oh!

Conversation

Raine-Yang-UofT commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Type of Change

Checklist

Questions and Comments

Uh oh!

coveralls commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 14286000465

Details

💛 - Coveralls

Uh oh!

david-yz-liu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

david-yz-liu commented Apr 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Raine-Yang-UofT commented Apr 4, 2025 •

edited

Loading

coveralls commented Apr 4, 2025 •

edited

Loading