Skip to content

Commit

Permalink
MDL-65527 mod_assign: Update group assignment submission filter to sh…
Browse files Browse the repository at this point in the history 
…ow all group members by submission status
  • Loading branch information
Kwabena committed Jan 14, 2025
1 parent f4f1666 commit 16e2d26
Showing 1 changed file with 13 additions and 0 deletions.
13 changes: 13 additions & 0 deletions mod/assign/locallib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6133,6 +6133,19 @@ protected function update_team_submission(stdClass $submission, $userid, $update
} else {
$submission->status = ASSIGN_SUBMISSION_STATUS_DRAFT;
}

// MDL-65527-main -> Start
// Update Group assigment submission for members so it can be filtered by status to show all members
if( count($team) > 1 && $anysubmitted){
foreach ($team as $member) {
$groupmembersubmission = $this->get_user_submission($member->id, false, $submission->attemptnumber);
$groupmembersubmission->status = $submission->status;
$groupmembersubmission->timemodified = $submission->timemodified;
$result = $DB->update_record('assign_submission', $groupmembersubmission);
}
}
// MDL-65527-main -> End

$result = $DB->update_record('assign_submission', $submission);
}
} else {
Expand Down

1 comment on commit 16e2d26

@tonyjbutler
Copy link
Member

Choose a reason for hiding this comment

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

Thanks Kwabena, initial peer review below:

  • The commit message contains the issue key and component name, which is great, however it also needs to be max 72 characters in length (in total, so that's including the issue key and component name).
  • Line 6137: this comment is not necessary, and the same goes for the comment on line 6147.
  • Line 6138: this comment needs to have a full stop at the end.
  • Line 6139: there should be a space between 'if' and '(', but no space after the '('. There should be a space between the ')' and '{' at the end of the line.
  • Line 6144: $result is overwritten with each iteration (and is then overwritten again outside of the loop), so there's little point in having it here. Ideally we'd like to avoid having database calls in a loop like this, to avoid degrading performance for large groups, but since Moodle's DML API doesn't have a method to update multiple rows in a single call I guess we have no other option (for updates).

Most of the above issues should be highlighted by PHP CodeSniffer if you configure its integration with PHPStorm. Have a look at https://moodledev.io/general/development/tools/phpcs if you haven't already, and search the web for instructions on how to configure PHPStorm for it (e.g. https://www.jetbrains.com/help/phpstorm/using-php-code-sniffer.html).

Overall I'm not actually convinced that this is the best approach to fixing this issue. In https://tracker.moodle.org/browse/MDL-65527, Tim reported that the bug only shows up with a particular assignment config setting enabled:

If the "require students to click submit" is not enabled, it works fine.

From playing around with the config myself, I've found that even with that setting enabled, the 'Status' column in the submissions table always shows the right thing, so I think I'd be tempted to look at the criteria used by the filter code to determine whether a submission is submitted or not, and modify that to make it consistent with the logic that determines which status to display in the table.

The problem with changing what's stored in the database, as you've done here, is that it changes much more fundamentally the behaviour of the assignment activity, and this is far more likely to affect other functionality and have unwanted knock-on effects, potentially causing new regressions in the activity. Try to address the issue directly and change as little as possible to reduce the impact of the change.

I hope this all makes sense.

Please sign in to comment.