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

fix: service name and carrier name parsing #672

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jacobshilitz
Copy link
Collaborator

this fixes the bug that currently eshipper returns itself as the carrier, and it's different from how it was in the eshipper lagecy XML API, and how other providers that provide multi carrier rates.

      "meta": {
        "carrier": "eshipper",
        "carrierId": "5000011",
        "carrierName": "Purolator",
        "carrier_connection_id": "car_4b3f387ad1fc4daca568756d12475346",
        "ext": "eshipper",
        "rate_provider": "eshipper",
        "serviceName": "Purolator Ground",
        "service_name": "PUROLATOR GROUND"
      },

@jacobshilitz
Copy link
Collaborator Author

don't merge before #675 is fixed, becouse in prod this will cause that no service name will show

@@ -29,6 +29,8 @@ def _extract_details(
service = provider_units.ShippingService.map(str(rate.serviceId))
carrierId = provider_units.ShippingService.carrier_id(service.value_or_key)
rate_provider = provider_units.ShippingService.carrier(service.value_or_key).lower()
service_name = service.name.replace("eshipper_", "")
Copy link
Member

Choose a reason for hiding this comment

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

The issue with this is it doesn't take into consideration API changes from eshipper.
If they introduce a new service or carrier that we don't have in the enum, service.name will be None causing an exception.

Essentially, we need to anticipate unexpected data as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

correct. I already fixed it on my local... becouse when testing with production it returned only carrier ID

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but I got overwhelmed with the mapping problem

Ensure `service_name` uses `rate.serviceName` as a fallback when `service.name` is not available. This prevents potential attribute errors and ensures consistent naming by replacing spaces with underscores.
@jacobshilitz
Copy link
Collaborator Author

@danh91 can you recommend what else we need to do to fix the mappings types?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants