Improved dashboard querying for TA data in overview graph

[hemmatio]

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

Currently (especially with a large amount of graders), rendering the graphs in the dashboard view for an assignment is inefficient. This is due to a number of inefficient queries made obtaining completion counts.

• Cached versions of existing functions:
  • def get_num_marked_from_cache(assignment)
  • def get_num_assigned_from_cache(assignment)
• These functions take advantage of cached data by the newmarked_result_ids_for(assignment) function.
• In the controller, the TA grade distribution code now calls these cached versions instead of the previous get_num_marked and get_num_assigned methods. This reduces the number of queries made per TA.

Screenshots of your changes (if applicable)

Before: 4 queries per TA



After: 1 query per TA

Associated documentation repository pull request (if applicable)

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)
🐛 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)	X
🚦 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.)
02/06/25: This is still WIP. Current "fix" is very shoddy.
02/09/25: Correctly uses assignment id to query data. Only one query is created to obtain all results associated with the TA.
02/17/25: Added testcases. One potential thing to note: The cached versions of the functions cannot be used interchangeably with their non-cached versions. This is the main reason why I added them to the ta class, to separate them to be for their very specific ta-defined usages.

[coveralls]

Pull Request Test Coverage Report for Build 13381292272

Details

• 60 of 60 (100.0%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.01%) to 91.873%

---

Totals
Change from base Build 13293140801:	0.01%
Covered Lines:	41460
Relevant Lines:	44445

---

💛 - Coveralls

[david-yz-liu]

So overall you have a good idea, but this should really be an assignment method. This is because the true N+1 query is the fact that there's one query made per TA, and this method does not help address that problem.

You can instead write an assignment method that uses the same instance variable caching idea, but stores a hash mapping TA id to the two lists (what you currently call total_results and marked_result_ids). As a tip, you can first use pluck to extract just the columns you want (ta id, result id, and marking state) and then Enumerable#group_by.

Finally, you made a good comment about not wanting to change the behaviour of the existing assignment methods. What you can do is add to it an optional argument (say, bulk: false) which, when true, will run the query to get all TAs and set the instance attribute. But when it's false you can default to just using the original code.

Finally, you should look closely at how Assignment#get_num_marked currently works; it has a separate case for is_criteria_mark?. This part has different logic for considering which results a TA has completed marking, based on the criteria the TA has been assigned rather than the result marking state.