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

feat(pos): multiple item prices #33005

Merged
merged 14 commits into from Jan 28, 2023
Merged

feat(pos): multiple item prices #33005

merged 14 commits into from Jan 28, 2023

Conversation

ssiyad
Copy link
Contributor

@ssiyad ssiyad commented Nov 17, 2022

  • show uom with product price
  • multiple item (variant) depending on uom

@ssiyad
fix(pos): multiple item prices
a17af5a 
feat: show uom with product price
feat: multiple item (variant) depending on uom

Signed-off-by: Sabu Siyad <hello@ssiyad.com>
@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Nov 17, 2022
@ssiyad ssiyad linked an issue Nov 17, 2022 that may be closed by this pull request
Barcode wise UOM not working in Point of Sale #32922
Closed
@ssiyad ssiyad marked this pull request as ready for review November 28, 2022 07:40
@ssiyad ssiyad changed the title fix(pos): multiple item prices feat(pos): multiple item prices Nov 28, 2022
@codecov
Copy link

codecov bot commented Nov 28, 2022 •
edited

Codecov Report

Merging #33005 (f9f9cdc) into develop (3598bcc) will decrease coverage by 0.03%.
The diff coverage is 33.33%.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #33005      +/-   ##
===========================================
- Coverage    63.87%   63.85%   -0.03%     
===========================================
  Files          818      818              
  Lines        59119    59140      +21     
===========================================
- Hits         37764    37761       -3     
- Misses       21355    21379      +24
Impacted Files Coverage Δ
...rpnext/selling/page/point_of_sale/point_of_sale.py 43.90% <33.33%> (-8.55%) ⬇️

@ssiyad
feat(pos): uom based stock display
8029a99 
Signed-off-by: Sabu Siyad <hello@ssiyad.com>
@ssiyad
use // instead of math.floor()
6734b62 
Signed-off-by: Sabu Siyad <hello@ssiyad.com>
@ssiyad
feat(pos): uom by barcode
2a8ee02 
Signed-off-by: Sabu Siyad <hello@ssiyad.com>
@ssiyad
fix: replace is not with !=
90701d7 
Signed-off-by: Sabu Siyad <hello@ssiyad.com>
@ssiyad
fix(pos): barcode_info next(): fallback
b18ea64 
Signed-off-by: Sabu Siyad <hello@ssiyad.com>
@ssiyad
chore: format
404de1e 
Signed-off-by: Sabu Siyad <hello@ssiyad.com>
@barredterra barredterra added the squash Meant to tell reviewers that this PR should be squashed into a single commit while merging. label Dec 6, 2022
barredterra
barredterra reviewed Dec 6, 2022
View reviewed changes
erpnext/selling/page/point_of_sale/point_of_sale.py
item_code = result.get("item_code", search_term)
serial_no = result.get("serial_no", "")
batch_no = result.get("batch_no", "")
barcode = result.get("barcode", "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you aware that

  • {}.get("barcode", "") is equal to {}.get("barcode") or "", but
  • {"barcode": None}.get("barcode") or "" returns "" while {"barcode": None}.get("barcode", "") returns None?

So, your changes could produce None values, while the former code didn't. Is that what was intended here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Valid point, but search_for_serial_or_batch_or_barcode_number() (which use scan_barcode()) does not return explicit None values.

ssiyad and others added 2 commits December 6, 2022 14:44
@ssiyad @barredterra
Update erpnext/selling/page/point_of_sale/point_of_sale.py
dd78af7 
Co-authored-by: Raffael Meyer <14891507+barredterra@users.noreply.github.com>
@ssiyad
chore: un-ignore unused local variable
01d6d09 
Signed-off-by: Sabu Siyad <hello@ssiyad.com>
@stale
Copy link

stale bot commented Dec 22, 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 Dec 22, 2022
@stale stale bot closed this Dec 25, 2022
@barredterra barredterra reopened this Dec 25, 2022
@stale
Copy link

stale bot commented Jan 10, 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 the inactive label Jan 10, 2023
@deepeshgarg007
Copy link
Member

keep-alive

@stale stale bot removed the inactive label Jan 10, 2023
@deepeshgarg007 deepeshgarg007 merged commit 9e50aa4 into frappe:develop Jan 28, 2023
@Shram007 Shram007 mentioned this pull request Mar 14, 2023
Merged
@ankush ankush mentioned this pull request Oct 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@barredterra barredterra barredterra left review comments

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

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

@ruthra-kumar ruthra-kumar Awaiting requested review from ruthra-kumar ruthra-kumar is a code owner

Assignees
No one assigned
Labels
needs-tests This PR needs automated unit-tests. squash Meant to tell reviewers that this PR should be squashed into a single commit while merging.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Barcode wise UOM not working in Point of Sale
3 participants
@ssiyad @deepeshgarg007 @barredterra