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

fix: Respect system precision for user facing balance qty values #30837

Merged
merged 8 commits into from
Jun 17, 2022

Conversation

marination
Copy link
Collaborator

@marination marination commented Apr 28, 2022

Issue:

System Precision: 3

Consider the following ledger:

Qty Diff Bal Qty (for users) Bal Qty (in db)
0 559.833 559.8327
-470.84 88.993 88.9927
  • Now try subtracting 100 from balance (post a Sales Invoice), it says qty is insufficient and it needs more (100-88.9927) 11.0073 Qty
  • The deficiency shown to users does not respect system precision. It could have 7 places after the decimal too. Really bad UX

Solution:

  • Round off qty deficiency in system precision so that the negative stock validation considers rounded off precision as thr right value and users can adjust stock as per this value
  • E.g. Here users will be prompted to add more 11.007 qty instead of 11.0073
  • The Ledger will now look like:
Qty Diff Bal Qty (for users) Bal Qty (in db)
0 559.833 559.8327
-470.84 88.993 88.9927
11.007 100.000 99.9997
-100.000 0.000 -0.0003

To the reviewer:

The diff of -0.0002999999999957481 in the last SLE will be rounded up to 0.000 and so no error is raised since diff is 0. This will however technically causes some negative stock (that again if rounded becomes 0)

Todo:

  • Adjust precision for future negative qty validation
  • Tests

- `get_precision` -> `set_precision`
- Use system wide currency precision for `stock_value`
- Round of qty defiiciency as per user defined precision (system flt precision), so that it is WYSIWYG for users
@github-actions github-actions bot added needs-tests This PR needs automated unit-tests. stock labels Apr 28, 2022
@codecov
Copy link

codecov bot commented Apr 28, 2022

Codecov Report

Merging #30837 (9194665) into develop (1a3997a) will increase coverage by 0.56%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop   #30837      +/-   ##
===========================================
+ Coverage    63.06%   63.63%   +0.56%     
===========================================
  Files          984      984              
  Lines        67556    67570      +14     
===========================================
+ Hits         42604    42996     +392     
+ Misses       24952    24574     -378     
Impacted Files Coverage Δ
erpnext/stock/doctype/stock_entry/stock_entry.py 81.25% <ø> (ø)
erpnext/stock/stock_ledger.py 90.57% <100.00%> (+0.12%) ⬆️
...eport/item_variant_details/item_variant_details.py 31.32% <0.00%> (-53.02%) ⬇️
...t/product_bundle_balance/product_bundle_balance.py 79.54% <0.00%> (-15.91%) ⬇️
...enefit_application/employee_benefit_application.py 64.49% <0.00%> (-8.29%) ⬇️
...e/asset_value_adjustment/asset_value_adjustment.py 86.04% <0.00%> (-3.49%) ⬇️
.../report/stock_projected_qty/stock_projected_qty.py 86.84% <0.00%> (-1.32%) ⬇️
erpnext/stock/reorder_item.py 74.16% <0.00%> (-0.84%) ⬇️
...xt/e_commerce/doctype/website_item/website_item.py 47.52% <0.00%> (-0.42%) ⬇️
erpnext/hr/utils.py 76.40% <0.00%> (-0.38%) ⬇️
... and 34 more

@stale
Copy link

stale bot commented May 13, 2022

This pull request has been automatically marked as inactive because it has not had recent activity. It will be closed within 3 days 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 May 13, 2022
@stale stale bot closed this May 16, 2022
@ankush ankush reopened this May 17, 2022
@stale stale bot removed the inactive label May 17, 2022
@marination
Copy link
Collaborator Author

The test is getting a bit hard. Values set are not honouring system precision, will take a look at EOW

@stale
Copy link

stale bot commented Jun 7, 2022

This pull request has been automatically marked as inactive because it has not had recent activity. It will be closed within 3 days 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 Jun 7, 2022
- Test for Immediate Negative Qty precision
- Stock Entry Negative Qty message: Format available qty in system precision
- Pass `stock_uom` as confugrable option in `make_item`
@marination marination removed the needs-tests This PR needs automated unit-tests. label Jun 16, 2022
@marination marination marked this pull request as ready for review June 16, 2022 09:15
@ankush ankush self-assigned this Jun 17, 2022
- `get_field_precision` defaults to number format for precision (maintain old behaviour)
- Don't pass `currency` to `get_field_precision` as its not used anymore
@ankush ankush merged commit d6078aa into frappe:develop Jun 17, 2022
mergify bot pushed a commit that referenced this pull request Jun 17, 2022
)

* fix: Respect system precision for user facing balance qty values

- `get_precision` -> `set_precision`
- Use system wide currency precision for `stock_value`
- Round of qty defiiciency as per user defined precision (system flt precision), so that it is WYSIWYG for users

* fix: Consider system precision when validating future negative qty

* test: Immediate Negative Qty precision test

- Test for Immediate Negative Qty precision
- Stock Entry Negative Qty message: Format available qty in system precision
- Pass `stock_uom` as confugrable option in `make_item`

* test: Future Negative Qty validation with precision

* fix: Use `get_field_precision` for currency precision as it used to

- `get_field_precision` defaults to number format for precision (maintain old behaviour)
- Don't pass `currency` to `get_field_precision` as its not used anymore

(cherry picked from commit d6078aa)

# Conflicts:
#	erpnext/stock/doctype/stock_ledger_entry/test_stock_ledger_entry.py
marination added a commit that referenced this pull request Jun 21, 2022
…-30837

fix: Respect system precision for user facing balance qty values (backport #30837)
frappe-pr-bot pushed a commit that referenced this pull request Jun 28, 2022
## [13.34.2](v13.34.1...v13.34.2) (2022-06-28)

### Bug Fixes

* add UOM validation for planned-qty ([559bde3](559bde3))
* dont update RM items table if not required (backport [#31408](#31408)) ([#31457](#31457)) ([8155306](8155306))
* General Ledger and TB opening entries mismatch issues ([a0c5c73](a0c5c73))
* Monthly depreciation using WDV method ([e7659a1](e7659a1))
* Quotation and Sales Order item sync ([2219132](2219132))
* Respect system precision for user facing balance qty values ([#30837](#30837)) ([642b9c5](642b9c5))
* **Salary Slip:** Components not updated when amount evaluates to 0 due to payment days ([#31425](#31425)) ([abfe926](abfe926))
* translation for filter status on report ([736f206](736f206))
* update ru translate (backport [#31404](#31404)) ([#31417](#31417)) ([8b78a12](8b78a12))
@frappe frappe deleted a comment from yhx5773489 Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants