verbb / postie

A Craft Commerce shipping calculator plugin.
Other
12 stars 18 forks source link

Postie forcing units of measurements (kg + cm) bug #46

Closed siebird closed 3 years ago

siebird commented 3 years ago

https://github.com/verbb/postie/blob/4c72de984ff05152fe7a3f12bb1309fb40718aab/src/providers/FedEx.php#L90

This one took a while to uncover as we were getting double the rates back from FedEx. Our store is using lbs & in as the unit of measurement for all the products. So currently a package is getting double the weight and dimensions are roughly 60% smaller.

engram-design commented 3 years ago

I'm not sure this is a bug? The getDimensions() function detects what your store settings are, and converts them to kg and cm. The payload we send to Fedex uses kg and cm (see https://github.com/verbb/postie/blob/4c72de984ff05152fe7a3f12bb1309fb40718aab/src/providers/FedEx.php#L255-L283). This is done so we can work with any number of Commerce stores and unit settings.

I'm not sure how you're getting double rates, but they should be accurate. The box-packing algorithm on the other hand is extremely basic, so depending on how many items in a cart you have, it might be setting the box dimensions to be overly large.

siebird commented 3 years ago

Hey, Josh.

We're dumping the response from FedEx before Postie filters our available methods. In this instance, the weight value should be 34.2 kg if it's doing the proper conversion. 75.2 is in lbs

...
[TotalBillingWeight] => FedEx\RateService\ComplexType\Weight Object
(
    [name:protected] => Weight
    [values:protected] => Array
        (
            [Units] => KG
            [Value] => 75.2
        )

)
...
engram-design commented 3 years ago

That's interesting, so that's the response coming back from Fedex? I'll do some testing, I don't actually quite know why its converting it to metric, seeing as though Fedex is primarily US-based, and should be using imperial units anyway...

engram-design commented 3 years ago

Yeah, so I can definitely see a difference in changing this.

Can you give this a go to see if things work on your end, change your verbb/postie requirement in composer.json to:

"require": {
  "verbb/postie": "dev-craft-3 as 2.3.1",
  "...": "..."
}

Then run composer update.

siebird commented 3 years ago

Correct. That's the response from FedEx. Give us a second to test.

siebird commented 3 years ago

Unit of measurement is correct. However, still doubling the weight. I DM'd you on Discord

sparkalow commented 3 years ago

I'm seeing something similar possibly related:

provider UPS version 2.3.0 shipping a single 1 pound item $~16 version 2.3.1 shipping same single 1 pound item $~137

Here is a portion of the v 2.3.1 api request to UPS (note the weight is more than 1):

<PackageWeight>
        <Weight>148</Weight>
        <UnitOfMeasurement>
          <Code>LBS</Code>
          <Description/>
        </UnitOfMeasurement>
</PackageWeight>

getSplitBoxWeights method maybe?

We've reverted to v 2.3.0 so no critical problem for now.

engram-design commented 3 years ago

Interesting, looks related to https://github.com/verbb/postie/commit/c544fa99e4c837397878e54c71b2776d10d455e6 which was supposed to handle max weights of parcels and split into parcels if it exceeded its weight.

Checking it out!

siebird commented 3 years ago

We found some other interesting find today and not quite sure if it's FedEx or logic in the plugin.

Since we're selling odd shaped items I think FedEx is interpreting it as two items and doubling the weight. And it only seems package dimensions throw it off.

I have a developer contact at FedEx that I'm waiting to here back on. I'll DM the video I sent to him.

engram-design commented 3 years ago

@sparkalow I believe your issue might be different to @siebird who is on an older version of Postie before split boxes were introduced in 2.3.1.

But, look out for a release shortly to fix that.

engram-design commented 3 years ago

@sparkalow Refer to 2.3.2.

@siebird closing for now, as I believe this is the response from Fedex (and UPS) about a parcel that's oversized, and not a whole lot we can do about it. But your original issue is also fixed in 2.3.2