Add support for custom shipping setups and several packages #157
Conversation
Version 1.6.1
Version 1.7.0
Version 1.7.1
Version 1.8.0
Version 1.8.1
Version 1.9.0
This could be used to create unique line_ids when there are several packages
|
Thanks for the pull request, @voldern. Will pass it on to Krokedil for review. |
|
Hi @voldern, thanks for this! We have assigned @MichaelBengtsson to do a code review on this when he gets the time and will get back to you if we have any questions related to that. After that we will then do some more QA related testing to make sure that other functionality works as expected before we can merge it in. |
|
Thank you for your swift feedback! Looking more into the code when resolving issue #158 I see that there are more places where shipping is handled in the plugin, especially related to the orders portion of the code, so it will need some more changes to also handle the different checkout options that we are currently not using. I've also divided the added filters into two different kinds, but they might be possible to consolidate into one kind of filter or use a different distinction then what I have. |
| * @return boolean | ||
| */ | ||
| public function show_express_shipping() { | ||
| if ( 'no' === $this->settings['express_show_shipping'] ) { |
There was a problem hiding this comment.
Since the setting was commented out before, this will probably cause a error notice to be generated for existing merchants on a update. Since the key wont be set in the options table unless they save the settings again. So making a isset check on this would be needed.
| 'quantity' => 1, | ||
| /* Dintero needs to know this is an order with multiple shipping options by setting the 'type'. */ | ||
| 'type' => 'shipping', | ||
| public function get_shipping_item( $shipping_method, $shipping_index ) { |
There was a problem hiding this comment.
To be safe the $shipping_index parameter should probably be defaulted to null, incase its used elsewhere that's not been spotted, or by others. That way its backwards compatible.
| * @return void | ||
| */ | ||
| public static function add_shipping( &$body, $helper, $is_embedded, $is_express, $is_shipping_in_iframe ) { | ||
| public static function add_shipping( &$body, $helper, $is_embedded, $is_express, $is_shipping_in_iframe, $show_express_shipping ) { |
There was a problem hiding this comment.
Not sure on this, but the $show_express_shipping should probably have a default as well to ensure full backwards compatibility, but not sure what the value should be set to. Since the setting had a different default before, but it was commented out i think we need to discuss this on our end,
There was a problem hiding this comment.
I suggest setting $show_express_shipping to true as that will mimic the current behavior:
| 'vat' => ( empty( floatval( $shipping_method->get_cost() ) ) ) ? 0 : self::format_number( $shipping_method->get_shipping_tax() / $shipping_method->get_cost() ), | ||
| 'quantity' => 1, | ||
| 'type' => 'shipping', | ||
| public function get_shipping_item( $shipping_method, $package_index ) { |
There was a problem hiding this comment.
To be safe the $package_index parameter should probably be defaulted to null, incase its used elsewhere that's not been spotted, or by others. That way its backwards compatible.
|
@voldern Thank you for the PR! The code looks solid overall, except for the small things i pointed out in my review. But we would need to test it internally before we can merge this. The main concern i have is to make sure its compatible with other plugins that i know we worked with early in the plugins development for compatibility. I would also want to make sure that the changes are applied everywhere its needed before merging this as well. As you stated in your last comment. I also see some things in our code originally that could be improved to make this change a bit more clean, with less complexity and more customization using filters. But we will need to look into that internally as well. |
|
Hello @voldern ! I've pushed some changes to |
|
Hi @mntzrr, sorry for the late reply. I didn't get notifications for updates to this PR. We have decided to change our setup to move away from multi-vendor support, so we should be able to switch to the mainline of the plugin instead. But if you still want to support this in the plugin then feel free to take over the changes and adjust. I think the changes that you have suggested in this thread makes sense 👍 |
Hi,
We are running a Dokan setup with support for multi-vendor orders and multiple shipping packages. To make that work with Dintero we have had to make some changes to this great plugin:
PS: Its not unlikely that there are some bugs lurking since we have only tested it for our use-case with embedded express checkout.