Optimize Empty Submission Querying in graders_controller #7381
Conversation
hemmatio
changed the title
Pull Request Test Coverage Report for Build 12960952565Details
💛 - Coveralls |
There was a problem hiding this comment.
Thanks @hemmatio, I left some inline comments; also please do a merge from master to update this branch.
| end | ||
| submissions = Submission.where(grouping_id: grouping_ids, submission_version_used: true) | ||
| .pluck(:grouping_id, :is_empty) | ||
| non_empty_grouping_ids = submissions.reject { |_, is_empty| is_empty }.map { |grouping_id, _| grouping_id.to_s } |
There was a problem hiding this comment.
The reject is awkward here; instead, you can move the is_empty check into the .where method above.
| submissions = Submission.where(grouping_id: grouping_ids, submission_version_used: true) | ||
| .pluck(:grouping_id, :is_empty) | ||
| non_empty_grouping_ids = submissions.reject { |_, is_empty| is_empty }.map { |grouping_id, _| grouping_id.to_s } | ||
| non_empty_grouping_ids & grouping_ids |
There was a problem hiding this comment.
The & grouping_ids isn't necessary, the where filter should only include grouping ids that were included in the original grouping_ids argument
|
I'm seeing that the coverage decreased. I believe this is because I decreased the total number of lines, so the overall percentage is lower. |
There was a problem hiding this comment.
@hemmatio nice work! I left one inline comment; don't worry about the coverage decrease, I suspect you are correct.
| submission && !submission.is_empty | ||
| end | ||
| Submission.where(grouping_id: grouping_ids, is_empty: false, submission_version_used: true) | ||
| .pluck(:grouping_id, :is_empty) |
There was a problem hiding this comment.
You should only pluck grouping_id here (not sure why the tests didn't catch this, perhaps you could look into that)
There was a problem hiding this comment.
The call for filter_empty_submissions only compares the count before & after. The number of columns didn't affect the count, which is why no tests failed.
filtered_grouping_ids = filter_empty_submissions(grouping_ids)
if filtered_grouping_ids.count != grouping_ids.count
found_empty_submission = true
end
There was a problem hiding this comment.
@hemmatio this isn't accurate, the filtered_grouping_ids variable is used a bit further down in Line 137. If that branch is not currently being tested, you should add test cases for it
… assign-grader-querying
… assign-grader-querying
| skip_empty_submissions: 'true' | ||
| } | ||
|
|
||
| filtered_grouping_ids = GradersController.new.__send__(:filter_empty_submissions, groupings) |
There was a problem hiding this comment.
It's not necessary to call this method directly, especially since it's private. If you're trying to assessing that the correct branch of code was executed, you can check the contents of the flash.
…pty_submission is true
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.)
When "Assign graders to groups with no files submitted" is unselected when assigning graders to groups, MarkUs checks for empty submissions. This PR refactors the filter_empty_submissions method in graders_controller.rb to resolve an N+1 query issue when assigning graders to many submissions. The current implementation performs individual database queries for each grouping to check if a submission is empty.
Fix: Refactored the logic to use a single query that fetches all relevant grouping IDs and their submission statuses, significantly reducing redundant queries.
...
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.)
12/01/25: I don't see any necessity for further tests as the ones in
graders_controller_spec.rbare quite comprehensive, including cases testingskip_empty_submissions.23/01/25: Created test case to confirm correct return of filter_empty_submissions