Added apply_shipping_rules support to the createLabel call #35
Conversation
There was a problem hiding this comment.
@linaori Thanks for the PR! I've left a comment/question on on the combination of shipping method and applyShippingRules. Please let me know your opnion here.
Additionally, the createParcel() method also supports passing a shipping method so I think adding this new argument would also be desireable there, right? Maybe even for updateParcel() too, but I don't really see a use case to set the shipping method in an update right now.
| @@ -671,7 +677,8 @@ protected function getParcelData( | |||
| ?string $customsInvoiceNumber, | |||
| ?int $customsShipmentType, | |||
| ?array $items, | |||
| ?string $postNumber | |||
| ?string $postNumber, | |||
| bool $applyShippingRules = false | |||
There was a problem hiding this comment.
I think false is the right default here because it will not change API behavior for extending classes (which I doubt there are any though).
| * @throws SendcloudRequestException | ||
| */ | ||
| public function createLabel(Parcel|int $parcel, ShippingMethod|int $shippingMethod, SenderAddress|Address|int|null $senderAddress): Parcel | ||
| public function createLabel(Parcel|int $parcel, ShippingMethod|int $shippingMethod, SenderAddress|Address|int|null $senderAddress, bool $applyShippingRules = false): Parcel |
There was a problem hiding this comment.
Is requiring $shippingMethod in combination with $applyShippingRules: true desirable if it's going to be ignored? Or does it serve as a fallback in case no shipping rules apply? I can't make up from the documentation whether the shipment property is required in this case. If adding $shippingMethod does nothing when $applyShippingRules: true we could add it as a separate method instead (createLabelWithShippingRules($parcel, ?$senderAddress)).
There was a problem hiding this comment.
From what I can tell it's still required to pass a shipping method and this will indeed be used as fallback when the shipping rules can't be applied. This is how it was explained to me in a call with Sendcloud. That said it's a bit difficult for me to test as my Sendcloud account currently doesn't have any contracts with any carriers. I will try to verify this behavior myself:
- pass a shipping method that requires a contract
- pass apply rules: true
- add a shipping rule that would set the shipping method to 8 (unstamped letter) with a condition that will never be hit
- call createLabel() and see what happens
A second method is still possible regardless as the fallback shipping method would most likely be 8 (unstamped letter), but it does take away the ability to change this default. Unfortunately there's no API call to retrieve the default shipping method configured for your account.
I've looked into it and was unable to trigger the shipping rules unless I requested the label. My initial design included the create and update calls and I reverted to just |
When requesting labels it can also apply shipping rules when this feature is available. This will effectively override the shipping method given.
I've given it a default value in
getParcelDatato ensure backwards compatibility due to this method being protected. I've addedfalseas default value to the callers as all parameter values are explicitly passed along, but can change this if desired.