Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Add exception handling in background job within BOM Update Tool #30146

Merged
merged 17 commits into from
Mar 31, 2022

Conversation

marination
Copy link
Collaborator

@marination marination commented Mar 9, 2022

List View throws an error due to frappe/frappe#16302 (its still usable)

Issue:

  • BOM Update Tool jobs run in the background, there's no way to know about failures, if any (due to timeout or bug)
  • This PR adds error logging and enhances the UX

Working:

  • The Replace button is inactive until both BOMs are mentioned. On clicking on Replace, a BOM Update Log is created. This log maintains state and lets user know of failure.
    2022-03-17 13 01 58

  • This log has two types: Replace BOM and Update Cost. Each is self explanatory as to what job is being executed.

  • List View:
    Screenshot 2022-03-31 at 12 53 55 PM

  • Failed Log:
    Screenshot 2022-03-17 at 1 05 44 PM

  • Completed Log
    Screenshot 2022-03-17 at 1 07 03 PM

  • Failed Log with simulated timeout error:
    2022-03-17 17 13 14

Todo:

  • Add BOM Update Log with status
  • Tests for Log

@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Mar 9, 2022
@marination marination removed the needs-tests This PR needs automated unit-tests. label Mar 9, 2022
@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #30146 (cd9d070) into develop (901d8ea) will increase coverage by 0.04%.
The diff coverage is 83.65%.

@@             Coverage Diff             @@
##           develop   #30146      +/-   ##
===========================================
+ Coverage    60.90%   60.94%   +0.04%     
===========================================
  Files         1082     1083       +1     
  Lines        69078    69111      +33     
===========================================
+ Hits         42071    42120      +49     
+ Misses       27007    26991      -16     
Impacted Files Coverage Δ
erpnext/hooks.py 100.00% <ø> (ø)
erpnext/manufacturing/doctype/bom/bom.py 87.48% <ø> (-0.14%) ⬇️
...cturing/doctype/bom_update_tool/bom_update_tool.py 81.25% <80.00%> (+7.56%) ⬆️
...facturing/doctype/bom_update_log/bom_update_log.py 84.52% <84.52%> (ø)
...e/asset_value_adjustment/asset_value_adjustment.py 86.04% <0.00%> (-3.49%) ⬇️
erpnext/stock/doctype/warehouse/warehouse.py 84.42% <0.00%> (-2.46%) ⬇️
...saction/incorrect_balance_qty_after_transaction.py 97.67% <0.00%> (-2.33%) ⬇️
erpnext/crm/doctype/prospect/prospect.py 56.71% <0.00%> (-1.50%) ⬇️
erpnext/controllers/sales_and_purchase_return.py 85.13% <0.00%> (-0.75%) ⬇️
...rpnext/stock/report/stock_balance/stock_balance.py 78.10% <0.00%> (-0.60%) ⬇️
... and 16 more

marination and others added 8 commits March 16, 2022 19:47
- Created BOM Update Log that will handle queued job status and failures
- Moved validation and BG job to thus new doctype
- BOM Update Tool only works as an endpoint
- Added Typing
- Moved all job business logic to bom update log
- Added `run_bom_job` that handles errors and runs either of two methods
- UX: Replace button disabled until both inputs are filled
- Show log creation message on UI for correctness
- APIs return log document as result
- Converted raw sql to QB
- test creation of log and it's impact
@marination marination marked this pull request as ready for review March 30, 2022 07:32
- Explain explicit commits and skip semgrep
- Format client side translated string correctly
- Renamed public function`update_new_bom` to `update_new_bom_in_bom_items`
- Replaced `get_cached_doc` with `get_doc`
- Removed click progress bar (drive through update log)
- Removed `bom_obj.update_new_bom()`, was redundant. Did same job as `update_new_bom_in_bom_items`
- Removed `update_new_bom()` in `bom.py`, unused.
- Prettier query formatting
- `update_type` annotated as non optional Literal
- Removed redundant use of JobTimeoutException
- Corrected type annotations in `create_bom_update_log()`
- Covers full validate function
@marination
Copy link
Collaborator Author

@ankush as discussed will fix cost updation inconsistencies in a different PR. Good to go for now ?

@ankush
Copy link
Member

ankush commented Mar 31, 2022

unrelated test failing.

@marination
Copy link
Collaborator Author

Fixed the failing test in #30509

@marination marination merged commit 96fc6ad into frappe:develop Mar 31, 2022
marination added a commit that referenced this pull request Apr 6, 2022
…-30146

refactor: Add exception handling in background job within BOM Update Tool (backport #30146)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants