association-improvement #6989

mimischly7 · 2024-03-01T21:44:52Z

Added dependent: :destroy to all the associations that needed it (in the Assignment and Assessment models), changed the order of associations in Assignment to make groups-and-groupings first (this is the first thing that should be checked when deleting an assignment), created new locale string for use in the AnnotationCategory and Assignment models, and added another rescue block in the #destroy action of assignments controller to handle other potential complications that could arise in assignment deletion. This last one is temporary, since we would ideally like to perform a more targetted handling of different exceptions (but for now this prevents crashing and gives an idea of the problem).

Also, wrote thorough RSPEC tests for assignments_controller to ensure that, upon assignment destruction, all associated entities are also destroyed appropriately, and also tested for the case that (the attempt of) destroying an Assignment raises a StandardError.

Motivation and Context

Your Changes

Description:

Type of change (select all that apply):

Bug fix (non-breaking change which fixes an issue)

Testing

RSPEC (in annotations_controller_spec)

Questions and Comments (if applicable)

Checklist

I have performed a self-review of my own code.
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 on Coveralls.
I have added tests for my changes.
I have updated the Changelog.md file.

Pull request to make documentation changes (if applicable)

coveralls · 2024-03-15T20:52:14Z

Pull Request Test Coverage Report for Build 8486266995

Details

35 of 35 (100.0%) changed or added relevant lines in 6 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 91.993%

Totals
Change from base Build 8485710953:	0.2%
Covered Lines:	39630
Relevant Lines:	42456

💛 - Coveralls

david-yz-liu · 2024-03-16T16:56:00Z

app/models/assignment.rb

+ has_many :current_submissions_used,
+ through: :groupings,
+ source: :current_submission_used,
+ dependent: :destroy


Don't add this to through associations (it's either redundant or misleading: in this case, for example, groupings can't be destroyed, and so current_submissions_used will also never be destroyed)

david-yz-liu · 2024-03-16T16:56:32Z

spec/controllers/assignments_controller_spec.rb

@@ -2065,5 +2075,28 @@
 expect(flash.to_hash.length).to eq(1)
 expect(response).to have_http_status(302)
 end
+ it 'should remove associated entities on destroy' do
+ # These were lazily initialized, so in order to actually be created we need to "use" them
+ checkbox_criterion


Create a separate test case for each of these

david-yz-liu · 2024-03-16T16:57:32Z

app/controllers/assignments_controller.rb

@@ -667,6 +667,9 @@ def destroy
 rescue ActiveRecord::DeleteRestrictionError
 flash_message(:error, I18n.t('assignments.assignment_has_groupings'))
 redirect_back fallback_location: { action: :edit, id: @assignment.id }
+ rescue StandardError => e
+ flash_message(:error, "Problem with deletion: #{e.message}")


make sure to add a test case that covers these two lines of code

Also, make sure to internationalize this string

david-yz-liu · 2024-03-23T02:09:11Z

app/models/assignment.rb


 has_many :notes, as: :noteable, dependent: :destroy

 has_many :exam_templates, dependent: :destroy, inverse_of: :assignment, foreign_key: :assessment_id

 has_many :starter_file_groups, dependent: :destroy, inverse_of: :assignment, foreign_key: :assessment_id

- has_many :tas, -> { distinct }, through: :ta_memberships, source: :role
+ has_many :tas, -> { distinct }, through: :ta_memberships, source: :role, dependent: :destroy


There shouldn't be a dependent: :destroy here

david-yz-liu · 2024-03-23T02:09:35Z

app/models/assignment.rb

 has_many :peer_reviews, through: :groupings
 has_many :pr_peer_reviews, through: :parent_assignment, source: :peer_reviews

- has_many :current_submissions_used, through: :groupings,
- source: :current_submission_used
+ has_many :current_submissions_used,


