Data Layer: Fix formData not being sent when apiVersion is used i…

…n the `query`

wpcom's `req.post` has the signature: `params, [query], body, [callback]` which means positionnal arguments will move around based on the function's arity:

4: `params, query, body, callback`
3: `params, query, body` or `params, body, callback`
2: `params, body`

With that in mind, the use of `compact` in the data-layer considerably complexifies the situation, as we can hit those various arities for many combinations of arguments. This tries to simplify the process while fixing a few edge cases already encountered in a few past PRs (#12135, #12839):

If you want to pass `formData` (in `params`) with an `apiVersion` (in `query`), you NEED to pass a `body`, but any truth-y body will overwrite the `formData` in the HTTP request. This disqualifies the usage of `compact` as you - sometimes - want to pass a false-y values. (PR #12135)

If you want to just pass an `apiVersion` (in `query`), you need - at least - another argument to have an arity of 3, which means you need to pass an empty body (PR #12839)

client/state/data-layer/wpcom-http/actions.js

@@ -21,7 +21,7 @@ import { WPCOM_HTTP_REQUEST } from 'state/action-types';
  */
 export const http = ( {
 	apiVersion = 'v1',
-	body,
+	body = {},
 	method,
 	path,
 	query = {},
@@ -35,7 +35,10 @@ export const http = ( {
 	body,
 	method,
 	path,
-	query: method === 'GET' ? { ...query, apiVersion } : { apiVersion },
+	query: Object.assign(
+		{ apiVersion },
+		query,
+	),
 	formData,
 	onSuccess: onSuccess || action,
 	onFailure: onFailure || action,

client/state/data-layer/wpcom-http/index.js

@@ -1,7 +1,7 @@
 /**
  * External dependencies
  */
-import { compact, get } from 'lodash';
+import { get } from 'lodash';
 
 /**
  * Internal dependencies
@@ -21,11 +21,26 @@ import {
  * fetcherMap :: String -> (Params -> Query -> [Body] -> Promise)
  *
  * @param {string} method name of HTTP method for request
+ * @param {string} wpcomReq wpcom.req object (exposed for easier testing)
  * @returns {Function} the fetcher
  */
-const fetcherMap = method => get( {
-	GET: wpcom.req.get.bind( wpcom.req ),
-	POST: wpcom.req.post.bind( wpcom.req ),
+export const fetcherMap = ( method, wpcomReq ) => get( {
+	GET: ( { path, query = {} }, callback ) => wpcomReq.get(
+		{ path },
+		query,
+		callback
+	),
+	POST: ( { body = {}, formData, path, query = {}, }, callback ) => wpcomReq.post(
+		Object.assign(
+			{ path },
+			formData && { formData }
+		),
+		query,
+		// NOTE: We prioritize formData over body, wpcom.js will overwrite any
+		// formData in presence of a body
+		formData ? null : body,
+		callback
+	),
 }, method, null );
 
 export const successMeta = data => ( { meta: { dataLayer: { data } } } );
@@ -40,38 +55,31 @@ export const queueRequest = ( processOutbound, processInbound ) => ( { dispatch
 	}
 
 	const {
-		body,
-		formData,
 		method: rawMethod,
 		onProgress,
-		path,
-		query = {},
 	} = action;
 
 	const method = rawMethod.toUpperCase();
 
-	const request = fetcherMap( method )( ...compact( [
-		{ path, formData },
-		query,
-		method === 'POST' && body,
-		( error, data ) => {
-			const {
-				failures,
-				nextData,
-				nextError,
-				shouldAbort,
-				successes
-			} = processInbound( action, { dispatch }, data, error );
-
-			if ( true === shouldAbort ) {
-				return null;
-			}
+	const requestCallback = ( error, data ) => {
+		const {
+			failures,
+			nextData,
+			nextError,
+			shouldAbort,
+			successes
+		} = processInbound( action, { dispatch }, data, error );
 
-			return !! nextError
-				? failures.forEach( handler => dispatch( extendAction( handler, failureMeta( nextError ) ) ) )
-				: successes.forEach( handler => dispatch( extendAction( handler, successMeta( nextData ) ) ) );
+		if ( true === shouldAbort ) {
+			return null;
 		}
-	] ) );
+
+		return !! nextError
+			? failures.forEach( handler => dispatch( extendAction( handler, failureMeta( nextError ) ) ) )
+			: successes.forEach( handler => dispatch( extendAction( handler, successMeta( nextData ) ) ) );
+	};
+
+	const request = fetcherMap( method, wpcom.req )( action, requestCallback );
 
 	if ( 'POST' === method && onProgress ) {
 		request.upload.onprogress = event => dispatch( extendAction( onProgress, progressMeta( event ) ) );

client/state/data-layer/wpcom-http/test/index.js

@@ -11,6 +11,7 @@ import useNock, { nock } from 'test/helpers/use-nock';
 import { extendAction } from 'state/utils';
 import {
 	failureMeta,
+	fetcherMap,
 	queueRequest,
 	successMeta,
 } from '../';
@@ -77,3 +78,99 @@ describe( '#queueRequest', () => {
 		}, 10 );
 	} );
 } );
+
+describe( '#fetcherMap', () => {
+	const wpcomReq = {};
+	const noopCallback = () => {};
+
+	describe( 'GET', () => {
+		let getFooAction;
+
+		beforeEach( () => {
+			wpcomReq.get = spy();
+			getFooAction = {
+				method: 'GET',
+				path: '/foo',
+			};
+		} );
+
+		it( 'should send with query', () => {
+			getFooAction.query = {
+				apiVersion: 'v2.0',
+				apiNamespace: 'wp/v2',
+			};
+
+			fetcherMap( 'GET', wpcomReq )( getFooAction, noopCallback );
+
+			expect( wpcomReq.get ).to.have.been.calledWith(
+				{ path: '/foo' },
+				{
+					apiVersion: 'v2.0',
+					apiNamespace: 'wp/v2',
+				},
+				noopCallback
+			);
+		} );
+	} );
+
+	describe( 'POST', () => {
+		let postFooAction;
+
+		beforeEach( () => {
+			wpcomReq.post = spy();
+			postFooAction = {
+				method: 'POST',
+				path: '/foo',
+			};
+		} );
+
+		it( 'should send formData with query', () => {
+			postFooAction.formData = [ [ 'foo', 'bar' ], ];
+			postFooAction.query = { apiVersion: 'v1.1' };
+
+			fetcherMap( 'POST', wpcomReq )( postFooAction, noopCallback );
+
+			expect( wpcomReq.post ).to.have.been.calledWith(
+				{
+					path: '/foo',
+					formData: [ [ 'foo', 'bar' ], ],
+				},
+				{
+					apiVersion: 'v1.1',
+				},
+				null,
+				noopCallback
+			);
+		} );
+
+		it( 'it should prioritize formData over body', () => {
+			postFooAction.formData = [ [ 'foo', 'bar' ], ];
+			postFooAction.body = { lorem: 'ipsum' };
+
+			fetcherMap( 'POST', wpcomReq )( postFooAction, noopCallback );
+
+			expect( wpcomReq.post ).to.have.been.calledWith(
+				{
+					path: '/foo',
+					formData: [ [ 'foo', 'bar' ], ],
+				},
+				{ },
+				null,
+				noopCallback
+			);
+		} );
+
+		it( 'should send body in the absence of any formData', () => {
+			postFooAction.body = { lorem: 'ipsum' };
+
+			fetcherMap( 'POST', wpcomReq )( postFooAction, noopCallback );
+
+			expect( wpcomReq.post ).to.have.been.calledWith(
+				{ path: '/foo' },
+				{ },
+				{ lorem: 'ipsum' },
+				noopCallback
+			);
+		} );
+	} );
+} );