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

Round prices to two decimal places in price comparison check in basket #4221

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fortelll
Copy link

@fortelll fortelll commented Jan 7, 2024

Fixed price comparison by rounding to two decimal places, preventing misleading cart notifications when prices are effectively the same. Previously, the comparison between these two prices was done directly, which caused issues when the number of decimal places differed.

Fix price comparison by rounding to two decimal places, preventing misleading cart notifications when prices are effectively the same.
Previously, the comparison between these two prices was done directly, which caused issues when the number of decimal places differed.
Copy link

codecov bot commented Jan 9, 2024

Codecov Report

Merging #4221 (05d1887) into master (5370843) will not change coverage.
Report is 23 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4221   +/-   ##
=======================================
  Coverage   87.81%   87.81%           
=======================================
  Files         291      291           
  Lines       16145    16145           
=======================================
  Hits        14177    14177           
  Misses       1968     1968           
Files Coverage Δ
src/oscar/apps/basket/abstract_models.py 91.66% <100.00%> (ø)

Copy link

@aekespong aekespong left a comment

Choose a reason for hiding this comment

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

Minor change that fixes this bug.

@viggo-devries
Copy link
Contributor

@fortelll please use round_half_up like everywhere else in the basket.

Could you please add a test that shows this failed before?

@viggo-devries viggo-devries added the Needs tests This pull request does not have sufficient tests label Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs tests This pull request does not have sufficient tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants