Conversation
Fix CI issues
There was a problem hiding this comment.
Pull request overview
This PR implements enhancements to the RFC (Request for Change) workflow in the customfield_sprogramme plugin. The changes introduce a "Continue editing" feature that allows users to cancel/revert an RFC before submission and continue editing, along with various UI improvements and test coverage.
Changes:
- Modified RFC cancellation logic to allow canceling RFCs in RFC_REQUESTED, RFC_SUBMITTED, and RFC_REJECTED states (not just RFC_SUBMITTED)
- Added "Continue editing" button for RFCs in the requested (saved but not submitted) state
- Added comprehensive Behat test scenarios covering RFC creation, submission, cancellation, and editing workflows
- Migrated Bootstrap 4 spacing utilities (mr-1) to Bootstrap 5 utilities (me-2) across all templates
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/local/rfc_manager_test.php | Updated test assertion to expect cancancel=true for RFC_REQUESTED state |
| tests/behat/create_rfc.feature | New comprehensive Behat test scenarios for RFC workflows including create, submit, cancel, and re-edit operations |
| tests/behat/behat_customfield_sprogramme.php | Added new Behat step definitions for checking cell editability and clicking data-action elements; fixed cell value checking logic |
| templates/table/rows.mustache | Updated Bootstrap spacing classes from mr-1 to me-2 |
| templates/table/rfc.mustache | Added "Continue editing" button for RFC_REQUESTED state; updated Bootstrap spacing classes |
| templates/table/modules.mustache | Updated Bootstrap spacing classes from mr-1 to me-2 |
| templates/formfield.mustache | Updated Bootstrap spacing classes from mr-1 to me-2 |
| lang/fr/customfield_sprogramme.php | Added French translation for "reeditrfc" (Continue editing) |
| lang/en/customfield_sprogramme.php | Added English string for "reeditrfc" (Continue editing) |
| classes/privacy/provider.php | Reordered interface implementation (cosmetic change for consistency) |
| classes/local/rfc_manager.php | Modified can_cancel to allow canceling any RFC except ACCEPTED; updated has_submitted to ignore CANCELLED RFCs; enhanced create method to reuse CANCELLED RFCs; updated cancel method to handle RFC_REQUESTED and RFC_REJECTED states |
| classes/local/programme_manager.php | Commented out continue statement for deleted rows in has_protected_data_changes method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $this->assertFalse($data['canaccept']); // Teacher 1 is not admin so cannot accept. | ||
| $this->assertTrue($data['cansubmit']); // Teacher 1 can submit. | ||
| $this->assertFalse($data['cancancel']); // Teacher 1 cannot cancel as nothing submitted. | ||
| $this->assertTrue($data['cancancel']); // Teacher 1 cannot cancel as nothing submitted. |
There was a problem hiding this comment.
The assertion has been changed from assertFalse to assertTrue, but the comment still says "Teacher 1 cannot cancel as nothing submitted." This is contradictory. The comment should be updated to reflect the new behavior, such as "Teacher 1 can cancel before submitting" or "Teacher 1 can continue editing before submitting."
| } | ||
| if ($oldvalue = $cell->find('css', '.staticdata')) { |
There was a problem hiding this comment.
The logic for checking cell values when there's no input field has a potential bug. If both .newvalue and .staticdata elements are found, the cellvalue will be set twice - first to the newvalue, then immediately overwritten by the oldvalue. This could lead to incorrect validation. Consider using an if-else structure or clarifying which value should take precedence when both exist.
| } | |
| if ($oldvalue = $cell->find('css', '.staticdata')) { | |
| } elseif ($oldvalue = $cell->find('css', '.staticdata')) { |
No description provided.