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

@@ -28,7 +28,7 @@ import { WPCOM_HTTP_REQUEST } from 'state/action-types';
 export const http = ( {
 	apiVersion,
 	apiNamespace,
-	body,
+	body = {},
 	method,
 	path,
 	query = {},
@@ -47,7 +47,7 @@ export const http = ( {
 		body,
 		method,
 		path,
-		query: method === 'GET' ? { ...query, ...version } : version,
+		query: { ...query, ...version },
 		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,20 +55,14 @@ 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,
+	const request = fetcherMap( method, wpcom.req )(
+		action,
 		( error, data ) => {
 			const {
 				failures,
@@ -71,7 +80,7 @@ export const queueRequest = ( processOutbound, processInbound ) => ( { dispatch
 				? failures.forEach( handler => dispatch( extendAction( handler, failureMeta( nextError ) ) ) )
 				: successes.forEach( handler => dispatch( extendAction( handler, successMeta( nextData ) ) ) );
 		}
-	] ) );
+	);
 
 	if ( 'POST' === method && onProgress ) {
 		request.upload.onprogress = event => dispatch( extendAction( onProgress, progressMeta( event ) ) );

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

@@ -2,6 +2,7 @@
  * External dependencies
  */
 import { expect } from 'chai';
+import { noop } from 'lodash';
 import { spy } from 'sinon';
 
 /**
@@ -11,6 +12,7 @@ import useNock, { nock } from 'test/helpers/use-nock';
 import { extendAction } from 'state/utils';
 import {
 	failureMeta,
+	fetcherMap,
 	queueRequest,
 	successMeta,
 } from '../';
@@ -31,20 +33,22 @@ const failer = { type: 'FAIL' };
 const getMe = {
 	method: 'GET',
 	path: '/me',
-	apiVersion: 'v1.1',
+	query: {
+		apiVersion: '1.1',
+	},
 	onFailure: failer,
 	onSuccess: succeeder,
 };
 
 describe( '#queueRequest', () => {
-	let dispatch;
-	let next;
+	const dispatch = spy();
+	const next = spy();
 
 	useNock();
 
 	beforeEach( () => {
-		dispatch = spy();
-		next = spy();
+		dispatch.reset();
+		next.reset();
 	} );
 
 	it( 'should call `onSuccess` when a response returns with data', done => {
@@ -77,3 +81,100 @@ describe( '#queueRequest', () => {
 		}, 10 );
 	} );
 } );
+
+describe( '#fetcherMap', () => {
+	const wpcomReq = {
+		get: spy(),
+		post: spy(),
+	};
+
+	beforeEach( () => {
+		wpcomReq.get.reset();
+		wpcomReq.post.reset();
+	} );
+
+	describe( 'GET', () => {
+		it( 'should send with query', () => {
+			const getFooAction = {
+				method: 'GET',
+				path: '/foo',
+				query: {
+					apiNamespace: 'wp/v2',
+				},
+			};
+
+			fetcherMap( 'GET', wpcomReq )( getFooAction, noop );
+
+			expect( wpcomReq.get ).to.have.been.calledWith(
+				{ path: '/foo' },
+				{
+					apiNamespace: 'wp/v2',
+				},
+				noop
+			);
+		} );
+	} );
+
+	describe( 'POST', () => {
+		it( 'should send formData with query', () => {
+			const postFooAction = {
+				method: 'POST',
+				path: '/foo',
+				formData: [ [ 'foo', 'bar' ], ],
+				query: { apiVersion: '1.1' },
+			};
+
+			fetcherMap( 'POST', wpcomReq )( postFooAction, noop );
+
+			expect( wpcomReq.post ).to.have.been.calledWith(
+				{
+					path: '/foo',
+					formData: [ [ 'foo', 'bar' ], ],
+				},
+				{
+					apiVersion: '1.1',
+				},
+				null,
+				noop
+			);
+		} );
+
+		it( 'it should prioritize formData over body', () => {
+			const postFooAction = {
+				method: 'POST',
+				path: '/foo',
+				formData: [ [ 'foo', 'bar' ], ],
+				body: { lorem: 'ipsum' },
+			};
+
+			fetcherMap( 'POST', wpcomReq )( postFooAction, noop );
+
+			expect( wpcomReq.post ).to.have.been.calledWith(
+				{
+					path: '/foo',
+					formData: [ [ 'foo', 'bar' ], ],
+				},
+				{ },
+				null,
+				noop
+			);
+		} );
+
+		it( 'should send body in the absence of any formData', () => {
+			const postFooAction = {
+				method: 'POST',
+				path: '/foo',
+				body: { lorem: 'ipsum' },
+			};
+
+			fetcherMap( 'POST', wpcomReq )( postFooAction, noop );
+
+			expect( wpcomReq.post ).to.have.been.calledWith(
+				{ path: '/foo' },
+				{ },
+				{ lorem: 'ipsum' },
+				noop
+			);
+		} );
+	} );
+} );