fix(Job Card): apply precision to total time in minutes

[barredterra]

This pull request introduces improvements to the calculation and precision handling of time and quantity fields in the Job Card doctype, ensuring consistency and accuracy in manufacturing job tracking. The most important changes focus on using the flt() function for precision and updating values only when necessary.

Precision and calculation improvements:

• Updated the calculation of time_in_mins and total_time_in_mins in the validate_time_logs method to use the flt() function with field-specific precision, ensuring more accurate and consistent values.
• Modified the calculation of completed_time and completed_qty in the update_sub_operation_status method to use the flt() function with the appropriate precision for each field, improving consistency for sub-operation tracking.

Logic and update optimizations:

• Added logic to update completed_time only if the value actually changed, preventing unnecessary writes and ensuring the field is only updated when needed.

Screenshot of original issue:

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

erpnext/manufacturing/doctype/job_card/job_card.py (1)

248-281: Consider centralizing time-diff and rounding logic for all code paths

Right now, time_in_mins / total_time_in_mins are computed and/or updated in several places (validate_time_logs, set_expected_and_actual_time, add_time_log, add_time_logs), and only this block applies explicit field-based precision. For long‑term maintainability and fully consistent rounding, consider extracting a small helper (e.g., get_time_in_mins(from_time, to_time, precision)) and reusing it everywhere you derive or aggregate minutes, and optionally also applying self.precision("total_time_in_mins") inside set_expected_and_actual_time when setting total_time_in_mins.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0ca6e19 and b46088f.

📒 Files selected for processing (1)

• erpnext/manufacturing/doctype/job_card/job_card.py (1 hunks)

⏰ 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: Patch Test
• GitHub Check: Python Unit Tests (2)
• GitHub Check: Python Unit Tests (4)
• GitHub Check: Python Unit Tests (1)
• GitHub Check: Python Unit Tests (3)
• GitHub Check: Summary

🔇 Additional comments (1)

erpnext/manufacturing/doctype/job_card/job_card.py (1)

270-281: Precision handling for time_in_mins and total_time_in_mins looks correct

Using flt(..., d.precision("time_in_mins")) for each row and flt(..., self.precision("total_time_in_mins")) for the aggregate will make stored values consistent with the field’s configured precision and should address rounding discrepancies without changing the existing control flow.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

erpnext/manufacturing/doctype/job_card/job_card.py (2)

657-675: Precision and conditional update logic looks good.

The precision handling is correctly applied for both completed_time and completed_qty. The conditional update at lines 674-675 is a good optimization to avoid unnecessary writes.

Minor inconsistency: completed_qty assignment at lines 667-670 doesn't have the same "only if changed" guard that completed_time has. Consider applying the same pattern for consistency if write avoidance is a goal.

+                # Only update completed_qty if value actually changed
+                new_completed_qty = flt(
+                    operation_deatils.completed_qty / len(set(operation_deatils.employee)),
+                    self.precision("completed_qty", "sub_operations"),
+                )
+                if flt(row.completed_qty) != new_completed_qty:
+                    row.completed_qty = new_completed_qty
-                if operation_deatils.completed_qty:
-                    row.completed_qty = flt(
-                        operation_deatils.completed_qty / len(set(operation_deatils.employee)),
-                        self.precision("completed_qty", "sub_operations"),
-                    )

---

676-680: Conditional reset is correctly implemented for completed_time.

Same minor inconsistency as above: completed_qty at line 680 doesn't have the conditional guard. For completeness, consider:

             row.status = "Pending"
             if flt(row.completed_time) != 0.0:
                 row.completed_time = 0.0
-            row.completed_qty = 0.0
+            if flt(row.completed_qty) != 0.0:
+                row.completed_qty = 0.0

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b46088f and 7d095e4.

📒 Files selected for processing (1)

• erpnext/manufacturing/doctype/job_card/job_card.py (2 hunks)

⏰ 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: Patch Test
• GitHub Check: Python Unit Tests (1)
• GitHub Check: Python Unit Tests (3)
• GitHub Check: Python Unit Tests (4)
• GitHub Check: Python Unit Tests (2)
• GitHub Check: Summary

🔇 Additional comments (2)

erpnext/manufacturing/doctype/job_card/job_card.py (2)

271-273: LGTM!

Correct application of precision to individual time log entries. Using d.precision("time_in_mins") properly retrieves the field precision from the child table row.

---

280-280: LGTM!

Good approach to apply precision after aggregation rather than rounding during each iteration, which avoids compounding rounding errors.