Enable removal of a student from a course

[donny-wong]

Proposed Changes

Bulk uploading students to a course with an incorrect CSV column order results in incorrect students being created. This is one reason why this feature was implemented, which is, enabling the deletion of a single student from a course.

In order to allow the deletion of a student, they would need to not be associated to any groups, meaning there should not be any records in the memberships table for this student for the course they are being deleted from.

...

Screenshots of your changes (if applicable)

For testing, I created a new course with a new student: I then associated this student to a group: I then tried to delete the student, but got the following error: I then disassociated the student from the group and was able to delete the student:

Associated documentation repository pull request (if applicable)

Instructions to remove a student: https://github.com/MarkUsProject/Wiki/pull/232

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

[coveralls]

Pull Request Test Coverage Report for Build 14629824156

Details

• 85 of 87 (97.7%) changed or added relevant lines in 12 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.01%) to 91.92%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
app/javascript/Components/student_table.jsx	6	7	85.71%
app/javascript/Components/ta_table.jsx	0	1	0.0%

Totals
Change from base Build 14629035342:	0.01%
Covered Lines:	41711
Relevant Lines:	44693

---

💛 - Coveralls

[david-yz-liu]

@donny-wong excellent work! This PR is good, I just left a few stylistic comments.

Please also make an update to the MarkUs wiki.

[david-yz-liu]

@donny-wong make sure to link the Wiki PR to this one (there's a section for doing so in the PR description).

Also I realized that there should be one other restriction on student deletion: if there are associated grade_entry_students, and the grade_entry_student has an associated grade with a non-nil grade, then we should also prevent deletion. I expect the easiest way to implement this is to set the grade_entry_student association to dependent: :destroy and then in the grade entry student model, a check for non-nil grades.

[donny-wong]

@donny-wong make sure to link the Wiki PR to this one (there's a section for doing so in the PR description).

Also I realized that there should be one other restriction on student deletion: if there are associated grade_entry_students, and the grade_entry_student has an associated grade with a non-nil grade, then we should also prevent deletion. I expect the easiest way to implement this is to set the grade_entry_student association to dependent: :destroy and then in the grade entry student model, a check for non-nil grades.

@david-yz-liu, I am not able to delete the student role, regardless of having dependent: :destroy, as it still raises the following error on the DB level:

ActiveRecord::NotNullViolation - PG::NotNullViolation: ERROR:  null value in column "role_id" of relation "grade_entry_students" violates not-null constraint
2025-04-10T15:08:09.662828042Z DETAIL:  Failing row contains (450, f, 2025-04-10 15:06:43.54127, 2025-04-10 15:06:43.54127, 21, null).:

I would like to get your thoughts if it would be ok to remove the DB foreign key constraint of role_id in the grade_entry_students table, which would then allow me to proceed with the deletion and use dependent: :destroy. I would create a migration for this foreign constraint removal, and add the foreign key constraint on the Model level. I'm not sure if there is another way to go about this? Thank you.

[david-yz-liu]

@donny-wong you don't need to remove the foreign key constraint. The point of doing the destroy is that the associated records (i.e., grade entry forms) should be deleted as well. If this is not happening there may be something else going on that you need to investigate and fix.

[donny-wong]

@donny-wong you don't need to remove the foreign key constraint. The point of doing the destroy is that the associated records (i.e., grade entry forms) should be deleted as well. If this is not happening there may be something else going on that you need to investigate and fix.

@david-yz-liu , you are correct. I was putting the dependent: :destroy in the parent role instead of the student subclass, which was overwriting it. It all makes sense now. Thank you!

[david-yz-liu]

@donny-wong please add a test case for when there is a grade entry form (and GradeEntryStudent) but all of the grades are nil.

Also I'm not sure where in the code you are checking for non-nil grades before deletion?

[donny-wong]

@donny-wong please add a test case for when there is a grade entry form (and GradeEntryStudent) but all of the grades are nil.

Also I'm not sure where in the code you are checking for non-nil grades before deletion?

@david-yz-liu I am checking for non-nil grades before deleting in app/models/grade_entry_student.rb line 27

[david-yz-liu]

@donny-wong ah yes I missed that (line 27), thanks for clarifying 👍