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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: price list with 0 value are ignored #26655

Merged
merged 4 commits into from
Aug 9, 2021
Merged

Conversation

ankush
Copy link
Member

@ankush ankush commented Jul 26, 2021

Steps to reproduce:

  1. Create 2 item price for two different supplier. One of them should be
    zero.
  2. Create PO
  3. Add supplier with non-zero price and add item.
  4. change supplier. Price won't change. If price was non-zero it would've changed.

Root cause: falsy-ness check instead of null value check is used for checking if price list value exists. 0 is evaluated as false.

Fix: use null value check is None instead.

Also refactored the function to make it pure. Passing around dictionaries to update is like programming in assembly and passing around registers. 馃槃

Steps to reproduce:
1. Create 2 item price for two different supplier. One of them should be
   zero.
2. Create PO
3. Add supplier with non-zero price and add item.
4. change supplier. Price won't change. If price was non-zero it
   would've changed.

Root cause: falsiness check instead of null value check is used for
checking if price list value exists. 0 is evaluated as false.
@ankush ankush added the squash Meant to tell reviewers that this PR should be squashed into a single commit while merging. label Jul 26, 2021
@ankush ankush merged commit 16d4de5 into frappe:develop Aug 9, 2021
@frappe-pr-bot
Copy link
Collaborator

The backport to version-12-hotfix failed:

The process '/usr/bin/git' failed with exit code 1

frappe-pr-bot pushed a commit to frappe-pr-bot/erpnext that referenced this pull request Aug 9, 2021
* fix: price list with 0 value are ignored

Steps to reproduce:
1. Create 2 item price for two different supplier. One of them should be
   zero.
2. Create PO
3. Add supplier with non-zero price and add item.
4. change supplier. Price won't change. If price was non-zero it
   would've changed.

Root cause: falsiness check instead of null value check is used for
checking if price list value exists. 0 is evaluated as false.

* refactor: make get_price_list_rate function pure

(cherry picked from commit 16d4de5)
@ankush ankush deleted the price_list_zero branch August 9, 2021 05:21
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 42.698% when pulling fee90de on ankush:price_list_zero into 07e65ab on frappe:develop.

ankush added a commit that referenced this pull request Aug 9, 2021
* fix: price list with 0 value are ignored

Steps to reproduce:
1. Create 2 item price for two different supplier. One of them should be
   zero.
2. Create PO
3. Add supplier with non-zero price and add item.
4. change supplier. Price won't change. If price was non-zero it
   would've changed.

Root cause: falsiness check instead of null value check is used for
checking if price list value exists. 0 is evaluated as false.

* refactor: make get_price_list_rate function pure

(cherry picked from commit 16d4de5)

Co-authored-by: Ankush <ankush@iwebnotes.com>
asoral pushed a commit to asoral/erpnext that referenced this pull request Nov 12, 2021
* fix: price list with 0 value are ignored

Steps to reproduce:
1. Create 2 item price for two different supplier. One of them should be
   zero.
2. Create PO
3. Add supplier with non-zero price and add item.
4. change supplier. Price won't change. If price was non-zero it
   would've changed.

Root cause: falsiness check instead of null value check is used for
checking if price list value exists. 0 is evaluated as false.

* refactor: make get_price_list_rate function pure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport version-13-hotfix old-backport version-12-hotfix squash Meant to tell reviewers that this PR should be squashed into a single commit while merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants