Document Task progress api - #1710
Open
brendanheywood wants to merge 2 commits into
Open
brendanheywood wants to merge 2 commits into
brendanheywood wants to merge 2 commits into
Conversation
✅ Deploy Preview for moodledevdocs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The adhoc stored-progress initialization issue must be corrected before approval.
Review effort: Lite
Findings: 4
Open (4)
What changed in this PR
Documents task progress and stored progress polling APIs across current and versioned Moodle documentation.
Changes:
- Adds scheduled and adhoc task progress examples.
- Documents polling and cleanup behavior.
- Mirrors updates across Moodle 4.5–5.2.
| File | Summary and findings |
|---|---|
versioned_docs/version-5.2/apis/subsystems/task/index.md |
Adds task progress guidance. Moderate (4 votes): queue adhoc tasks before initializing stored progress. Nit (1 vote): add polling/rendering instructions. |
versioned_docs/version-5.2/apis/subsystems/output/index.md |
Documents polling and cleanup. Nit (1 vote each, reported twice): clarify cleanup behavior for records with old lastupdate values. |
versioned_docs/version-5.1/apis/subsystems/task/index.md |
Adds task progress guidance. Moderate (4 votes): queue adhoc tasks before initializing stored progress. Nit (1 vote): add polling/rendering instructions. |
versioned_docs/version-5.1/apis/subsystems/output/index.md |
Documents polling and cleanup. Nit (1 vote): clarify cleanup behavior for records with old lastupdate values. |
versioned_docs/version-5.0/apis/subsystems/task/index.md |
Adds task progress guidance. Moderate (4 votes): queue adhoc tasks before initializing stored progress. Nit (1 vote): add polling/rendering instructions. |
versioned_docs/version-5.0/apis/subsystems/output/index.md |
Documents polling and cleanup. Nit (1 vote): clarify cleanup behavior for records with old lastupdate values. |
versioned_docs/version-4.5/apis/subsystems/task/index.md |
Adds task progress guidance. Nit (1 vote): add polling/rendering instructions. |
versioned_docs/version-4.5/apis/subsystems/output/index.md |
Documents polling and cleanup. |
docs/apis/subsystems/task/index.md |
Adds task progress guidance. Moderate (4 votes): queue adhoc tasks before initializing stored progress. Nit (1 vote): add polling/rendering instructions. |
docs/apis/subsystems/output/index.md |
Documents polling and cleanup. Nit (1 vote): clarify cleanup behavior for records with old lastupdate values. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
brendanheywood
force-pushed
the
task-progress
branch
from
September 28, 2026 01:03
c014a03 to
6227b87
Compare
brendanheywood
force-pushed
the
task-progress
branch
from
September 28, 2026 01:20
6227b87 to
8a043ea
Compare
safatshahin
self-requested a review
September 28, 2026 05:55
safatshahin
requested changes
Sep 28, 2026
safatshahin
left a comment
safatshahin
left a comment
Collaborator
There was a problem hiding this comment.
Hi @brendanheywood
Thank you working on his. The patch is looking great, some quick ones worth fixing:
- Broken links on main: Both links point to main/admin/tool/task/, since the move to public/ folder in 5.1, that path returns 404 on main. Either link to public/admin/tool/task/..., or, better, pin each versioned doc to its stable branch (e.g. MOODLE_405_STABLE).
- One code comment is misleading: In the 5.x versions, the comment says start_stored_progress() "updates the stored progress record with a start time". That's only true if a pending record already exists; otherwise it creates a new one. The 4.5 wording ("creates the stored progress record") is closer.
- Cleanup doesn't catch every record: The cleanup task deletes rows where lastupdate < 24h ago. A pending record that was never updated has a NULL lastupdate, so it won't be deleted. The docs say records "not updated within the last 24 hours" are removed, which overstates it. It could use a short caveat, or be raised as a core bug.
- Super minor
- The example message 'i am at ' . $i would be nicer as a get_string() or at least properly capitalised.
- The display example assigns $idnumber twice in a row, which reads like a mistake. An if/else or separate snippets would be clearer.
Cheers!
Contributor
Author
|
thanks @safatshahin I've added a fix up addressing all of those except point 3. I don't think we should document the bug I think we should fix it https://moodle.atlassian.net/browse/MDL-89942 |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.