Merge pull request #452 from woocommerce/pcp-443-address

Fix shipping address handling (443)

modules/ppcp-api-client/src/Endpoint/OrderEndpoint.php

@@ -198,14 +198,20 @@ class OrderEndpoint {
 				return $is_purchase_unit;
 			}
 		);
-		$shipping_preferences    = $contains_physical_goods
-			? $shipping_address_is_fixed ?
-				ApplicationContext::SHIPPING_PREFERENCE_SET_PROVIDED_ADDRESS
-				: ApplicationContext::SHIPPING_PREFERENCE_GET_FROM_FILE
-			: ApplicationContext::SHIPPING_PREFERENCE_NO_SHIPPING;
 
-		if ( $this->has_items_without_shipping( $items ) ) {
+		$shipping_preference = ApplicationContext::SHIPPING_PREFERENCE_NO_SHIPPING;
-			$shipping_preferences = ApplicationContext::SHIPPING_PREFERENCE_NO_SHIPPING;
+		if ( $contains_physical_goods ) {
+			if ( $shipping_address_is_fixed ) {
+				// Checkout + no address given? Probably something weird happened, like no form validation?
+				// Also note that $items currently always seems to be an array with one item.
+				if ( $this->has_items_without_shipping( $items ) ) {
+					$shipping_preference = ApplicationContext::SHIPPING_PREFERENCE_NO_SHIPPING;
+				} else {
+					$shipping_preference = ApplicationContext::SHIPPING_PREFERENCE_SET_PROVIDED_ADDRESS;
+				}
+			} else {
+				$shipping_preference = ApplicationContext::SHIPPING_PREFERENCE_GET_FROM_FILE;
+			}
 		}
 
 		$bearer = $this->bearer->bearer();
@@ -218,7 +224,7 @@ class OrderEndpoint {
 				$items
 			),
 			'application_context' => $this->application_context_repository
-				->current_context( $shipping_preferences )->to_array(),
+				->current_context( $shipping_preference )->to_array(),
 		);
 		if ( $payer ) {
 			$data['payer'] = $payer->to_array();
@@ -591,7 +597,7 @@ class OrderEndpoint {
 	/**
 	 * Checks if there is at least one item without shipping.
 	 *
-	 * @param array $items The items.
+	 * @param PurchaseUnit[] $items The items.
 	 * @return bool Whether items contains shipping or not.
 	 */
 	private function has_items_without_shipping( array $items ): bool {

modules/ppcp-api-client/src/Entity/Address.php

@@ -143,13 +143,15 @@ class Address {
 	 * @return array
 	 */
 	public function to_array(): array {
-		return array(
+		return array_filter(
-			'country_code'   => $this->country_code(),
+			array(
-			'address_line_1' => $this->address_line_1(),
+				'country_code'   => $this->country_code(),
-			'address_line_2' => $this->address_line_2(),
+				'address_line_1' => $this->address_line_1(),
-			'admin_area_1'   => $this->admin_area_1(),
+				'address_line_2' => $this->address_line_2(),
-			'admin_area_2'   => $this->admin_area_2(),
+				'admin_area_1'   => $this->admin_area_1(),
-			'postal_code'    => $this->postal_code(),
+				'admin_area_2'   => $this->admin_area_2(),
+				'postal_code'    => $this->postal_code(),
+			)
 		);
 	}
 }

modules/ppcp-api-client/src/Factory/PurchaseUnitFactory.php

@@ -112,7 +112,7 @@ class PurchaseUnitFactory {
 		if (
 			! $this->shipping_needed( ... array_values( $items ) ) ||
 			empty( $shipping->address()->country_code() ) ||
-			( $shipping->address()->country_code() && ! $shipping->address()->postal_code() )
+			( ! $shipping->address()->postal_code() && ! $this->country_without_postal_code( $shipping->address()->country_code() ) )
 		) {
 			$shipping = null;
 		}
@@ -121,7 +121,6 @@ class PurchaseUnitFactory {
 		$payee           = $this->payee_repository->payee();
 		$custom_id       = (string) $order->get_id();
 		$invoice_id      = $this->prefix . $order->get_order_number();
-		$retry           = $order->get_meta( 'ppcp-retry' ) ? '-' . $order->get_meta( 'ppcp-retry' ) : '';
 		$soft_descriptor = '';
 
 		$purchase_unit = new PurchaseUnit(
@@ -158,9 +157,8 @@ class PurchaseUnitFactory {
 		if ( $this->shipping_needed( ... array_values( $items ) ) && is_a( $customer, \WC_Customer::class ) ) {
 			$shipping = $this->shipping_factory->from_wc_customer( \WC()->customer );
 			if (
-				2 !== strlen( $shipping->address()->country_code() )
+				2 !== strlen( $shipping->address()->country_code() ) ||
-				|| ( ! $shipping->address()->postal_code() )
+				( ! $shipping->address()->postal_code() && ! $this->country_without_postal_code( $shipping->address()->country_code() ) )
-				|| $this->country_without_postal_code( $shipping->address()->country_code() )
 			) {
 				$shipping = null;
 			}
@@ -276,9 +274,6 @@ class PurchaseUnitFactory {
 	 */
 	private function country_without_postal_code( string $country_code ): bool {
 		$countries = array( 'AE', 'AF', 'AG', 'AI', 'AL', 'AN', 'AO', 'AW', 'BB', 'BF', 'BH', 'BI', 'BJ', 'BM', 'BO', 'BS', 'BT', 'BW', 'BZ', 'CD', 'CF', 'CG', 'CI', 'CK', 'CL', 'CM', 'CO', 'CR', 'CV', 'DJ', 'DM', 'DO', 'EC', 'EG', 'ER', 'ET', 'FJ', 'FK', 'GA', 'GD', 'GH', 'GI', 'GM', 'GN', 'GQ', 'GT', 'GW', 'GY', 'HK', 'HN', 'HT', 'IE', 'IQ', 'IR', 'JM', 'JO', 'KE', 'KH', 'KI', 'KM', 'KN', 'KP', 'KW', 'KY', 'LA', 'LB', 'LC', 'LK', 'LR', 'LS', 'LY', 'ML', 'MM', 'MO', 'MR', 'MS', 'MT', 'MU', 'MW', 'MZ', 'NA', 'NE', 'NG', 'NI', 'NP', 'NR', 'NU', 'OM', 'PA', 'PE', 'PF', 'PY', 'QA', 'RW', 'SA', 'SB', 'SC', 'SD', 'SL', 'SN', 'SO', 'SR', 'SS', 'ST', 'SV', 'SY', 'TC', 'TD', 'TG', 'TL', 'TO', 'TT', 'TV', 'TZ', 'UG', 'UY', 'VC', 'VE', 'VG', 'VN', 'VU', 'WS', 'XA', 'XB', 'XC', 'XE', 'XL', 'XM', 'XN', 'XS', 'YE', 'ZM', 'ZW' );
-		if ( in_array( $country_code, $countries, true ) ) {
+		return in_array( $country_code, $countries, true );
-			return true;
-		}
-		return false;
 	}
 }

tests/PHPUnit/ApiClient/Endpoint/OrderEndpointTest.php

@@ -6,6 +6,7 @@ namespace WooCommerce\PayPalCommerce\ApiClient\Endpoint;
 use Hamcrest\Matchers;
 use Requests_Utility_CaseInsensitiveDictionary;
 use WooCommerce\PayPalCommerce\ApiClient\Authentication\Bearer;
+use WooCommerce\PayPalCommerce\ApiClient\Entity\Address;
 use WooCommerce\PayPalCommerce\ApiClient\Entity\ApplicationContext;
 use WooCommerce\PayPalCommerce\ApiClient\Entity\Capture;
 use WooCommerce\PayPalCommerce\ApiClient\Entity\CaptureStatus;
@@ -15,6 +16,7 @@ use WooCommerce\PayPalCommerce\ApiClient\Entity\PatchCollection;
 use WooCommerce\PayPalCommerce\ApiClient\Entity\Payer;
 use WooCommerce\PayPalCommerce\ApiClient\Entity\Payments;
 use WooCommerce\PayPalCommerce\ApiClient\Entity\PurchaseUnit;
+use WooCommerce\PayPalCommerce\ApiClient\Entity\Shipping;
 use WooCommerce\PayPalCommerce\ApiClient\Entity\Token;
 use WooCommerce\PayPalCommerce\ApiClient\Exception\RuntimeException;
 use WooCommerce\PayPalCommerce\ApiClient\Factory\OrderFactory;
@@ -31,11 +33,14 @@ use function Brain\Monkey\Functions\when;
 
 class OrderEndpointTest extends TestCase
 {
+	private $shipping;
 
 	public function setUp(): void
 	{
 		parent::setUp();
 		when('wc_print_r')->returnArg();
+
+		$this->shipping = new Shipping('shipping', new Address('US', 'street', '', 'CA', '', '12345'));
 	}
 
 	public function testOrderDefault()
@@ -912,7 +917,7 @@ class OrderEndpointTest extends TestCase
         $purchaseUnit
             ->expects('to_array')
             ->andReturn(['singlePurchaseUnit']);
-        $purchaseUnit->expects('shipping')->andReturn(true);
+        $purchaseUnit->shouldReceive('shipping')->andReturn($this->shipping);
 
         expect('wp_remote_get')
             ->andReturnUsing(
@@ -1015,7 +1020,7 @@ class OrderEndpointTest extends TestCase
         $purchaseUnit
             ->expects('to_array')
             ->andReturn(['singlePurchaseUnit']);
-		$purchaseUnit->expects('shipping')->andReturn(true);
+		$purchaseUnit->shouldReceive('shipping')->andReturn($this->shipping);
 
         expect('wp_remote_get')
             ->andReturnUsing(
@@ -1092,7 +1097,7 @@ class OrderEndpointTest extends TestCase
         $purchaseUnit
             ->expects('to_array')
             ->andReturn(['singlePurchaseUnit']);
-		$purchaseUnit->expects('shipping')->andReturn(true);
+		$purchaseUnit->shouldReceive('shipping')->andReturn($this->shipping);
 
         expect('wp_remote_get')
             ->andReturnUsing(
@@ -1177,7 +1182,7 @@ class OrderEndpointTest extends TestCase
         $purchaseUnit
             ->expects('to_array')
             ->andReturn(['singlePurchaseUnit']);
-		$purchaseUnit->expects('shipping')->andReturn(true);
+		$purchaseUnit->shouldReceive('shipping')->andReturn($this->shipping);
 
         expect('wp_remote_get')
             ->andReturnUsing(

tests/PHPUnit/ApiClient/Factory/PurchaseUnitFactoryTest.php

@@ -26,7 +26,6 @@ class PurchaseUnitFactoryTest extends TestCase
         $wcOrder = Mockery::mock(\WC_Order::class);
         $wcOrder->expects('get_order_number')->andReturn($this->wcOrderNumber);
         $wcOrder->expects('get_id')->andReturn($this->wcOrderId);
-        $wcOrder->expects('get_meta')->andReturn('');
         $amount = Mockery::mock(Amount::class);
         $amountFactory = Mockery::mock(AmountFactory::class);
         $amountFactory
@@ -49,7 +48,6 @@ class PurchaseUnitFactoryTest extends TestCase
         $address = Mockery::mock(Address::class);
         $address
             ->shouldReceive('country_code')
-            ->twice()
             ->andReturn('DE');
         $address
             ->shouldReceive('postal_code')
@@ -91,7 +89,6 @@ class PurchaseUnitFactoryTest extends TestCase
         $wcOrder = Mockery::mock(\WC_Order::class);
         $wcOrder->expects('get_order_number')->andReturn($this->wcOrderNumber);
         $wcOrder->expects('get_id')->andReturn($this->wcOrderId);
-		$wcOrder->expects('get_meta')->andReturn('');
         $amount = Mockery::mock(Amount::class);
         $amountFactory = Mockery::mock(AmountFactory::class);
         $amountFactory
@@ -147,7 +144,6 @@ class PurchaseUnitFactoryTest extends TestCase
         $wcOrder = Mockery::mock(\WC_Order::class);
         $wcOrder->expects('get_order_number')->andReturn($this->wcOrderNumber);
         $wcOrder->expects('get_id')->andReturn($this->wcOrderId);
-		$wcOrder->expects('get_meta')->andReturn('');
         $amount = Mockery::mock(Amount::class);
         $amountFactory = Mockery::mock(AmountFactory::class);
         $amountFactory