[Bugfix:TAGrading] Progress bar show correct percentage#10272
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10272 +/- ##
=============================================
- Coverage 38.91% 22.76% -16.16%
- Complexity 0 8374 +8374
=============================================
Files 36 228 +192
Lines 3045 29985 +26940
Branches 75 75
=============================================
+ Hits 1185 6825 +5640
- Misses 1788 23088 +21300
Partials 72 72
Flags with carried forward coverage won't be shown. Click here to find out more. |
wuj18
changed the title
JWei17
left a comment
JWei17
left a comment
There was a problem hiding this comment.
Approve, the peer grading gradable display the correct percentage. The progress bar no longer dropped down when ungraded some students homework.
|
|
||
| public function getTotalComponentCount($g_id) { | ||
| $this->course_db->query("SELECT count(*) AS cnt FROM gradeable_component WHERE g_id=?", [$g_id]); | ||
| $this->course_db->query("SELECT count(*) AS cnt FROM gradeable_component WHERE g_id=? AND gc_is_peer=?", [$g_id, $this->course_db->convertBoolean(false)]); |
There was a problem hiding this comment.
It looks like this change makes it so that getTotalComponentCount only returns the non-peer components. Wouldn't a better fix be to include peer graded components in the %complete calculation?
There was a problem hiding this comment.
Not really, because the progress bar for the TA side should be calculated using the TA-completed components/TA-only components as stated in the expected behavior for the issue. The progress bar for the peer side should be calculated through peer-completed components/peer components, but Nia has another PR that addresses the peer progress bar, so this PR is specifically aimed at resolving the issues with the TA progress bar.
There was a problem hiding this comment.
That makes sense. I still feel that as long as getTotalComponentCount is in the codebase, it should return all components. Can you create a new function (or, if it wouldn't break anything else, rename this one) named getTaComponentCount or similar to make it clear when you only want TA components as opposed all components?
There was a problem hiding this comment.
Thanks for the feedback. I agree that the getTotalComponentCount() should only return all components. I created a new function named getTaComponentCount()
DarthNyan
left a comment
DarthNyan
left a comment
There was a problem hiding this comment.
I like the renaming that you did I think it makes things clearer, and I may have a bit more for you. Is getGradedComponentsCountByGradingSections is ta-specific? If so I think it should be renamed as such. I also think getGradingProgress should be renamed for clarity if it's specifically ta grading progress.
DarthNyan
left a comment
DarthNyan
left a comment
There was a problem hiding this comment.
Looks good to me. I think it's a lot more clear now exactly what's going on in the progress bar calculation.
williamjallen
left a comment
williamjallen
left a comment
There was a problem hiding this comment.
A couple brief stylistic comments:
Co-authored-by: William Allen <[email protected]>
Co-authored-by: William Allen <[email protected]>
williamjallen
left a comment
williamjallen
left a comment
There was a problem hiding this comment.
My requested changes have been made. I'm not very familiar with this part of the codebase, so it would be good for someone else to take a look as well.
5 participants
Please check if the PR fulfills these requirements:
What is the current behavior?
Fixes #10061
When peer grading components are included in TA grading, the progress bar fails to display the correct completion percentage.
What is the new behavior?
The TA grading status progress bar accurately shows the completion percentage of non-peer components assigned to the grader (instructor, TA, or limited access grader).
Other information?
Test with an assignment that involves peer grading. Verify if the percentage displayed on the progress bar matches the expected percentage.