Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved dashboard querying for TA data in overview graph #7420

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
1 change: 1 addition & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
- Update Python version to 3.13 in seed autotest schemas (#7388)
- Rename jupyter notebook content functions and files to generalize to html content (#7391)
- Update to React v18 (#7392)
- Fix inefficient TA querying in dashboard graphs (#7420)

## [v2.6.1]

Expand Down
7 changes: 4 additions & 3 deletions app/controllers/assignments_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -374,11 +374,12 @@ def grade_distribution
grade_distribution = { labels: assignment_labels, datasets: assignment_datasets }
ta_labels = (0..intervals - 1).map { |i| "#{5 * i}-#{5 * i + 5}" }
ta_datasets = assignment.tas.map do |ta|
grade_distribution_arr = ta.grade_distribution_array(assignment, intervals)
num_marked_label = t('submissions.how_many_marked',
num_marked: assignment.get_num_marked(ta.id),
num_assigned: assignment.get_num_assigned(ta.id))
num_marked: ta.get_num_marked_from_cache(assignment),
num_assigned: ta.get_num_assigned_from_cache(assignment))
{ label: "#{ta.display_name} (#{num_marked_label})",
data: ta.grade_distribution_array(assignment, intervals) }
data: grade_distribution_arr }
end
json_data = {
summary: summary,
Expand Down
27 changes: 23 additions & 4 deletions app/models/ta.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,25 @@ def get_groupings_by_assignment(assignment)
.includes(:students, :tas, :group, :assignment)
end

def marked_result_ids_for(assignment)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@total_results ||= {}
@total_results[assignment.id] ||= assignment.current_results
.joins(grouping: :tas)
.where('roles.id': self.id).to_a
@marked_result_ids ||= {}
@marked_result_ids[assignment.id] ||= @total_results[assignment.id]
.select do |result|
result.marking_state == Result::MARKING_STATES[:complete]
end
.map(&:id)
[@total_results, @marked_result_ids]
end

# An array of all the grades for an assignment for this TA.
# If TAs are assigned to grade criteria, returns just the subtotal
# for the criteria the TA was assigned.
def percentage_grades_array(assignment)
result_ids = assignment.current_results
.joins(grouping: :tas)
.where(marking_state: Result::MARKING_STATES[:complete], 'roles.id': self.id)
.ids
result_ids = marked_result_ids_for(assignment)[1][assignment.id]

if assignment.assign_graders_to_criteria
criterion_ids = self.criterion_ta_associations.where(assessment_id: assignment.id).pluck(:criterion_id)
Expand Down Expand Up @@ -61,6 +72,14 @@ def grade_distribution_array(assignment, intervals = 20)
distribution
end

def get_num_marked_from_cache(assignment)
marked_result_ids_for(assignment)[1][assignment.id].size
end

def get_num_assigned_from_cache(assignment)
marked_result_ids_for(assignment)[0][assignment.id].size
end

private

def create_grader_permission
Expand Down
77 changes: 77 additions & 0 deletions spec/models/ta_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,83 @@
end
end

describe '#get_num_marked_from_cache' do
let(:instructor) { create(:instructor) }
let(:ta) { create(:ta) }
let(:assignment) { create(:assignment) }
let(:grouping1) { create(:grouping_with_inviter_and_submission, assignment: assignment, is_collected: true) }
let(:grouping2) { create(:grouping_with_inviter_and_submission, assignment: assignment, is_collected: true) }

before do
create(:complete_result, submission: grouping1.submissions.first)
create(:incomplete_result, submission: grouping2.submissions.first)
create(:ta_membership, role: ta, grouping: grouping1)
create(:ta_membership, role: ta, grouping: grouping2)
end

context 'When user is a TA' do
it 'should return the number of marked submissions for groupings associated to them' do
expect(ta.get_num_marked_from_cache(assignment)).to eq(assignment.get_num_marked(ta.id))
end

context 'when they are assigned a remark request that is incomplete' do
before do
create(:remark_result,
submission: grouping1.submissions.first,
marking_state: Result::MARKING_STATES[:incomplete])
end

it 'does not count the remark request as marked' do
expect(ta.get_num_marked_from_cache(assignment)).to eq(0)
end
end

context 'when they are assigned a remark request that is complete' do
before do
create(:remark_result,
submission: grouping1.submissions.first,
marking_state: Result::MARKING_STATES[:complete])
end

it 'counts the remark request as marked' do
expect(ta.get_num_marked_from_cache(assignment)).to eq(1)
end
end
end
end

describe '#get_num_assigned_from_cache' do
let(:instructor) { create(:instructor) }
let(:ta) { create(:ta) }
let(:ta2) { create(:ta) }
let(:assignment) { create(:assignment) }
let(:grouping1) { create(:grouping_with_inviter_and_submission, assignment: assignment, is_collected: true) }
let(:grouping2) { create(:grouping_with_inviter_and_submission, assignment: assignment, is_collected: true) }
let(:grouping3) { create(:grouping_with_inviter_and_submission, assignment: assignment, is_collected: true) }
let(:grouping4) { create(:grouping_with_inviter_and_submission, assignment: assignment, is_collected: false) }

before do
create(:complete_result, submission: grouping1.submissions.first)
create(:incomplete_result, submission: grouping2.submissions.first)
create(:complete_result, submission: grouping3.submissions.first)
create(:incomplete_result, submission: grouping4.submissions.first)
create(:ta_membership, role: ta, grouping: grouping1)
create(:ta_membership, role: ta, grouping: grouping2)
create(:ta_membership, role: ta2, grouping: grouping3)
create(:ta_membership, role: ta2, grouping: grouping4)
end

context 'When user is a TA' do
it 'should return the number of submissions assigned to them' do
expect(ta.get_num_assigned_from_cache(assignment)).to eq(assignment.get_num_assigned(ta.id))
end

it 'should not count submissions assigned to another ta' do
expect(ta.get_num_assigned_from_cache(assignment)).to eq(2)
end
end
end

context 'Associated grader permission validation' do
subject { create(:ta) }

Expand Down