Include parameters in multipart POST request

[fluiddot]

Description

Fixes #333

Unfortunately Alamofire doesn't provide a way to serialise the parameters object to be included in the multipartFormData object, as a workaround I'm only adding the String and Int values that are converted to Data.

Testing Details

The easiest way to test this is by adding items from the free photo library as they include as the caption value its photo credit in the upload parameters.

1. Go to the media screen.
2. Tap on + button.
3. Tap on Free Photo Library.
4. Search any term.
5. Tap one or multiple items.
6. Tap on Add button.
7. Wait for the items to be uploaded.
8. Tap on one of the uploaded items.
9. Check that the caption value is defined.

• Please check here if your pull request includes additional test coverage.

[ceyhun]

Checking for one of the use cases for https://developer.wordpress.com/docs/api/1.1/post/sites/%24site/media/new/,
I think treating parameters as Query Parameters and adding another function parameter specifically for Request Parameters could be beneficial in case we also want to send Query Parameters next to Request Parameters in the future.
One other option could be to change the type of parameters to [String: String]? instead of checking for types here.
Yet another option could be adding another parameter to the method that takes a function like multipartFormData -> Void and the caller can deal with how to append those?

[fluiddot]

Thanks @ceyhun for the feedback!

I think treating parameters as Query Parameters and adding another function parameter specifically for Request Parameters could be beneficial in case we also want to send Query Parameters next to Request Parameters in the future.

I though about this option but I wasn't sure if having both could be confusing. In regular POST requests we only have parameters and they're considered as the Request Parameters, in that case Query Parameters should be previously added to the URL.

One other option could be to change the type of parameters to [String: String]? instead of checking for types here.

Yeah that would prevent having to check the types as I did, I included Int type because I saw a couple of cases where there were arguments of that type. In any case since we're converting them into String when parsing the value to Data we could use [String: String]? and let the callers explicitly do the conversion.

Yet another option could be adding another parameter to the method that takes a function like multipartFormData -> Void and the caller can deal with how to append those?

I think this one is probably the best since it will let the caller to include more complex values than primitives. The downside is that callers would have to do the parsing to Data but I think that's expected since it's a multipart POST request.

[fluiddot]

Yet another option could be adding another parameter to the method that takes a function like multipartFormData -> Void and the caller can deal with how to append those?

I tried to apply option 3 but unfortunately the class MultipartFormData that Alamofire provides is not compatible with ObjC and the places where this type of request is being used are ObjC, however this gave me an idea.

The idea was to unify the Request Parameters into one argument, including the file parts which in the end are also body parts. Following this I created a new argument named bodyParts that is an array of BodyPart, a new class I added (based on the previous one FilePart) but more generic that represents a body part of the multipartPOST requests.

I'd appreciate feedback of this change since I'm unaware of the implications of such a refactor, thanks!

[ceyhun]

That's a great idea! But there appears to be a BodyPart class in Alamofire as well. Should we maybe rename this?

[fluiddot]

Thanks! I'm not familiar of class name conflicts on iOS but yeah if it could produce it let's rename it. I was thinking to use FormPart or RequestPart 🤔 .

[ceyhun]

I think FormPart could be better since it's used in MultipartFormData.

[fluiddot]

Yeah makes sense, I'll rename it then, thanks!

[ceyhun]

Overall the changes look good. Thanks a lot for coming up with this and implementing. I would like to test all the affected paths inside WPiOS to be confident. I saw you mentioned under Testing Details to upload a photo from the Free Photo Library which should cover the MediaServiceRemoteREST parts and I guess testing to upload a local image would be necessary as well. What would be the best way to test PostServiceRemoteREST parts?

[fluiddot]

Overall the changes look good. Thanks a lot for coming up with this and implementing. I would like to test all the affected paths inside WPiOS to be confident. I saw you mentioned under Testing Details to upload a photo from the Free Photo Library which should cover the MediaServiceRemoteREST parts and I guess testing to upload a local image would be necessary as well. What would be the best way to test PostServiceRemoteREST parts?

Yeah sorry, due the latest changes I should update the Testing Details because we should cover more cases. For MediaServiceRemoteREST we should try to upload different types of media, for PostServiceRemoteREST it's a bit tricky because the function that invokes the upload media is not being used anywhere 😄 . So in this case unless we explicitly connect it somewhere in the app we can't test it.