Refactor UPS rate calculation logic #673
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Simplify the logic for determining total charges by consolidating conditional checks. Added BaseServiceCharge handling and mapped SurchargeType codes to descriptive names for improved readability.
danh91
reviewed
Sep 5, 2024
Improve the surcharge type mapping logic for readability and efficiency. Capitalize mapped surcharge type names and ensure consistent formatting by handling cases where the type name is not available.
Adjusted the base charge amounts and renamed surcharge codes to 'Fuel surcharge' in UPS rate tests. This change ensures consistency and accuracy in the test data for various UPS services. changed to use `BaseServiceCharge` instead of `TransportationCharges` because the docs state that `BaseServiceCharge` is The base value of the specific service for the shipment. This is equal to transportation charges - fuel surcharges. and the fuel charge has a separate itemized charge and appears as code 375
Deleted zero-amount extra charge entries from various test cases in the UPS rate tests. This streamlines the test cases and clarifies the relevant charges being tested.
danh91
approved these changes
Sep 11, 2024
There was a problem hiding this comment.
I am going to handle this. But for future reference, the schema python files are automatically generated based on the JSON schema files here so you want to make the change on the JSON file and regenerate not change directly the Python files
Comment on lines
+43
to
+59
| (rate.Service.Code, rate.ServiceOptionsCharges.MonetaryValue) | ||
| if lib.to_int(rate.ServiceOptionsCharges.MonetaryValue) > 0 | ||
| else [] | ||
| ), | ||
| *( | ||
| ( | ||
| ( | ||
| provider_units.SurchargeType.map( | ||
| str(getattr(c, "Code", None) or getattr(c, "Type", None)) | ||
| ).name.capitalize().replace("_", " ") | ||
| if provider_units.SurchargeType.map( | ||
| str(getattr(c, "Code", None) or getattr(c, "Type", None)) | ||
| ).name | ||
| else (getattr(c, "Code", None) or getattr(c, "Type", None)) | ||
| ), | ||
| c.MonetaryValue | ||
| ) |
There was a problem hiding this comment.
I will read this to see if it can be simplified. I might be hard to understand for the next person who will debug this 😅
There was a problem hiding this comment.
Thanks, also please look into this line, if we need this part at all, I allways saw it empty
*(
(rate.Service.Code, rate.ServiceOptionsCharges.MonetaryValue)
if lib.to_int(rate.ServiceOptionsCharges.MonetaryValue) > 0
else []
),
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Simplify the logic for determining total charges by consolidating conditional checks. Added BaseServiceCharge handling and mapped SurchargeType codes to descriptive names for improved readability.
See #672
ups, in fact, send the base rate of the negotiated rate, so putting the original rate does not make sense.