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.

Sign up for GitHub

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

Jump to bottom

fix: Update Bin via Update Item on Purchase/Sales Order #23509

Merged
merged 8 commits into from
Mar 31, 2021
Merged

fix: Update Bin via Update Item on Purchase/Sales Order #23509

merged 8 commits into from
Mar 31, 2021

Conversation

AndyOverLord
Copy link
Contributor

@AndyOverLord AndyOverLord commented Oct 4, 2020 •
edited by marination
Loading

Issue:

  • On deletion of row Bin was not updated
  • Furthermore, in PO, Material Request and requested wasn't updated either on row deletion

Fix:

  • Explicitly update bin on row deletion in validate_and_delete_children
  • First validate if row can be deleted, if yes proceed to delete
  • Then update linked documents , like MR. Without these up to date Bin will get outdated values after recalculation. It recalculates from child table rows
  • Finally update requested & ordered / reserved qty in Bin
  • Added Tests
  • Re-ran patches to updates requested(MR), reserved(SO) and ordered(PO) qty
  • Broke row deletion logic into smaller functions

Miscellanous:

  • Made field Reserved Qty for subcontracting read only in Bin

@AndyOverLord
fix: Update Items on Purchase Order
7602489 
If user add rows or remove rows to update items on purchase order, the quantity in bin won't get updated.
This fix is not mature yet but to give an tempopary solution for fixing this issue.
@marination marination self-assigned this Oct 5, 2020
@AndyOverLord
fix: Update Items on Purchase Order
1062d7e 
1. set warehouse using `get_item_warehouse`
2. update "reserved_qty" for sales order
@marination marination added the needs-tests This PR needs automated unit-tests. label Oct 12, 2020
@marination
Copy link
Collaborator

@AndyOverLord can you add unit tests for the same ?

@AndyOverLord
Copy link
Contributor Author

@AndyOverLord can you add unit tests for the same ?

@marination Sorry, not sure how to add unit tests for this? Any instructions I can follow?

@AndyOverLord
Update accounts_controller.py
b44af32 
Updating Bin quantity based on doctype to optimize running efficiency.
@marination
Copy link
Collaborator

@AndyOverLord you can refer to test_sales_order.py, in that test_update_child_removing_item is a unit test for removing items. You can add a before and after reserved_qty check (assertEqual) there.
You can can add a before and after reserved_qty check in test_update_child_adding_new_item as well to increase test coverage.
The same in test_purchase_order.

@stale
Copy link

stale bot commented Dec 19, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within a week 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.

@stale stale bot added the inactive label Dec 19, 2020
@nabinhait
Copy link
Member

@marination @rohitwaghchaure Can you add test cases for this PR? Also need to write a patch for reposting reserved and ordered qty. This is actually critical, should be fixed asap.

@marination
Copy link
Collaborator

marination commented Mar 30, 2021 •
edited
Loading

@marination @rohitwaghchaure Can you add test cases for this PR? Also need to write a patch for reposting reserved and ordered qty. This is actually critical, should be fixed asap.

@nabinhait done

@marination
fix: Cleaned up and fixed validation and bin updation on deletion
0673f55 
- Created separate smaller functions for validation and bin updation of deleted row
- Made sure previous doc (linked doc) was also updated after deletion of row
- Tests to check bin updation on add/update/delete
- Made reserved qty for subcontrating read only in bin
@marination marination removed the needs-tests This PR needs automated unit-tests. label Mar 30, 2021
@marination marination mentioned this pull request Mar 30, 2021
Merged
@marination marination changed the title fix: Update Items on Purchase Order fix: Update Bin via Update Item on Purchase/Sales Order Mar 31, 2021
@marination
Copy link
Collaborator

Recent test added
Screenshot 2021-03-31 at 9 03 40 PM

@marination marination merged commit b4913e7 into frappe:develop Mar 31, 2021
@marination marination mentioned this pull request Jun 2, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marination marination marination requested changes

@deepeshgarg007 deepeshgarg007 Awaiting requested review from deepeshgarg007 deepeshgarg007 is a code owner

@rohitwaghchaure rohitwaghchaure Awaiting requested review from rohitwaghchaure rohitwaghchaure is a code owner

Assignees

@nabinhait nabinhait

@marination marination

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AndyOverLord @marination @nabinhait @rohitwaghchaure