feat(task): update progress on parent task#51339
feat(task): update progress on parent task#51339SowmyaArunachalam wants to merge 8 commits intofrappe:developfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #51339 +/- ##
===========================================
- Coverage 79.31% 79.30% -0.01%
===========================================
Files 1170 1170
Lines 124288 124361 +73
===========================================
+ Hits 98583 98629 +46
- Misses 25705 25732 +27
🚀 New features to boost your workflow:
|
SowmyaArunachalam
force-pushed
the
task-enhancements
branch
2 times, most recently
from
55281d2 to
6ff2ba6
Compare
📝 WalkthroughWalkthroughThe pull request implements automatic parent task progress propagation in the PMS module. The Task doctype configuration is updated to mark the progress field as conditionally read-only for group tasks. Two new methods are introduced: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
erpnext/projects/doctype/task/task.js (1)
62-70: Improve robustness of the project validation logic.The handler correctly enforces that parent and child tasks must belong to the same project, but there are a few improvements to consider:
- The check
frm.doc.project !== ""doesn't handlenullorundefinedvalues properly. In JavaScript, bothnullandundefinedare not strictly equal to"".- No user feedback is provided when
parent_taskis automatically cleared, which could be confusing.- No error handling for the database call failure.
🔎 Proposed improvements
project: function (frm) { - if (frm.doc.parent_task && frm.doc.project !== "") { + if (frm.doc.parent_task && frm.doc.project) { frappe.db.get_value("Task", frm.doc.parent_task, "project").then((r) => { if (r.message && r.message.project !== frm.doc.project) { frm.set_value("parent_task", ""); + frappe.show_alert({ + message: __("Parent Task has been cleared because it belongs to a different project."), + indicator: "orange" + }); } + }).catch((error) => { + console.error("Failed to validate parent task project:", error); }); } },erpnext/projects/doctype/task/test_task.py (1)
161-179: LGTM! Core parent progress aggregation is well-tested.The test correctly verifies that:
- A parent task's progress is updated to match a single child's progress (30%)
- With two children (30% and 50%), the parent progress becomes the average (40%)
The test validates the core functionality introduced in this PR.
Optional: Consider testing additional edge cases.
While the current test is solid, you might want to add test cases for:
- A parent with a child at 0% progress
- A parent with multiple children where the average requires rounding (e.g., three children at 33%, 33%, 34% → 33.33%)
- Removing a child task and verifying the parent's progress updates accordingly
These edge cases would strengthen test coverage, but they can be addressed in future iterations if needed.
erpnext/projects/doctype/task/task.py (1)
159-169: Consider error handling for missing parent tasks.The method correctly handles reparenting by updating both the old parent (if changed) and the new parent's progress. However, there's no error handling if the parent task referenced by
parent_taskordoc_before_save.parent_taskno longer exists in the database.Recommendation:
Wrap the
set_parent_progresscalls with existence checks or error handling to prevent failures when a parent task is deleted or invalid:🔎 Proposed improvements
def update_parent_progress(self): doc_before_save = self.get_doc_before_save() if ( doc_before_save and doc_before_save.parent_task and doc_before_save.parent_task != self.parent_task ): - self.set_parent_progress(doc_before_save.parent_task) + if frappe.db.exists("Task", doc_before_save.parent_task): + self.set_parent_progress(doc_before_save.parent_task) if self.parent_task: - self.set_parent_progress(self.parent_task) + if frappe.db.exists("Task", self.parent_task): + self.set_parent_progress(self.parent_task)Note: While the validation logic in
validate_parent_is_group(line 207-215) should prevent invalid parent tasks, defensive programming here would make the code more resilient to edge cases (e.g., concurrent deletions or data integrity issues).
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
erpnext/projects/doctype/task/task.jserpnext/projects/doctype/task/task.jsonerpnext/projects/doctype/task/task.pyerpnext/projects/doctype/task/test_task.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-01T11:04:59.343Z
Learnt from: karm1000
Repo: frappe/erpnext PR: 48865
File: erpnext/accounts/doctype/purchase_invoice/purchase_invoice.json:1657-1664
Timestamp: 2025-08-01T11:04:59.343Z
Learning: In ERPNext/Frappe framework, virtual fields (marked with "is_virtual": 1) are always read-only by default and do not require an explicit "read_only": 1 property in their JSON definition.
Applied to files:
erpnext/projects/doctype/task/task.json
📚 Learning: 2025-12-16T05:33:58.723Z
Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.
Applied to files:
erpnext/projects/doctype/task/task.pyerpnext/projects/doctype/task/test_task.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (8)
erpnext/projects/doctype/task/task.json (3)
191-196: LGTM! Progress field correctly made read-only for group tasks.The
read_only_depends_onconstraint ensures that progress cannot be manually edited for group tasks (whereis_group == 1), which is appropriate since the backend automatically computes parent task progress as the average of child tasks' progress values. This prevents user confusion and data inconsistency.
412-412: LGTM! Metadata normalization.The naming rule has been updated from the deprecated "Expression (old style)" to "Expression", which is a standard framework migration.
429-429: LGTM! Row format enhancement.The addition of
row_format: "Dynamic"enables dynamic row formatting for the Task doctype, which can improve the UI presentation.erpnext/projects/doctype/task/test_task.py (1)
187-206: LGTM! Reparenting cleanup is well-tested.The test correctly verifies that when a child task is moved from one parent to another:
- The old parent's
depends_onlist is cleared (no stale references)- The new parent's
depends_onlist includes the child taskThis validates the cleanup logic introduced in
populate_depends_onmethod.erpnext/projects/doctype/task/task.py (4)
11-11: LGTM! Import addition is correct.The
Avgfunction is correctly imported and used in theset_parent_progressmethod to compute the average progress of child tasks.
171-177: LGTM! Parent progress calculation is efficient and correct.The method:
- Uses the query builder to compute the average progress of all child tasks efficiently in a single query
- Handles the case where there are no children (returns 0)
- Uses
frappe.db.set_valuewhich bypasses theon_updatehook, preventing infinite recursionThe implementation is sound and follows best practices for this type of aggregation.
Performance note: Each child task save will trigger a parent progress update. While this is the correct behavior, be aware that bulk operations on many child tasks will result in multiple parent updates. If bulk operations become a performance concern in the future, consider batching or debouncing the parent updates.
234-234: LGTM! Parent progress update is correctly triggered.Adding
update_parent_progress()to theon_updatehook ensures that parent task progress is automatically recalculated whenever a child task is modified. The method has appropriate guards to avoid unnecessary work when the task has no parent.
322-341: LGTM! Cleanup logic correctly handles reparenting.The enhanced
populate_depends_onmethod now:
- Adds the child task to the new parent's
depends_onlist (existing behavior)- NEW: Removes the child task from the old parent's
depends_onlist when reparenting occursThe cleanup logic (lines 333-341) correctly:
- Checks if the document existed before save (not a new document)
- Verifies the old parent exists and differs from the new parent
- Uses the query builder to delete the stale
TaskDependsOnrowThis prevents orphaned dependency references and maintains data integrity during reparenting operations.
SowmyaArunachalam
changed the title
SowmyaArunachalam
force-pushed
the
task-enhancements
branch
from
6ff2ba6 to
8cb2cb5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
erpnext/projects/doctype/task/test_task.py (1)
161-179: Test validates parent progress aggregation correctly.The test logic is sound: it verifies that parent progress is computed as the average of its children's progress (30 from first child, then 40 after second child with progress 50 is added).
Optional: Consider testing multi-level hierarchy propagation
The current test validates one level of parent-child relationships. You might want to add a test case for multi-level hierarchies (grandparent → parent → child) to verify that progress updates cascade correctly up the entire tree:
def test_multilevel_parent_task_progress(self): grandparent_task = create_task( subject="_Test Grandparent Task", is_group=1, ) parent_task = create_task( subject="_Test Parent Task", is_group=1, parent_task=grandparent_task.name, ) child_task = create_task( subject="_Test Child Task", parent_task=parent_task.name, save=False ) child_task.progress = 60 child_task.save() parent_task.reload() grandparent_task.reload() self.assertEqual(parent_task.progress, 60) self.assertEqual(grandparent_task.progress, 60)
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
erpnext/projects/doctype/task/task.jserpnext/projects/doctype/task/task.jsonerpnext/projects/doctype/task/task.pyerpnext/projects/doctype/task/test_task.py
🚧 Files skipped from review as they are similar to previous changes (2)
- erpnext/projects/doctype/task/task.js
- erpnext/projects/doctype/task/task.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-16T05:33:58.723Z
Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.
Applied to files:
erpnext/projects/doctype/task/task.pyerpnext/projects/doctype/task/test_task.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (5)
erpnext/projects/doctype/task/test_task.py (1)
187-206: Test correctly validates depends_on cleanup when reparenting.The test verifies that when a child task is reassigned to a different parent, the old parent's
depends_onentries are cleaned up and the new parent'sdepends_onis populated correctly.erpnext/projects/doctype/task/task.py (4)
11-11: LGTM!The
Avgimport is necessary for calculating the average progress of child tasks.
159-169: LGTM!The orchestration logic correctly handles updating both the old parent (when reparenting) and the current parent. The condition on line 164 ensures the old parent is only updated when it differs from the new one.
236-236: Verify parent progress update placement.The call to
update_parent_progress()is appropriately placed at the end ofon_updateafter other operations. However, ensure that the recursion issue identified inset_parent_progress(lines 171-179) is resolved to prevent cascading updates up the parent chain.
324-343: LGTM!The enhanced logic correctly removes stale
TaskDependsOnentries when a task is reparented. The query builder syntax properly targets rows where the parent matches the old parent task and the task field matches the current task name.
| frappe.db.get_value("Task", frm.doc.parent_task, "project").then((r) => { | ||
| if (r.message && r.message.project !== frm.doc.project) { | ||
| frm.set_value("parent_task", ""); | ||
| } |
There was a problem hiding this comment.
This code should be written on the Python side so that it also applies when someone creates the task using the REST API.
| self.set_parent_progress(doc_before_save.parent_task) | ||
|
|
||
| if self.parent_task: | ||
| self.set_parent_progress(self.parent_task) |
There was a problem hiding this comment.
The method set_parent_progress can only call one time instead of two
If self.parent_task and (self.is_new() or not doc_before_save.parent_task or doc_before_save.parent_task != self.parent_task):
self.set_parent_progress(self.parent_task)
There was a problem hiding this comment.
Since set_parent_progress is executed only once, unlinking a parent task does not trigger a recalculation, causing the progress of the removed parent task to remain outdated.
| frappe.qb.from_(Task).select(Avg(Task.progress)).where(Task.parent_task == parent_task) | ||
| ).run()[0][0] or 0 | ||
|
|
||
| doc = frappe.get_doc("Task", parent_task) |
There was a problem hiding this comment.
instead of get_doc, use frappe.db.set_value("Task", parent_task, "progress", avg_progress)
There was a problem hiding this comment.
Here, the get_doc is intentionally used to recursively update the progress of all parent tasks.
There was a problem hiding this comment.
Could you explain with examples
There was a problem hiding this comment.
P1 → P2 → T1
If a task T1 has a parent P2, and P2 has a parent P1, then while updating the progress of T1, the progress must also be updated for both P2 and P1.
To update the progress recursively up to the root task, get_doc is used instead of set_value.
SowmyaArunachalam
force-pushed
the
task-enhancements
branch
from
8cb2cb5 to
418f4dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
erpnext/projects/doctype/task/task.py (2)
220-224: Consider notifying users when parent_task is auto-cleared.The method silently clears
parent_taskwhen projects don't match. While this achieves the objective, users might be confused when their parent assignment disappears without explanation.💡 Optional: Add a message to inform the user
def validate_parent_task_project(self): if self.parent_task and self.project: parent_proj = frappe.get_value("Task", self.parent_task, "project") if self.project != parent_proj: + frappe.msgprint( + _("Parent Task {0} has been unlinked as it belongs to a different project.").format( + get_link_to_form("Task", self.parent_task) + ), + alert=True, + ) self.parent_task = ""
172-180: Variable shadowing and potential error on missing parent.Two observations:
Taskvariable on line 173 shadows the class name. While it works, it reduces readability.- If
parent_taskdoesn't exist (e.g., deleted concurrently),frappe.get_docwill raise an exception.♻️ Suggested improvements
def set_parent_progress(self, parent_task): - Task = frappe.qb.DocType("Task") + TaskDoc = frappe.qb.DocType("Task") avg_progress = ( - frappe.qb.from_(Task).select(Avg(Task.progress)).where(Task.parent_task == parent_task) + frappe.qb.from_(TaskDoc).select(Avg(TaskDoc.progress)).where(TaskDoc.parent_task == parent_task) ).run()[0][0] or 0 + if not frappe.db.exists("Task", parent_task): + return + doc = frappe.get_doc("Task", parent_task) doc.progress = avg_progress doc.save()
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
erpnext/projects/doctype/task/task.jsonerpnext/projects/doctype/task/task.pyerpnext/projects/doctype/task/test_task.py
🚧 Files skipped from review as they are similar to previous changes (1)
- erpnext/projects/doctype/task/task.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-16T05:33:58.723Z
Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.
Applied to files:
erpnext/projects/doctype/task/test_task.pyerpnext/projects/doctype/task/task.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (7)
erpnext/projects/doctype/task/task.py (5)
11-11: LGTM!The
Avgimport addition is correct and necessary for the new parent progress calculation logic.
160-170: LGTM!The method correctly handles both reparenting scenarios: updating the old parent's progress when a child is moved away, and updating the new parent's progress. Using
get_doc_before_save()is the idiomatic Frappe pattern for detecting field changes.
93-93: LGTM!Correct placement in the validation chain to ensure
parent_taskis cleared beforeon_updateruns.
243-243: LGTM!Appropriate placement at the end of
on_update()ensures progress propagation happens after dependency updates are complete.
331-350: LGTM!The enhancement correctly handles task reparenting by:
- Adding the child to the new parent's
depends_on(existing logic)- Removing the child from the old parent's
depends_onwhen reparented (new logic)The delete query is properly scoped to only remove the specific dependency row.
erpnext/projects/doctype/task/test_task.py (2)
161-179: Good test coverage for the happy path.The test correctly validates that parent progress equals the average of child progress values. Consider adding edge case tests in a follow-up:
- Single child with 0% or 100% progress
- Child progress update (not just creation)
- Child task deletion and its effect on parent progress
187-206: LGTM!Comprehensive test that validates both sides of the reparenting operation:
- Old parent's
depends_onis cleared- New parent's
depends_oncontains the childThe test properly reloads both parent tasks to verify database state.
|
This pull request has been automatically marked as inactive because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing. |
|
commenting to keep it active |
|
@rohitwaghchaure can you review this PR |
|
This pull request has been automatically marked as inactive because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing. |
|
This pull request has been automatically marked as inactive because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing. |
SowmyaArunachalam
force-pushed
the
task-enhancements
branch
from
418f4dc to
51c67d6
Compare
mihir-kandoi
assigned
mihir-kandoi and
nishkagosalia and unassigned
rohitwaghchaure and
mihir-kandoi
mihir-kandoi
added
squash
6 participants
Issue:
When child tasks are updated with their progress percentage, the parent task's progress is not automatically recalculated based on its children's progress.
fixes: PMS Module: Child Task Progress Not Reflected in Parent Task #47267
When the child task is updated with the new parent task, the depends on reference is not removed from the previous parent task of the child task.
The user created task T3, associated with project P1 and parent task PT1. If the project is changed to Project P2, the parent task PT1 is still linked to the task T3
(Note: After this, Task T3 is assigned to Project P2, but the parent task PT1 is assigned to the P1)
Solution
Backport needed for v15, v16