This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Allow third party methods to appear in local pickup area #8256
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
opr
added
status: needs review
type: enhancement
opr
requested review from
senadir and
mikejolley
and removed request for
senadir
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThe
This comment was automatically generated by the TypeScript Errors Report
assets/js/base/context/hooks/use-checkout-address.ts
|
|
Size Change: +293 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
opr
force-pushed
the
add/local-pickup-methods
branch
from
3c8fe36
to
58d5288
Compare
mikejolley
reviewed
Jan 24, 2023
7 tasks
mikejolley
approved these changes
Feb 1, 2023
src/Shipping/ShippingController.php
Outdated
| array_filter( | ||
| WC()->shipping()->get_shipping_methods(), | ||
| function( $method ) { | ||
| return $method->supports( 'collectible' ); |
There was a problem hiding this comment.
I'd like to suggest we make this wording/variable slightly clearer by making it:
return $method->supports( 'collection' );
or
return $method->supports( 'local_pickup' );
What do you think?
There was a problem hiding this comment.
Sounds good, I will update it.
|
|
||
| export interface minMaxPrices { | ||
| min: CartShippingPackageShippingRate | undefined; | ||
| max: CartShippingPackageShippingRate | undefined; | ||
| } | ||
|
|
||
| const collectibleMethodIds = getSetting< string[] >( |
There was a problem hiding this comment.
Nothing wrong here, however, we could probably move this settings call to a new shared util which returns a function called isRateCollectable or similar. This can be consumed here and in assets/js/blocks/checkout/inner-blocks/checkout-pickup-options-block/block.tsx
There was a problem hiding this comment.
If this method accepted either a string or array of strings, you could also replace areRatesCollectible with this helper. The code that splits rate.split( ':' )[ 0 ] method ids would remain in place. Thoughts?
There was a problem hiding this comment.
See changes in assets/js/base/utils/shipping-rates.ts, this just checks a rate, or rates, and doesn't take care of splitting
src/Shipping/ShippingController.php
Outdated
| ); | ||
| // `pickup_location` will always be collectible. Adding it here means we can avoid hard coding it elsewhere on | ||
| // the client. | ||
| $collectible_method_ids[] = 'pickup_location'; |
There was a problem hiding this comment.
Why would the code above not return pickup_location?
There was a problem hiding this comment.
It does now, because of this change: https://github.com/woocommerce/woocommerce-blocks/pull/8256/files#diff-b9026989674ea1e4ec8f06e05634d871425015639a2177cd79499cb8ed564d1fR29
opr
commented
Feb 2, 2023
src/Shipping/ShippingController.php
Outdated
| * @return string[] List of payment method ids that support the 'local_pickup' feature. | ||
| */ | ||
| public function get_local_pickup_method_ids() { | ||
| $methods_supporting_local_pickup = array_unique( |
There was a problem hiding this comment.
I had to go back to doing it this way as WC()->shipping()->get_shipping_methods() returns like this:
For some reason it can include the same method multiple times, so stripping out duplicates and only outputting the method ID seems most sensible.
Is there a cleaner way of doing this?
There was a problem hiding this comment.
How about this:
$methods_supporting_local_pickup = array_reduce(
WC()->shipping()->get_shipping_methods(),
function( $methods, $method ) {
if ( $method->supports( 'local_pickup' ) ) {
$methods[] = $method->id;
}
return $methods;
},
array()
);There was a problem hiding this comment.
Nice, that looks better, but I still need to array_unique it, given WC()->shipping()->get_shipping_methods() can return the same one multiple times. I'll implement.
This is so the shipping methods get change to register before this is called. Passing a callback to `add` means it won't be called until just before it is output.
opr
force-pushed
the
add/local-pickup-methods
branch
from
af015a4
to
edff9a1
Compare
mikejolley
approved these changes
Feb 3, 2023
senadir
reviewed
Feb 6, 2023
| ) => void; | ||
| // Only true when ALL packages support local pickup. If true, we can show the collection/delivery toggle | ||
| isCollectable: boolean; | ||
| // True when a rate is currently being selected and persisted to the server. | ||
| isSelectingRate: boolean; | ||
|
|
||
| hasSelectedLocalPickup: boolean; |
| @@ -118,7 +119,7 @@ const Block = (): JSX.Element | null => { | |||
|
|
|||
| // Get pickup locations from the first shipping package. | |||
| const pickupLocations = ( shippingRates[ 0 ]?.shipping_rates || [] ).filter( | |||
| ( { method_id: methodId } ) => methodId === 'pickup_location' | |||
| isPackageRateCollectable | |||
There was a problem hiding this comment.
isPackageRateCollectable and hasCollectableRate seem to handle similar logic, why can't we use just one?
| @@ -26,6 +26,7 @@ public function init() { | |||
| $this->title = $this->get_option( 'title' ); | |||
| $this->tax_status = $this->get_option( 'tax_status' ); | |||
| $this->cost = $this->get_option( 'cost' ); | |||
| $this->supports = [ 'local-pickup' ]; | |||
There was a problem hiding this comment.
This should be documented, we currently don't have a dev documentation page for Local Pickup, but once we do, we should include this. Also this PR needs a dev note for this one.
7 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This PR will allow third party shipping methods that support 'collectible' to be displayed under local pickup.
To achieve this, I created a method in
ShippingController.phpto get all registered methods that declarecollectiblesupport, then add their IDs to thecollectibleMethodIdskey in the asset data registry.This can be retrieved on the client by
getSetting< string[] >( 'collectibleMethodIds', [] ).When deciding which methods to show in the
checkout-pickup-optionsblock it now checks to see if the method name is in thiscollectibleMethodIdsarray or if the method id ispickup_location.It also removes methods, following the same rules as above, from being displayed in the "regular" shipping options area.
I also modified
assets/js/base/context/hooks/shipping/types.tsto setpackageIdas optional, this type definition did not correctly reflect the implementation.This PR also removes hardcoded references to
pickup_locationwhen it is used to determine whether a local pickup method is in use, and instead uses thecollectibleMethodIdssite setting.Fixes #8012
Testing
Automated Tests
Internal developer testing
Expand for example shipping method
User Facing Testing
WooCommerce Visibility
Performance Impact
Changelog