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

feat(delivery): add cutoff item date for so delivery items #38561

Merged

Conversation

blaggacao
Copy link
Collaborator

@blaggacao blaggacao commented Dec 4, 2023

#38053 still addresses a very common use case and a current flaw in the end-2-end buisness workflow.

no-docs

@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Dec 4, 2023
Copy link

stale bot commented Dec 20, 2023

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 inactive and removed inactive labels Dec 20, 2023
Copy link

stale bot commented Jan 5, 2024

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 inactive and removed inactive labels Jan 5, 2024
Copy link

stale bot commented Jan 21, 2024

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 inactive and removed inactive labels Jan 21, 2024
@blaggacao

This comment was marked as resolved.

@ruthra-kumar ruthra-kumar self-assigned this Jan 23, 2024
@blaggacao blaggacao force-pushed the feat/add-delivery-cutoff-date-on-so branch from 2931fa8 to 252fae6 Compare January 23, 2024 16:46
Copy link

codecov bot commented Jan 23, 2024

Codecov Report

Attention: Patch coverage is 11.76471% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 60.20%. Comparing base (bdca718) to head (9a70516).
Report is 296 commits behind head on develop.

❗ Current head 9a70516 differs from pull request most recent head b8dac84. Consider uploading reports for the commit b8dac84 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #38561      +/-   ##
===========================================
- Coverage    60.20%   60.20%   -0.01%     
===========================================
  Files          757      757              
  Lines        70811    70822      +11     
===========================================
+ Hits         42635    42636       +1     
- Misses       28176    28186      +10     
Files Coverage Δ
erpnext/selling/doctype/sales_order/sales_order.py 68.29% <33.33%> (-0.28%) ⬇️
erpnext/utilities/bulk_transaction.py 0.00% <0.00%> (ø)

@blaggacao

This comment was marked as outdated.

@blaggacao

This comment was marked as outdated.

Copy link
Contributor

mergify bot commented Jan 30, 2024

update

✅ Branch has been successfully updated

@ruthra-kumar
Copy link
Member

@blaggacao
If there are no items within the cut-off date, then a delivery note with empty items table is created. User can unintentionally create lot of junk delivery notes.

so_to_delivery_note.webm

@blaggacao blaggacao force-pushed the feat/add-delivery-cutoff-date-on-so branch from 84fdd4f to 9a70516 Compare January 30, 2024 12:51
@blaggacao
Copy link
Collaborator Author

blaggacao commented Jan 30, 2024

@ruthra-kumar Thanks a lot for the friendly QA! I worked around this issue in the case of a bulk transaction with the amendment in 9a70516.

I've tested in on a local database and it appears to behave as intended, now. 🤝

I thought it might be good for performance on large bulk transactions to not depend on the python GC, hence: del obj (cc @ankush for my education: is manually freeing memory a valid thought, at all?)

@blaggacao
Copy link
Collaborator Author

blaggacao commented Feb 9, 2024

@ruthra-kumar I've been busy myself this week, but maybe we shouldn't let this dry out? I think it's ready.

@ruthra-kumar
Copy link
Member

Will take a look at it this week.

@blaggacao
Copy link
Collaborator Author

@ruthra-kumar maybe we can get this merged this week, still? 🙏

@ruthra-kumar
Copy link
Member

@blaggacao
Code looks ok. No junk Delivery notes now.

I think, making this a toggleable feature would be better. Add an option in Selling Settings and give the cut-off date popup only if enabled(?)

@blaggacao
Copy link
Collaborator Author

@ruthra-kumar added in: b8dac84

@ruthra-kumar
Copy link
Member

semgrep failure is unrelated.Ignoring.

@ruthra-kumar ruthra-kumar merged commit e017421 into frappe:develop Mar 2, 2024
17 of 18 checks passed
@blaggacao blaggacao deleted the feat/add-delivery-cutoff-date-on-so branch March 2, 2024 09:58
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2024
@ruthra-kumar
Copy link
Member

When backporting include #40509

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
defer backport needs-tests This PR needs automated unit-tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants