CTP-6546: Fix logic around marker name/mark being shown when using sampling. - #305
CTP-6546: Fix logic around marker name/mark being shown when using sampling.#305cwarwicker wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the grading report so assessors can see other assessors’ names when sampling applies to a student, and adds an automated Behat scenario to validate the behaviour.
Changes:
- Added sampling-aware visibility logic in the grading report “marking cell” so assessor names can be revealed for sampled allocatables.
- Added a new Behat feature covering “not in sample” vs “in sample” name visibility.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/behat/sampling_view_names.feature | Adds Behat coverage for assessor-name visibility with/without sampling inclusion. |
| classes/render_helpers/grading_report/data/marking_cell_data.php | Adjusts name-visibility logic for markers based on sampling state and membership. |
Comments suppressed due to low confidence (2)
tests/behat/sampling_view_names.feature:37
- This scenario is labelled "Without sampling" but sampling is enabled in the Background; the scenario is really asserting behaviour when the allocatable is not included in the sample. Renaming it will make the intent clearer.
Scenario: Without sampling, assessor should not see other assessor's names
tests/behat/sampling_view_names.feature:42
- The scenario name mentions "2 marker feedbacks", but the Background creates only a single assessor_1 feedback. Renaming avoids confusion when maintaining the test.
Scenario: Submissions with 2 marker feedbacks and sampling allocations created
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
classes/render_helpers/grading_report/data/marking_cell_data.php:120
assessment_set_membership::exists([...])uses$rowsbase->get_submission()->allocatabletype, butgrading_table_row_base::get_submission()can returnnull(e.g., students with no submission). This will throw when sampling is enabled and the other visibility conditions are false. Also, the sample-membership check is evaluated inside the marker loop even though it only depends on the allocatable, causing repeated DB work.
&& assessment_set_membership::exists([
'courseworkid' => $this->coursework->id,
'allocatabletype' => $rowsbase->get_submission()->allocatabletype,
'allocatableid' => $rowsbase->get_allocatable_id(),
])
tests/behat/sampling_view_names.feature:31
- This scenario title is misleading: sampling is enabled in the Background; what changes between scenarios is whether the allocatable is included in the sample. Also consider using the plural possessive consistently ("assessors'").
Scenario: Without sampling, assessor should not see other assessor's names
tests/behat/sampling_view_names.feature:36
- This scenario title mentions "2 marker feedbacks", but the Background only creates a single feedback (teacher1 / assessor_1). Renaming the scenario to match the setup makes the test intent clearer.
Scenario: Submissions with 2 marker feedbacks and sampling allocations created
|
Ready for review/testing. However, I’m not entirely sure the logic is correct. Needs someone to check it. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
classes/models/assessment_set_membership.php:150
membership_count()no longer caches a zero result. This avoids the stale-0 issue when new sample-set memberships are created viaassessment_set_membership->save()(e.g.classes/stages/base.php:519-525), but it also means the “called over and over during rendering of the grading page” path will now execute aCOUNT(*)query every time the allocatable is not in the sample. That can reintroduce an N+1 query pattern and hurt grading-report performance, especially when most students are not sampled.
Prefer fixing the cache invalidation on membership creation (so that caching 0 is safe), e.g. explicitly deleting this allocatable’s cache key when saving a new membership, rather than disabling caching for the most common result.
if ($cachedvalue > 0) {
$cache->set($cachekey, $cachedvalue);
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (5)
classes/ability.php:1266
- The new sampling-based
showrule returns true without any check that the current user is an assessor/marker on the coursework. Becausefeedback::can_show()is used for visibility (including mark visibility and the “show” link), this risks granting feedback visibility to unintended users (e.g. students) whenever any assessor_2/assessor_3 feedback exists.
if ($this->get_coursework()->sampling_enabled() && $feedback->get_submission()->stage_feedback_exists([
'assessor_2',
'assessor_3',
])) {
return true;
classes/ability.php:1257
- This new permission path is security-sensitive (it changes who can
showfeedback). There are PHPUnit tests for the ability mechanism, but none here verifying the sampling rule only permits assessors and does not allow students/other users to see feedback when sampling stages are marked.
This issue also appears on line 1262 of the same file.
private function allow_show_feedback_if_sampling_enabled_and_sample_marked(): void {
$this->allow(
'show',
'mod_coursework\models\feedback',
function (feedback $feedback) {
classes/models/submission.php:1254
- The PHPDoc says “a named stage”, but the method accepts an array of stage identifiers and an optional
$finalisedfilter. This mismatch makes the API easy to misuse (especially since it’s used for permission decisions).
/**
* Check that feedback exists on this submission for a named stage.
* @return bool
*/
public function stage_feedback_exists(array $stageidentifiers, bool $finalised = true): bool {
classes/models/submission.php:1258
stage_feedback_exists()is now part of the feedback visibility rules (permissions). There are existing PHPUnit tests for the submission model, but no coverage ensuring this helper behaves correctly for finalised vs non-finalised feedback and multiple stage identifiers.
public function stage_feedback_exists(array $stageidentifiers, bool $finalised = true): bool {
$feedback = $this->get_feedbacks();
foreach ($feedback as $f) {
if (in_array($f->stageidentifier, $stageidentifiers) && (!$finalised || (bool)$f->finalised)) {
return true;
classes/models/assessment_set_membership.php:150
membership_count()now only caches counts > 0. For allocatables not in the sample set (likely the majority), this means every call will run the DB COUNT query, which can regress grading page/render performance despite the comment saying this is called “over and over”. Consider caching 0 as well and fixing invalidation on create/save paths that previously left a stale 0 in cache (e.g. delete the specific cache key in the relevant save path, not just via DB-derived key lists).
if ($cachedvalue > 0) {
$cache->set($cachekey, $cachedvalue);
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
classes/models/submission.php:1262
- The docblock says this checks feedback for “a named stage”, but the method accepts an array of stage identifiers and the block omits parameter documentation. Also, using strict in_array avoids accidental matches if stage identifiers ever become non-strings.
/**
* Check that feedback exists on this submission for a named stage.
* @return bool
*/
public function stage_feedback_exists(array $stageidentifiers, bool $finalised = true): bool {
$feedback = $this->get_feedbacks();
foreach ($feedback as $f) {
if (in_array($f->stageidentifier, $stageidentifiers) && (!$finalised || (bool)$f->finalised)) {
return true;
}
}
return false;
}
classes/models/assessment_set_membership.php:150
- Not caching a 0 membership count means allocatables not in the sample set will trigger a DB COUNT query every time membership_count() is called. On pages that render many rows, this can reintroduce significant query volume for the common 'not sampled' case.
if ($cachedvalue > 0) {
$cache->set($cachekey, $cachedvalue);
}
classes/ability.php:1271
- This PR changes the feedback visibility rules when sampling is enabled, but there is no automated test asserting the new behavior (e.g., markers can see other markers' names only for sampled submissions, and students still cannot see unpublished feedback). Adding a focused PHPUnit test for ability->can('show', feedback) or a Behat scenario would help prevent regressions.
/**
* If sampling is enabled, and the student is in the selected sample, and both the initial mark and the sample
* mark are completed, then we should be able to see it and the marker's details.
*/
private function allow_show_feedback_if_sampling_enabled_and_sample_marked(): void {
$this->allow(
'show',
'mod_coursework\models\feedback',
function (feedback $feedback) {
// If we're using sampling, and some sampled feedback has been given.
// This might be assessor_2 or assessor_3. If sampling is enabled and there exists feedback for either
// of those stages, then we assume the sampling feedback has been given. I don't think it matters
// which one specifically it is.
if ($this->get_coursework()->sampling_enabled() && $feedback->get_submission()->stage_feedback_exists([
'assessor_2',
'assessor_3',
])) {
return true;
}
return false;
}
);
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
classes/ability.php:1273
- The comment says visibility should be granted only when both the initial mark and the sample mark are completed, but the condition only checks for sampled stages (assessor_2/assessor_3). This can allow showing feedback/marker details even if assessor_1 is not finalised yet.
if ($this->get_coursework()->sampling_enabled() && $feedback->get_submission()->stage_feedback_exists([
'assessor_2',
'assessor_3',
])) {
return true;
classes/models/submission.php:1262
- stage_feedback_exists() accepts an array of stage identifiers, but the docblock implies a single stage and the in_array() check is non-strict. Using strict comparison avoids unexpected matches (and makes intent clearer) if stage identifiers ever contain numeric-like values.
public function stage_feedback_exists(array $stageidentifiers, bool $finalised = true): bool {
$feedback = $this->get_feedbacks();
foreach ($feedback as $f) {
if (in_array($f->stageidentifier, $stageidentifiers) && (!$finalised || (bool)$f->finalised)) {
return true;
classes/models/assessment_set_membership.php:150
- Not caching a 0 membership_count() avoids stale zeroes, but it also means allocatables not in the sample will hit the DB every time membership_count() is called (and this method is documented as being called repeatedly during grading page rendering). Consider keeping zero caching but fixing invalidation at the point the allocation/sample set is saved (e.g., purge this cache area or bump a revision key) to avoid a performance regression.
if ($cachedvalue > 0) {
$cache->set($cachekey, $cachedvalue);
}
|
Hi @andrewhancox - I need help on this one. The Jira ticket is here: |
| ); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
"Ready for review/testing. However, I'm not entirely sure the logic is correct. Needs someone to check it."
The rule is pretty good, but doesn't quite work. The comments from copilot are mostly true - the rule needs to evaluate how many marks are expected, not only whether one of second/third marker exists.
1
It's possible for a submission to be in a sample for both marker 2 and marker 3 - so if the initial allocation and marker 2 are both marked, markers 1 and 2 get revealed to marker 3.
2
When sampling is enabled, students with submissions not in the sample never get their names shown. They'll never have a marker 2 or marker 3.
(Eliots comment on the JIRA task is: 'Sampling Enabled, but student NOT in the sample - Show both')
3
Marking can happen in parallel. ( is_parallell() ) e.g. Marker 2 can be marked (and finalised) before Marker 1 (in draft but not yet finalised). At this point the not finalised marker 1 name/mark/comments become visible.
(Eliots comment on the JIRA task is: 'they can't see each others marks or names until both have submitted their mark as final (not draft).')
Note: submission.php, marking_cell_data.php and stages/base.php all have a line like this:
count(get_assessor_feedbacks()) >= max_number_of_feedbacks()
function max_number_of_feedbacks() already looks at sampling:
Sampling on: membership_count() + 1
Sampling off: get_max_markers()
So that's the correct path I'd reckon.
Something like this:
/**
* If every mark this student is expecting has been given and finalised, then we should be able
* to see it and the marker's details. Where sampling is enabled, a student expects a mark for the
* first stage plus any later stages they were sampled into, which can be fewer than the number of
* markers configured.
*/
Change example function name can do the general case: allow_show_feedback_once_all_initial_marking_is_complete()
| ) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Simplify the next block to something like this:
is_agreed_grade() is a funny one because it looks for final_agreed_1 sometimes and 'assessor_1' other times.
// Don't reveal an unfinalised agreed grade through this rule.
if ($feedback->is_agreed_grade() && !$feedback->finalised) {
return false;
}
return $feedback->get_submission()->all_initial_marking_complete();
There was a problem hiding this comment.
assessment_set_membership is a weird one
note how assessment_set_membership::create() works
Add these lines here to fix $cache->set() - but don't have if ($cachedvalue > 0) before it.
$cache = cache::make('mod_coursework', self::CACHE_AREA_MEMBER_COUNT);
$cache->delete(
self::membership_count_cache_key($this->courseworkid, $this->allocatabletype, $this->allocatableid)
);
There was a problem hiding this comment.
See below comment. This comment is no longer relevant. Safe to delete this function.
| ) > 0; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
max_number_of_feedbacks() works out the sampling/not sampling issue for you.
Recommend replacing stage_feedback_exists with something like this:
/**
* Has every initial mark this submission expects been given and finalised?
* A sampled student expects a mark for the first stage plus any later stages they were sampled into.
* @return bool
*/
public function all_initial_marking_complete(): bool {
$feedbacks = $this->get_assessor_feedbacks();
if (count($feedbacks) < $this->max_number_of_feedbacks()) {
return false;
}
foreach ($feedbacks as $feedback) {
if (!$feedback->finalised) {
return false;
}
}
return true;
}
|
I think this is going to need another reviewer. Sorry! @ehoving viewinitialgrade off Sooo... what's supposed to happen here? Wait for agreed grade? Some of the behat tests add this capability in the tests, so I dunno. e.g. "When both markers have submitted their marks, Marker 1 and Marker 2 can see: Each other's marks. Each other's names associated with those marks." |
DavidUCL
left a comment
DavidUCL
left a comment
There was a problem hiding this comment.
Yeah...
assessment_set_membership cache sucks
It needs fixing now because half of @ehoving assertions in the JIRA ticket break when using the cache as is - and not using the cache means we're duplicating logic all over the place.
Sorry @cwarwicker - we've opened up a can of worms with this one.
The workarounds were kind of working - but the dodgey (existing) cache issue would break something eventually.
copilot was complaining about some real issues for once - but explaining them in a weird way (as normal)
i.e.
Should other markers see marker 1’s details?:
Sampling Enabled, but student NOT in the sample - Show both
Sampling enabled, student in sample, no mark 2 given yet - Don't show either
Sampling enabled, student in sample, marked by the second marker - Show both
Sampling enabled, marker specifically allocated to student, no mark given yet - Don't show
Sampling enaled, marked specifically allocated to student, mark given by second marker - Show both
|
Just double-posting this from JIRA so it doesn't get lost. Conn has some very handy findings on the JIRA ticket: First thing. Let’s remind ourselves why we are caching zeros - because that’s potentially a weird thing to do. In a coursework with 1000 course participants and 1000 submissions, lets say three markers with sampling on. The vast majority of the sample set cache is going to be zero for each submission. The savings in storing the zeros is actually useful - rather than hitting up the DB. Look at: sampled_feedback_exists() & max_number_of_feedbacks() Cache invalidation in coursework: assessment_set_membership is the weird outlier that does it differently currently - because the membership counts are cleared in pre_save_hook instead. The main problem is that pre_save_hook runs before the insert into coursework_sample_set_mbrs, and the membership_count_clear_cache_for_coursework function can only clear keys for rows that already exist. It’s trying to do that before it’s saved and no keys exist yet for the newly sampled submission - (if that submission doesn't already have a sample!). So when you include a student into a sample (for a submission that doesn't already have a sample), the row isn't there yet. It isn't in the result set, no key is built for it, and their cached zero persists through the cache reset in the save. So I reckon the fix is to move the membership_count_clear_cache_for_coursework() call out of pre_save_hook and into the post_save_hook() after the remove cache line. and delete pre_save_hook alltogether - I don’t know why pre_save_hook was trying to clear the cache before the parent hook in the first place. We completely agree on the diagnosis - zeros being in the cache that don’t refresh. Moving from pre_save_hook to post_save_hook is a minimal change. Option A fixes the issue, but stops caching zeros - which is actually really useful for us. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (4)
classes/ability.php:1275
- The condition here doesn’t match the docblock above it: it doesn’t check that the submission is in the selected sample, and the capability list omits
mod/coursework:addallocatedagreedgrade(used elsewhere for agreed graders). As written, this can grant/deny visibility inconsistently with othershowrules.
if (
$this->get_coursework()->sampling_enabled() && $feedback->get_submission()->stage_feedback_exists([
'assessor_2',
'assessor_3',
])
classes/render_helpers/grading_report/data/marking_cell_data.php:110
can_show()is being called with$feedback->get_coursework(), which internally callsget_submission()and can trigger extra lookups. In this file you already pass$this->courseworktocan_show()(e.g. line 187), so using it here avoids redundant work and keeps the context consistent.
if ($feedback = $row->get_feedback()) {
$canseeothermarkerdetails = $feedback->can_show($feedback->get_coursework(), $submission);
$marker = $this->create_marker_data($row->get_assessor(), $markernumber, $canseeothermarkerdetails);
classes/ability.php:1263
- This capability gate excludes users who only have
mod/coursework:addallocatedagreedgrade(which is treated as an agreed-grader capability elsewhere in this class). That can cause sampled-feedback visibility to differ from non-sampling visibility for the same role.
This issue also appears on line 1271 of the same file.
if (
!has_any_capability(
['mod/coursework:addinitialgrade', 'mod/coursework:addagreedgrade'],
$feedback->get_coursework()->get_context()
)
classes/models/assessment_set_membership.php:140
- The computed
$cachedvalueisn’t written back to the cache (the$cache->set(...)line was removed), so every miss will re-run the COUNT query even within the same page render. This defeats the purpose ofCACHE_AREA_MEMBER_COUNTand can add many queries on the grading report.
'allocatabletype' => $allocatabletype,
]
);
}
return $cachedvalue;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
classes/ability.php:1276
- The sampling visibility rule’s implementation doesn’t match its own docblock: it claims both the initial mark and the sample mark must be completed, but the code only checks for a finalised sampled-stage feedback (
assessor_2/assessor_3). This can allow showing feedback/marker details when a sampled-stage feedback exists but the initial-stage feedback is still missing or not finalised.
To align behavior with the stated rule, also require a finalised initial-stage feedback (typically assessor_1) before returning true.
// If we're using sampling, and some sampled feedback has been given.
// This might be assessor_2 or assessor_3. If sampling is enabled and there exists feedback for either
// of those stages, then we assume the sampling feedback has been given. I don't think it matters
// which one specifically it is.
if (
$this->get_coursework()->sampling_enabled() && $feedback->get_submission()->stage_feedback_exists([
'assessor_2',
'assessor_3',
])
) {
classes/render_helpers/grading_report/data/marking_cell_data.php:111
$feedback->can_show(...)is called twice for each feedback row: once to decide whether to expose marker details (line 109) and again insideprocess_feedback_data()(line 187). On grading report pages with many rows this adds avoidable ability checks and object creation overhead.
Consider computing $canshow once per feedback in get_table_cell_data() and reusing it for both create_marker_data() and process_feedback_data() (e.g., by passing it as a parameter or storing it on $marker).
if ($feedback = $row->get_feedback()) {
$canseeothermarkerdetails = $feedback->can_show($feedback->get_coursework(), $submission);
$marker = $this->create_marker_data($row->get_assessor(), $markernumber, $canseeothermarkerdetails);
$this->process_feedback_data($marker, $feedback, $rowsbase, $row);
classes/ability.php:1256
- A new permission path for
feedback:showis introduced for sampling (allow_show_feedback_if_sampling_enabled_and_sample_marked()), but there are no PHPUnit/Behat assertions covering its expected outcomes (e.g., who can/can’t see other markers’ details under sampling, draft vs finalised).
Please add a focused test case to lock in the intended sampling visibility rules and prevent regressions.
/**
* If sampling is enabled, and the student is in the selected sample, and both the initial mark and the sample
* mark are completed, then we should be able to see it and the marker's details.
*/
private function allow_show_feedback_if_sampling_enabled_and_sample_marked(): void {
$this->allow(
'show',
'mod_coursework\models\feedback',
|
Note: This is waiting on more testing from Eliot to confirm what we're doing. I've made the cache change as requested by David. I asked him if the rest f the comments are still valid, and he said "If Elliot is going to test after his interviews on preview - maybe he can confirm those suspect cases [when he's available to]. 4, 5, 6 need a confirmation of business rules.". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
classes/ability.php:1274
- The sampling visibility rule claims to require the student to be in the sample and both the initial and sample marks to be completed, but the current condition only checks that sampling is enabled and that some assessor_2/assessor_3 feedback exists. This can return true even when the initial (assessor_1) feedback is not yet completed, which doesn’t match the documented behaviour and can show marker details/marks earlier than intended.
if (
$this->get_coursework()->sampling_enabled() && $feedback->get_submission()->stage_feedback_exists([
'assessor_2',
'assessor_3',
])
…mpling. - Fixes a cache issue which was causing incorrect feedback to be returned. - Fixes logic as per description on ticket. - Fixes behat tests which look like they were previously wrong.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (4)
classes/ability.php:1274
- The docblock says this should only allow showing feedback once both the initial mark and the sampling mark are completed, but the condition currently only checks for sampled-stage feedback (assessor_2/assessor_3). This can allow visibility earlier than intended when assessor_1 feedback hasn’t been finalised yet.
if (
$this->get_coursework()->sampling_enabled() && $feedback->get_submission()->stage_feedback_exists([
'assessor_2',
'assessor_3',
])
classes/ability.php:1262
- In this sampling visibility rule, the capability gate omits
mod/coursework:addallocatedagreedgrade, which is used elsewhere to represent agreed graders who can act only on submissions they initially marked. As written, users with onlyaddallocatedagreedgradewill never satisfy this rule and may incorrectly be unable to view sampled feedback/marker details.
if (
!has_any_capability(
['mod/coursework:addinitialgrade', 'mod/coursework:addagreedgrade'],
$feedback->get_coursework()->get_context()
)
classes/models/submission.php:1264
stage_feedback_exists()usesin_array()without strict checking. Using strict comparison avoids any chance of loose type coercion causing an incorrect match and makes the intent clearer.
public function stage_feedback_exists(array $stageidentifiers, bool $finalised = true): bool {
$feedback = $this->get_feedbacks();
foreach ($feedback as $f) {
if (in_array($f->stageidentifier, $stageidentifiers) && (!$finalised || $f->finalised)) {
return true;
classes/render_helpers/grading_report/data/marking_cell_data.php:111
can_show()is computed here to decide whether to reveal marker details, butprocess_feedback_data()callscan_show()again for the same feedback/submission. On the grading report this doubles the ability checks (and any related work) per feedback row. Consider reusing a single computed boolean (e.g., pass it intoprocess_feedback_data()or set it on$marker) to avoid redundant permission checks.
if ($feedback = $row->get_feedback()) {
$canseeothermarkerdetails = $feedback->can_show($feedback->get_coursework(), $submission);
$marker = $this->create_marker_data($row->get_assessor(), $markernumber, $canseeothermarkerdetails);
$this->process_feedback_data($marker, $feedback, $rowsbase, $row);
Uh oh!
There was an error while loading. Please reload this page.