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: use batch, serial in future sle check for neg qty #25675

Closed
wants to merge 11 commits into from
Closed

fix: use batch, serial in future sle check for neg qty #25675

wants to merge 11 commits into from

Conversation

18alantom
Copy link
Member

@18alantom 18alantom commented May 12, 2021 •
edited
Loading

Description

  1. Uses batch and serial numbers filters to check future Stock Ledger Entries for negative values.
  2. Checks future SLE for negative values for matching warehouse and batch.

@18alantom 18alantom force-pushed the fix-use-batch-serial-for-future-neg-checks branch from 0745669 to 1ad87a2 Compare May 12, 2021 10:18
@18alantom 18alantom force-pushed the fix-use-batch-serial-for-future-neg-checks branch from 1ad87a2 to a59fae4 Compare May 12, 2021 13:45
nabinhait
nabinhait requested changes May 14, 2021
View reviewed changes
erpnext/stock/stock_ledger.py Outdated Show resolved Hide resolved
@18alantom 18alantom force-pushed the fix-use-batch-serial-for-future-neg-checks branch from bc6aea5 to bcf1dc3 Compare May 14, 2021 07:37
@18alantom 18alantom requested a review from nabinhait May 14, 2021 17:49
@marination marination added the needs-tests This PR needs automated unit-tests. label May 26, 2021
@marination
Copy link
Collaborator

@18alantom add tests for this ?

@stale
Copy link

stale bot commented Jul 20, 2021

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 Jul 20, 2021
@stale stale bot closed this Jul 23, 2021
@marination marination reopened this Jul 26, 2021
@stale stale bot removed the inactive label Jul 26, 2021
@18alantom 18alantom requested a review from ankush as a code owner July 27, 2021 05:44
@18alantom 18alantom force-pushed the fix-use-batch-serial-for-future-neg-checks branch from 1c8ea50 to 55b1f08 Compare July 29, 2021 05:12
@18alantom 18alantom force-pushed the fix-use-batch-serial-for-future-neg-checks branch from 3487014 to 45c862d Compare August 9, 2021 13:54
@18alantom
Copy link
Member Author

This PR was spuriously closed in the great PR purge of August 2021.

Many a test has been patched to get CI to pass but to no avail, new patches fail!
Fault lies not with the PR, but with the tests that do not account for future negative quantities.

@18alantom 18alantom reopened this Sep 16, 2021
@18alantom 18alantom added fix-conflicts and removed needs-tests This PR needs automated unit-tests. labels Sep 17, 2021
ankush
ankush requested changes Sep 21, 2021
View reviewed changes
erpnext/accounts/doctype/pos_closing_entry/test_pos_closing_entry.py
Comment on lines +29 to +30
is_allow_neg = frappe.db.get_single_value('Stock Settings', 'allow_negative_stock')
frappe.db.set_value('Stock Settings', 'Stock Settings', 'allow_negative_stock', 1)
Copy link
Member

@ankush ankush Sep 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Way too much duplicate code, should be a decorator instead, or if it applies class-wide then setupclass/teardownclass.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@18alantom can you remove as many of these as possible? Let's try to get this merged without toggling of such flags 😅

If required change the test case to ERPNextTestCase to stop committing in other tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup for sure, which is what I wanted to do almost as soon as I flipped the flag.

erpnext/stock/doctype/stock_entry/test_stock_entry.py
Comment on lines +880 to +881
illegal_sequence_executor = create_stock_entry_sequence_executor(illegal_sequence)
self.assertRaises(frappe.ValidationError, illegal_sequence_executor)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
illegal_sequence_executor = create_stock_entry_sequence_executor(illegal_sequence)
self.assertRaises(frappe.ValidationError, illegal_sequence_executor)
self.assertRaises(frappe.ValidationError, create_stock_entries, illegal_sequence)

args, kwargs can be passed to it directly, no need to create a partially applied funciton here.

erpnext/stock/doctype/stock_entry/test_stock_entry.py
Comment on lines +978 to +980
def create_stock_entry_sequence_executor(sequence_of_entries):
return lambda: create_stock_entries(sequence_of_entries)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def create_stock_entry_sequence_executor(sequence_of_entries):
return lambda: create_stock_entries(sequence_of_entries)

Not required.

@stale
Copy link

stale bot commented Oct 6, 2021

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 Oct 6, 2021
@stale stale bot closed this Oct 9, 2021
@18alantom 18alantom reopened this Oct 11, 2021
@stale stale bot removed the inactive label Oct 11, 2021
@18alantom 18alantom changed the base branch from version-13-hotfix to develop October 13, 2021 10:46
@18alantom 18alantom changed the base branch from develop to version-13-hotfix October 13, 2021 10:50
@18alantom 18alantom mentioned this pull request Oct 13, 2021
Closed
@18alantom
Copy link
Member Author

Closed cause #27945

@18alantom 18alantom closed this Oct 13, 2021
@18alantom 18alantom deleted the fix-use-batch-serial-for-future-neg-checks branch October 13, 2021 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nabinhait nabinhait nabinhait requested changes

@ankush ankush ankush requested changes

@marination marination Awaiting requested review from marination

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

@surajshetty3416 surajshetty3416 Awaiting requested review from surajshetty3416

Assignees

@rohitwaghchaure rohitwaghchaure

Labels
fix-conflicts hotfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@18alantom @marination @nabinhait @rohitwaghchaure @ankush