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

[14.0][FIX] delivery_schenker: round up package volume + weight #678

Closed
wants to merge 1 commit into from
Closed

[14.0][FIX] delivery_schenker: round up package volume + weight #678

wants to merge 1 commit into from

Conversation

hildickethan
Copy link
Member

@hildickethan hildickethan commented Jul 20, 2023 •
edited
Loading

  • Round up Schenker package volume
    • The Schenker API requires a volume to be non-zero, but also forces 2 decimal places.
    • The 0.01 minimum was removed here [14.0][IMP] delivery schenker and delivery_schenker_picking_volume #675 (comment) to avoid fake volumes when incorrectly configured, but we must still be able to deliver cases where the volume is non-zero but rounds to 0.00.
    • I think it's also safer to give an overestimate on the volume to the carrier, rather than an underestimate (though its only going to be miniscule volume differences)
    • I added float(float_repr()) to the float_round to avoid trailing decimals that can happen sometimes, in my case it was rounding 2.0086 to 2.01 would result in 2.0100000000000002 which errored
  • Same for weight

@hildickethan hildickethan changed the title [FIX] delivery_schenker: round up package volume [14.0][FIX] delivery_schenker: round up package volume Jul 20, 2023
@hildickethan hildickethan force-pushed the 14.0-schenker_round_up branch 3 times, most recently from 36dcaa4 to 3fbd67b Compare July 25, 2023 09:38
@hildickethan hildickethan changed the title [14.0][FIX] delivery_schenker: round up package volume [14.0][FIX] delivery_schenker: round up package volume + weight Sep 18, 2023
@mt-software-de
Copy link
Contributor

@hildickethan-S73 https://github.com/OCA/delivery-carrier/pull/710/files :-)

mt-software-de
mt-software-de approved these changes Nov 6, 2023
View reviewed changes
Copy link
Contributor

@mt-software-de mt-software-de left a comment

Choose a reason for hiding this comment

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

Minor remarks

delivery_schenker/models/delivery_carrier.py Outdated Show resolved Hide resolved
@mt-software-de mt-software-de mentioned this pull request Nov 6, 2023
Merged
@hildickethan
[FIX] delivery_schenker: round up package volume and weight
a516a34 
The Schenker API requires a volume to be non-zero, but also forces 2 decimal places.
The 0.01 minimum was removed for avoiding fake volumes when incorrectly configured, but we must still round up cases where the volume is non-zero but rounds to 0.00.
It is also logical to give an overestimate on the volume to the carrier, rather than an underestimate.

round -> repr -> float to avoid Python floating decimals that error when validating with the Schema.
float_round(2.086, precision_digits=2) = 2.0100000000000002
float_repr(float_round(2.086, precision_digits=2), 2) = '2.01' (string)
float(float_repr(float_round(2.086, precision_digits=2), 2)) = 2.01
@hildickethan hildickethan force-pushed the 14.0-schenker_round_up branch from 3b5226c to a516a34 Compare November 6, 2023 14:35
@mt-software-de
Copy link
Contributor

@jbaudoux can you review and merge this one?

jbaudoux
jbaudoux reviewed Dec 11, 2023
View reviewed changes
delivery_schenker/models/delivery_carrier.py
round -> repr -> float to avoid extra decimals that error with the Schema.
float_round(2.086, precision_digits=2) = 2.0100000000000002
"""
return float(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why reconvert back to float? You will get back extra decimals:

print('{0:.16f}'.format(float("2.09")))
2.0899999999999999

SOAP is transmitting in XML, so converting it all to text, so once you have the representation with 2 decimals, you better send that, you don't need a real float in your dict.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this was just to maintain the same type as before, but I see what you mean with it being pointless

delivery_schenker/models/delivery_carrier.py
The schenker api requires 2 decimal points.
Round up to avoid 0 if <0.005.
round -> repr -> float to avoid extra decimals that error with the Schema.
float_round(2.086, precision_digits=2) = 2.0100000000000002
Copy link
Contributor

Choose a reason for hiding this comment

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

Your example doesn't look right

Suggested change
float_round(2.086, precision_digits=2) = 2.0100000000000002
float_round(2.086, precision_digits=2) = 2.0899999999999999

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like I wrote the comment wrong, in the PR description the issue was 2.0086, not 2.086
image
I will change it

delivery_schenker/models/delivery_carrier.py
Comment on lines +365 to +366
round -> repr -> float to avoid extra decimals that error with the Schema.
float_round(2.086, precision_digits=2) = 2.0100000000000002
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
round -> repr -> float to avoid extra decimals that error with the Schema.
float_round(2.086, precision_digits=2) = 2.0100000000000002
Returns the string representation with the given precision

@mt-software-de
Copy link
Contributor

@hildickethan-S73 https://github.com/OCA/delivery-carrier/pull/710/files :-)

@jbaudoux we can reopen this of course.

@jbaudoux
Copy link
Contributor

@hildickethan-S73 can we close this PR in favor of #710 as it solves it properly ?

@hildickethan
Copy link
Member Author

@hildickethan-S73 can we close this PR in favor of #710 as it solves it properly ?

If we can merge it yes, I need the change suggested in #710 (comment) though since it was in this PR

@github-actions GitHub Actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Apr 14, 2024
@hildickethan
Copy link
Member Author

closed in favor of #710

@hildickethan hildickethan deleted the 14.0-schenker_round_up branch May 8, 2024 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbaudoux jbaudoux jbaudoux left review comments

@mt-software-de mt-software-de mt-software-de approved these changes

Assignees
No one assigned
Labels
stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hildickethan @mt-software-de @jbaudoux