refactor(accounts): repost accounting ledger#56442
Conversation
github-actions
Bot
added
needs-tests
Confidence Score: 3/5The new status lifecycle and explicit trigger are valuable improvements, but several correctness gaps in the Python layer need to be resolved before this is safe to merge. The refactor moves repost execution out of on_submit and adds meaningful observability (status field, error_log, background job tracking). However, Expense Claim vouchers will silently report Completed without any entries being reposted, there is no on_cancel hook so the status field is misleading after cancellation, start_repost can be triggered repeatedly allowing duplicate background jobs with orphaned job references, and the early-return in repost() can overwrite a valid Cancelled status with Pending on non-submitted documents. repost_accounting_ledger.py needs the most attention — specifically the _repost_vouchers dispatch table (Expense Claim), the missing on_cancel hook, the start_repost guard, and the early-return status reset.
|
| Filename | Overview |
|---|---|
| erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.py | Core repost logic refactored: moves repost trigger from on_submit to an explicit start_repost whitelist method, adds status tracking (Pending/Queued/In Progress/Completed/Failed), and splits the monolithic repost function into smaller helpers. Four logic issues: Expense Claim silently skipped, no on_cancel hook to set status=Cancelled, start_repost allows double-triggering, and the early-return in repost() incorrectly resets status to Pending on non-submitted docs. |
| erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.json | Adds status (Select), error_log (Code), and scheduled_job (Link to RQ Job) fields; enables bulk edit and dynamic row format; adds in_list_view/filter on status. Schema changes are consistent with the Python changes. |
| erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.js | Adds Start Reposting button that only appears for Failed/Pending submitted docs; refactors preview and repost into named event handlers. UI logic is consistent with the new server-side flow. |
| erpnext/accounts/doctype/repost_accounting_ledger_items/repost_accounting_ledger_items.json | Makes voucher_type and voucher_no required, adds a column break, increases column widths, enables bulk edit and dynamic row format. Straightforward schema cleanup. |
| erpnext/accounts/doctype/repost_accounting_ledger_items/repost_accounting_ledger_items.py | Updates type annotations to remove Optional on voucher_type/voucher_no, reflecting the newly required fields in the JSON schema. No behavioral change. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
actor User
participant Form as Repost Accounting Ledger
participant DB as Database
participant Queue as RQ Background Queue
participant Worker as Background Worker
User->>Form: Submit document
Form->>Form: before_submit() — msgprint alert
Form->>DB: "docstatus = 1, status = Pending"
User->>Form: Click Start Reposting
Form->>Form: "start_repost() — check docstatus == 1"
Form->>DB: "db_set(status = Queued)"
alt "len(vouchers) <= 5 synchronous"
Form->>Form: repost(name) inline
Form->>DB: "db_set(status = In Progress)"
Form->>DB: validate_parallel_voucher_reposting()
Form->>Worker: _repost_vouchers() for each voucher
alt Success
Form->>DB: "db_set(status = Completed)"
else Exception
Form->>DB: rollback()
Form->>DB: "db_set(status = Failed, error_log = traceback)"
end
else "len(vouchers) > 5 async"
Form->>Queue: frappe.enqueue(repost, job_id)
Form->>DB: "db_set(scheduled_job = job_id)"
Queue-->>User: Repost started in background
Queue->>Worker: pick up job
Worker->>DB: "db_set(status = In Progress)"
Worker->>DB: validate_parallel_voucher_reposting()
Worker->>Worker: _repost_vouchers() for each voucher
alt Success
Worker->>DB: "db_set(status = Completed)"
else Exception
Worker->>DB: rollback()
Worker->>DB: "db_set(status = Failed, error_log = traceback)"
end
end
User->>Form: Cancel document
Form->>Form: before_cancel() check_ral_is_cancellable()
Form->>DB: "docstatus = 2"
Note over Form,DB: No on_cancel — status stays at pre-cancel value
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
actor User
participant Form as Repost Accounting Ledger
participant DB as Database
participant Queue as RQ Background Queue
participant Worker as Background Worker
User->>Form: Submit document
Form->>Form: before_submit() — msgprint alert
Form->>DB: "docstatus = 1, status = Pending"
User->>Form: Click Start Reposting
Form->>Form: "start_repost() — check docstatus == 1"
Form->>DB: "db_set(status = Queued)"
alt "len(vouchers) <= 5 synchronous"
Form->>Form: repost(name) inline
Form->>DB: "db_set(status = In Progress)"
Form->>DB: validate_parallel_voucher_reposting()
Form->>Worker: _repost_vouchers() for each voucher
alt Success
Form->>DB: "db_set(status = Completed)"
else Exception
Form->>DB: rollback()
Form->>DB: "db_set(status = Failed, error_log = traceback)"
end
else "len(vouchers) > 5 async"
Form->>Queue: frappe.enqueue(repost, job_id)
Form->>DB: "db_set(scheduled_job = job_id)"
Queue-->>User: Repost started in background
Queue->>Worker: pick up job
Worker->>DB: "db_set(status = In Progress)"
Worker->>DB: validate_parallel_voucher_reposting()
Worker->>Worker: _repost_vouchers() for each voucher
alt Success
Worker->>DB: "db_set(status = Completed)"
else Exception
Worker->>DB: rollback()
Worker->>DB: "db_set(status = Failed, error_log = traceback)"
end
end
User->>Form: Cancel document
Form->>Form: before_cancel() check_ral_is_cancellable()
Form->>DB: "docstatus = 2"
Note over Form,DB: No on_cancel — status stays at pre-cancel value
Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.py:266-277
**Expense Claim silently skipped during repost**
`Expense Claim` was previously in the explicit `["Payment Entry", "Journal Entry", "Expense Claim"]` branch but is no longer handled explicitly and is also absent from `repost_allowed_doctypes` in `hooks.py`. Any user who adds `Expense Claim` to their Repost Allowed Types settings will find that the validation in `validate_docs_for_voucher_types` passes (it's in settings), but `_repost_vouchers` falls through all four branches without reposting anything. The loop completes normally, `db_set({"status": "Completed", ...})` is called, and the accounting entries are never touched — a silent no-op reported as success.
Either add `Expense Claim` back to the `_repost_pe_je` branch's doctype list, or add it to `repost_allowed_doctypes` in `hooks.py`.
### Issue 2 of 4
erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.py:190-211
**No guard against double-triggering `start_repost`**
`start_repost` only checks `docstatus == 1` before proceeding; it does not verify that the document's status is `"Pending"` or `"Failed"`. If a user (or an API caller) invokes this method twice in rapid succession while the async path is taken (`len > 5`), the second call will enqueue a second background job and overwrite `scheduled_job` with the new job's ID. Both jobs then enter `repost()`, both read `status == "Queued"` (the race window before either sets "In Progress"), both pass `validate_parallel_voucher_reposting`, and both proceed to independently repost the same vouchers concurrently. The `scheduled_job` pointer now points to job 2, so `check_ral_is_cancellable` is checking the wrong job entirely for job 1's progress.
Adding `if self.status not in ["Pending", "Failed"]: frappe.throw(...)` at the top of `start_repost` would close the window.
### Issue 3 of 4
erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.py:177-178
**No `on_cancel` hook — status not updated after cancellation**
The schema defines `"Cancelled"` as a valid status option, and `on_discard` correctly sets status to `"Cancelled"` for discard. However, there is no `on_cancel` method, so when a user cancels the document (docstatus→2), the `status` field remains at whatever value it held before (`"Pending"`, `"Failed"`, etc.). A cancelled document with `status == "Failed"` will look like it is still actionable in list views filtered by status, and any logic that queries `status == "Cancelled"` (e.g., `repost()` early-return guard) will never match a normally cancelled document.
Add `def on_cancel(self): self.db_set("status", "Cancelled")` to ensure the status field stays in sync with docstatus.
### Issue 4 of 4
erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.py:225-227
**Status incorrectly reset to "Pending" on non-submitted documents**
When the background job's `docstatus != 1` branch fires (e.g., the document was cancelled between `start_repost` and the job actually executing), `repost_doc.db_set("status", "Pending")` is called on a document that may have `docstatus == 2`. This writes `status = "Pending"` onto a cancelled document, contradicting the "Cancelled" status that `on_cancel` (once added) or `on_discard` would have set. The early-return is correct, but the status field should not be modified in this path — just return without touching it.
Reviews (1): Last reviewed commit: "refactor(accounts): repost accounting le..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @frappe.whitelist() | ||
| def start_repost(self): | ||
| if self.docstatus != 1: | ||
| frappe.throw(_("Reposting can be started only for submitted document.")) | ||
|
|
||
| self.check_permission("write") | ||
| self.db_set("status", "Queued") | ||
|
|
||
| if len(self.vouchers) > 5: | ||
| job_name = "repost_accounting_ledger_" + self.name | ||
| job_id = frappe.generate_hash() | ||
| frappe.enqueue( | ||
| method="erpnext.accounts.doctype.repost_accounting_ledger.repost_accounting_ledger.start_repost", | ||
| account_repost_doc=self.name, | ||
| method="erpnext.accounts.doctype.repost_accounting_ledger.repost_accounting_ledger.repost", | ||
| repost_doc_name=self.name, | ||
| is_async=True, | ||
| job_name=job_name, | ||
| job_name=f"repost_accounting_ledger_{self.name}", | ||
| job_id=job_id, | ||
| enqueue_after_commit=True, | ||
| ) | ||
| frappe.msgprint(_("Repost has started in the background")) | ||
| self.db_set("scheduled_job", job_id) | ||
| frappe.msgprint(_("Repost has started in the background"), alert=True, indicator="blue") | ||
| else: | ||
| start_repost(self.name) | ||
| repost(self.name) |
There was a problem hiding this comment.
start_repost
start_repost only checks docstatus == 1 before proceeding; it does not verify that the document's status is "Pending" or "Failed". If a user (or an API caller) invokes this method twice in rapid succession while the async path is taken (len > 5), the second call will enqueue a second background job and overwrite scheduled_job with the new job's ID. Both jobs then enter repost(), both read status == "Queued" (the race window before either sets "In Progress"), both pass validate_parallel_voucher_reposting, and both proceed to independently repost the same vouchers concurrently. The scheduled_job pointer now points to job 2, so check_ral_is_cancellable is checking the wrong job entirely for job 1's progress.
Adding if self.status not in ["Pending", "Failed"]: frappe.throw(...) at the top of start_repost would close the window.
Prompt To Fix With AI
This is a comment left during a code review.
Path: erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.py
Line: 190-211
Comment:
**No guard against double-triggering `start_repost`**
`start_repost` only checks `docstatus == 1` before proceeding; it does not verify that the document's status is `"Pending"` or `"Failed"`. If a user (or an API caller) invokes this method twice in rapid succession while the async path is taken (`len > 5`), the second call will enqueue a second background job and overwrite `scheduled_job` with the new job's ID. Both jobs then enter `repost()`, both read `status == "Queued"` (the race window before either sets "In Progress"), both pass `validate_parallel_voucher_reposting`, and both proceed to independently repost the same vouchers concurrently. The `scheduled_job` pointer now points to job 2, so `check_ral_is_cancellable` is checking the wrong job entirely for job 1's progress.
Adding `if self.status not in ["Pending", "Failed"]: frappe.throw(...)` at the top of `start_repost` would close the window.
How can I resolve this? If you propose a fix, please make it concise.| def before_submit(self): | ||
| frappe.msgprint(_("Press 'Start Reposting' to begin."), alert=True, indicator="blue") |
There was a problem hiding this comment.
on_cancel hook — status not updated after cancellation
The schema defines "Cancelled" as a valid status option, and on_discard correctly sets status to "Cancelled" for discard. However, there is no on_cancel method, so when a user cancels the document (docstatus→2), the status field remains at whatever value it held before ("Pending", "Failed", etc.). A cancelled document with status == "Failed" will look like it is still actionable in list views filtered by status, and any logic that queries status == "Cancelled" (e.g., repost() early-return guard) will never match a normally cancelled document.
Add def on_cancel(self): self.db_set("status", "Cancelled") to ensure the status field stays in sync with docstatus.
Prompt To Fix With AI
This is a comment left during a code review.
Path: erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.py
Line: 177-178
Comment:
**No `on_cancel` hook — status not updated after cancellation**
The schema defines `"Cancelled"` as a valid status option, and `on_discard` correctly sets status to `"Cancelled"` for discard. However, there is no `on_cancel` method, so when a user cancels the document (docstatus→2), the `status` field remains at whatever value it held before (`"Pending"`, `"Failed"`, etc.). A cancelled document with `status == "Failed"` will look like it is still actionable in list views filtered by status, and any logic that queries `status == "Cancelled"` (e.g., `repost()` early-return guard) will never match a normally cancelled document.
Add `def on_cancel(self): self.db_set("status", "Cancelled")` to ensure the status field stays in sync with docstatus.
How can I resolve this? If you propose a fix, please make it concise.| if repost_doc.docstatus != 1 or repost_doc.status in ["In Progress", "Completed", "Cancelled"]: | ||
| repost_doc.db_set("status", "Pending") | ||
| return |
There was a problem hiding this comment.
When the background job's docstatus != 1 branch fires (e.g., the document was cancelled between start_repost and the job actually executing), repost_doc.db_set("status", "Pending") is called on a document that may have docstatus == 2. This writes status = "Pending" onto a cancelled document, contradicting the "Cancelled" status that on_cancel (once added) or on_discard would have set. The early-return is correct, but the status field should not be modified in this path — just return without touching it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.py
Line: 225-227
Comment:
**Status incorrectly reset to "Pending" on non-submitted documents**
When the background job's `docstatus != 1` branch fires (e.g., the document was cancelled between `start_repost` and the job actually executing), `repost_doc.db_set("status", "Pending")` is called on a document that may have `docstatus == 2`. This writes `status = "Pending"` onto a cancelled document, contradicting the "Cancelled" status that `on_cancel` (once added) or `on_discard` would have set. The early-return is correct, but the status field should not be modified in this path — just return without touching it.
How can I resolve this? If you propose a fix, please make it concise.
|
Can we add reposting for the cancelled voucher? |
Thanks for highlighting. Will add this use case too. |
If the invoice is cancelled but gl entry is still present, that means there is some bug in the system. We should investigate it. It shouldn't be correcting via repost ledger tool? |
3 participants
The current workflow lacks user feedback on reposting success, making it difficult to trace failures, trying to improve that.