you can revert the changes here (they are now purely stylistic since you removed the dependent: :destroy

david-yz-liu · 2024-03-23T02:11:13Z

spec/controllers/assignments_controller_spec.rb

+ # Deleting the assignment - should be successful since there are not groupings
+ delete_as instructor, :destroy, params: { course_id: course.id, id: assignment.id }
+ expect(Assignment.exists?(assignment.id)).to be(false)
+ # Ensure that the associated checkbox_criterion was also removed


"checkbox_criterion" -> "entity"

david-yz-liu · 2024-03-23T02:11:28Z

spec/controllers/assignments_controller_spec.rb

+
+ shared_examples 'handling associated entities upon destroy' do |entity|
+ it "should remove associated #{entity}" do
+ # NOTE: the next line assume that an `assignment` is sufficient for the factory of `entity`


"assume" -> "assumes"

david-yz-liu · 2024-03-23T02:12:09Z

spec/controllers/assignments_controller_spec.rb

+ it "should remove associated #{entity}" do
+ # NOTE: the next line assume that an `assignment` is sufficient for the factory of `entity`
+ assoc_entity = create entity, assignment: assignment
+ # Deleting the assignment - should be successful since there are not groupings


"not" -> "no"

Added dependent: :destroy to all the associations that needed it (in the Assignment and Assessment models), changed the order of associations in Assignment to make groups-and-groupings first (this is the first thing that should be checked when deleting an assignment), created a new locale string for use in the AnnotationCategory model, and added another rescue block in the #destroy action of assignments controller to handle other potential complications that could arise in assignment deletion. This last one is temporary, since we would ideally like to perform a more targetted handling of different exceptions (but for now this prevents crashing and gives an idea of the problem).

(1) An rspec test for the assignments controller that, upon assignment destruction, all associated entities are also destroyed appropriately, and (2) add a dependent: :destroy to the template_divisions association in the AssignmentFile model

Remove options from indirect associations (i.e. associations) because they are redundant/misleading

DRY up the tests by introducing a shared example. Test the case where destroying an assignment leads to a StandardError. Internationalize strings.

…associations

mimischly7 force-pushed the improve-assignment-associations branch from 3fdde26 to f7308c6 Compare March 15, 2024 16:34

mimischly7 requested a review from david-yz-liu March 15, 2024 20:21

mimischly7 marked this pull request as ready for review March 15, 2024 20:28

david-yz-liu reviewed Mar 16, 2024

View reviewed changes

mimischly7 requested a review from david-yz-liu March 22, 2024 04:14

david-yz-liu reviewed Mar 23, 2024

View reviewed changes

mimischly7 added 5 commits March 28, 2024 22:07

Rspec test for assignment deletion

cc7e03f

(1) An rspec test for the assignments controller that, upon assignment destruction, all associated entities are also destroyed appropriately, and (2) add a dependent: :destroy to the template_divisions association in the AssignmentFile model

Improve assignment associations

f5aeeb9

Remove options from indirect associations (i.e. associations) because they are redundant/misleading

Improve testing of assignment#destroy

89d35fc

DRY up the tests by introducing a shared example. Test the case where destroying an assignment leads to a StandardError. Internationalize strings.

Small corrections

441d3c8

mimischly7 force-pushed the improve-assignment-associations branch from a68ce2a to 441d3c8 Compare March 29, 2024 02:41

Update Changelog

33f69ee

mimischly7 requested a review from david-yz-liu March 29, 2024 06:26

Merge remote-tracking branch 'origin/master' into improve-assignment-…

93d0877

…associations

david-yz-liu approved these changes Mar 30, 2024

View reviewed changes

david-yz-liu merged commit 5889df5 into MarkUsProject:master Mar 30, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

association-improvement #6989

association-improvement #6989

mimischly7 commented Mar 1, 2024 •

edited

Loading

coveralls commented Mar 15, 2024 •

edited

Loading

david-yz-liu Mar 16, 2024

david-yz-liu Mar 16, 2024

david-yz-liu Mar 16, 2024

david-yz-liu Mar 23, 2024

david-yz-liu Mar 23, 2024

david-yz-liu Mar 23, 2024

david-yz-liu Mar 23, 2024

david-yz-liu Mar 23, 2024

association-improvement #6989

association-improvement #6989

Conversation

mimischly7 commented Mar 1, 2024 • edited Loading

Motivation and Context

Your Changes

Testing

Questions and Comments (if applicable)

Checklist

Pull request to make documentation changes (if applicable)

coveralls commented Mar 15, 2024 • edited Loading

Pull Request Test Coverage Report for Build 8486266995

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimischly7 commented Mar 1, 2024 •

edited

Loading

coveralls commented Mar 15, 2024 •

edited

Loading