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: Incorrect packing list for recurring items & code cleanup #29456

Merged
merged 8 commits into from
Feb 3, 2022
Merged

fix: Incorrect packing list for recurring items & code cleanup #29456

merged 8 commits into from
Feb 3, 2022

Conversation

marination
Copy link
Collaborator

@marination marination commented Jan 25, 2022 •
edited
Loading

Introduced via #27124

Issues:

1) Recurring Items in Sales transactions

  • Add two same bundle items in a delivery note on different rows, with different quantities
  • The packed items table only shows packed items for the first row and ignores the second recurring item row

2) Code comprehensibility

  • Long functions
  • looping over the packed items table over and over again
  • Hard to understand/overly hacky logic due to confusing function names that do a lot more than stated

Fix:

  • Consider recurring bundle items
  • make_packing_list in packed_item.py should give a clear overview of the sequence of events
  • smaller functions and lesser nested calls
  • reset table if any deletion takes place. No. of loops to regenerate/update the table is the same. Deleting rows separately and looping again adds to the complexity.
  • Get rate of packed item from price list. Fallback on Item master's valuation_rate if no price is set as per price list

ToDo:

  • Rewrite price updation logic and add price fetching for packed items
  • Tests

To test:

  • Test product bundle addition/updation/deletion via normal form and update items
  • Can test in SO and DN, behaviour is same throughout
  • Test recurring items test case
  • Test packed items that have batches/serial nos too
  • Test enabling "calculate bundle price from packed items" in Selling Settings. Check how packed items price is fetched firstly. Earlier it was blindly pulled from Item master. Now it will fetch based on price list rate/buying price and fallback on Item master's valuation_rate

@marination
fix: Incorrect packing list for recurring items & code cleanup
3f48fc1 
- Fix Incorrect packing list for recurring items in the Items table
- Re-organised functions based on external use and order of use
- Deleted `clean_packing_list` function and reduced no.of loops
- Raw SQL to QB
- Minor formatting changes
@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Jan 25, 2022
@marination marination added needs-tests This PR needs automated unit-tests. and removed needs-tests This PR needs automated unit-tests. labels Jan 25, 2022
@marination
chore: SQL to QB & accomodate Update Items
f8a5786 
- `doc_before_save` does not exist via Update Items (updates stuff in the backend so doc isn't considered unsaved/dirty)
- converted more raw sql to qb and ORM
@marination marination force-pushed the product-bundle-packing-list-logic branch from 174ac88 to a925165 Compare January 25, 2022 19:30
@marination
fix: Linter and minor code refactor
4c677ea 
- Create an indexed map of stale packed items table to avoid loops to check if packed item row exists
- Reset packed items if row deletion takes place
- Renamed functions to self-explain them
- Split long function
- Reduce function calls inside function (makes it harder to follow through)
@marination marination force-pushed the product-bundle-packing-list-logic branch from a925165 to 4c677ea Compare January 25, 2022 19:41
@ankush ankush mentioned this pull request Jan 25, 2022
Open
@marination
chore: Break updation logic into smaller functions
2c14ab0 
- Smaller functions for updation
- All calls visible from parent function to avoid context switching due to nested calls
@codecov
Copy link

codecov bot commented Jan 28, 2022 •
edited
Loading

Codecov Report

Merging #29456 (f18ec2d) into develop (9b60e3f) will increase coverage by 18.21%.
The diff coverage is 87.80%.

@@             Coverage Diff              @@
##           develop   #29456       +/-   ##
============================================
+ Coverage    40.58%   58.80%   +18.21%     
============================================
  Files         1107     1108        +1     
  Lines        69136    69292      +156     
============================================
+ Hits         28058    40745    +12687     
+ Misses       41078    28547    -12531
Impacted Files Coverage Δ
erpnext/stock/doctype/packed_item/packed_item.py 87.12% <87.80%> (+3.64%) ⬆️
...em_wise_sales_register/item_wise_sales_register.py 51.81% <0.00%> (-11.37%) ⬇️
...t/accounts/report/sales_register/sales_register.py 73.33% <0.00%> (-7.28%) ⬇️
...pnext/accounts/report/gross_profit/gross_profit.py 82.49% <0.00%> (-4.29%) ⬇️
.../report/bom_operations_time/bom_operations_time.py 89.13% <0.00%> (-2.18%) ⬇️
.../report/accounts_receivable/accounts_receivable.py 71.79% <0.00%> (-0.22%) ⬇️
erpnext/e_commerce/product_data_engine/query.py 75.53% <0.00%> (ø)
erpnext/accounts/doctype/pricing_rule/utils.py 70.82% <0.00%> (+0.26%) ⬆️
...ype/account/chart_of_accounts/chart_of_accounts.py 78.08% <0.00%> (+0.68%) ⬆️
..._scorecard_variable/supplier_scorecard_variable.py 25.00% <0.00%> (+0.73%) ⬆️
... and 343 more

@marination
refactor: Price fetching and updation logic
2f4d266 
- fetch price from price list, use item master valuation rate as fallback fo0r packed item
- use a item code, item row name map to maintain cumulative price
- reset table if item in a row is replaced
- loop over items table only to set price, lesser iterations than packed items table
@marination
test: Packed Items test file
4e7b4dc
@marination marination marked this pull request as ready for review February 3, 2022 11:32
@marination marination added this to the v13.20 milestone Feb 3, 2022
@marination marination removed the needs-tests This PR needs automated unit-tests. label Feb 3, 2022
@marination marination merged commit 79ab8e6 into frappe:develop Feb 3, 2022
@marination
Copy link
Collaborator Author

@Mergifyio backport version-13-hotfix

@mergify mergify bot mentioned this pull request Feb 3, 2022
Merged
@mergify
Copy link
Contributor

mergify bot commented Feb 3, 2022

backport version-13-hotfix

✅ Backports have been created

@mergify mergify bot mentioned this pull request Feb 3, 2022
Merged
marination added a commit that referenced this pull request Feb 4, 2022
@marination
Merge pull request #29628 from frappe/mergify/bp/version-13-hotfix/pr…
51f9f42 
…-29456

fix: Incorrect packing list for recurring items & code cleanup (backport #29456)
marination added a commit that referenced this pull request Feb 4, 2022
@marination
Merge pull request #29629 from frappe/mergify/bp/version-13-pre-relea…
e095e27 
…se/pr-29456

fix: Incorrect packing list for recurring items & code cleanup (backport #29456)
@yadavyk yadavyk mentioned this pull request Feb 4, 2022
Closed
@rohitwaghchaure rohitwaghchaure mentioned this pull request Feb 9, 2022
Merged
@rohitwaghchaure rohitwaghchaure mentioned this pull request Feb 21, 2022
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ankush ankush Awaiting requested review from ankush

@nextchamp-saqib nextchamp-saqib Awaiting requested review from nextchamp-saqib

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

Assignees
No one assigned
Labels
backport version-13-hotfix
Projects
None yet
Milestone
v13.20
Development

Successfully merging this pull request may close these issues.
2 participants
@marination @Citysystems-smart