[#168] Fix fileid backfill and save_error for chunked file docids - #169
Conversation
There was a problem hiding this comment.
Version doesn't match. Should be 2026051405
| } | ||
| $rs->close(); | ||
|
|
||
| upgrade_plugin_savepoint(true, 2026051404, 'search', 'elastic'); |
There was a problem hiding this comment.
Version doesn't match. Should be 2026051405
dmitriim
left a comment
dmitriim
left a comment
There was a problem hiding this comment.
Please fix version mismatch. Otherwise looks good.
There was a problem hiding this comment.
Pull request overview
This PR updates the search_elastic plugin’s error tracking to correctly derive/backfill fileid from chunked file document IDs (e.g. 123_c1), ensuring both upgrade-time backfill and runtime save_error() behavior work with chunked docids.
Changes:
- Bumps plugin version to trigger a new upgrade step.
- Updates the upgrade backfill logic to correctly parse numeric + chunk-suffix docids and populate
search_elastic_errors.fileid. - Updates
error_service::save_error()to derivefileidvia a dedicated helper and adds PHPUnit coverage for the docid→fileid mapping.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
version.php |
Bumps plugin version to run the new upgrade step. |
db/upgrade.php |
Adjusts fileid backfill to handle chunked docids safely. |
classes/local/service/error_service.php |
Uses a helper to extract fileid from chunked/plain file docids. |
tests/local/service/error_service_test.php |
Adds tests covering fileid extraction for multiple docid formats. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $select = $DB->sql_like('docid', ':pattern', true, true, true); | ||
| $rs = $DB->get_recordset_select('search_elastic_errors', $select, ['pattern' => '%-%']); | ||
| foreach ($rs as $record) { | ||
| if (preg_match('/^(\d+)(?:_c\d+)?$/', $record->docid, $matches)) { | ||
| $record->fileid = (int) $matches[1]; | ||
| $DB->update_record('search_elastic_errors', $record); | ||
| } | ||
| } | ||
| $rs->close(); |
There was a problem hiding this comment.
Thanks @dmitriim I've updated the code as per copilot feedback :)
| private static function extract_fileid($documentid): ?int { | ||
| if (preg_match('/^(\d+)(?:_c\d+)?$/', (string)$documentid, $matches)) { | ||
| return (int) $matches[1]; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Thanks @dmitriim I've updated the code as per copilot feedback :)
|
@tsmilan can you please have a look at comments raised by AI review and action if it makes sense. |
Testing Instructions
Verify the upgrade backfill
search_elastic_errorsto cover each case (or use ones already there from testing chunking):docid = '123'(plain file, not chunked) → should backfill tofileid = 123docid = '123_c1'(chunked file) → should backfill tofileid = 123docid = 'mod_assign-activity-123'(non-file) →fileidstaysnulldocid = 'mod_assign-activity-123_c1'(non-file, chunked) →fileidstaysnull2026051405and navigate to<moodlesite>/admin/index.php(or CLI upgrade) to trigger the upgrade step, then check thefileidcolumn matches the expectations above.