Skip to content
This repository is currently being migrated. It's locked while the migration is in progress.

Feature/hw 66544 v4 user upload multipart #60

Merged
merged 12 commits into from
Sep 3, 2020

Conversation

ramahalingam
Copy link
Collaborator

No description provided.

@coveralls
Copy link

coveralls commented Aug 30, 2020

Coverage Status

Coverage increased (+0.008%) to 99.676% when pulling 2a85d96 on feature/HW-66544-V4-UserUploadMultipart into 743412d on V4.

public WebResource getWebResource(final String url) {
client.addFilter(new HTTPBasicAuthFilter(this.username, this.password));
webResource = client.resource(url);
return webResource;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be changed to: return client.resource(url);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed as suggested.

hyperwalletUserresponse.getDocuments().get(0).getCountry().equals(ECountryCode.CA);
hyperwalletUserresponse.getDocuments().get(0).getStatus().equals(EKycDocumentVerificationStatus.NEW);
} catch (HyperwalletException e) {
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catch block is empty

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catch block removed and added assert statements


hyperwalletApiClient.put(baseUrl + "/documentUpload", multiPart, HyperwalletUser.class);
} catch (Exception exception) {
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

catch block is empty

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catch block removed and added assert statements.

@JsonFilter(HyperwalletJsonConfiguration.INCLUSION_FILTER)
@XmlRootElement
@XmlAccessorType(XmlAccessType.FIELD)
public class DocumentVerificationDocumentsRepresentation {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Class name can be changed to HyperwalletDocument with more details about the class in the javadoc

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed class name.

Copy link
Contributor

@peter-joseph peter-joseph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry please have a look, main takeaway is importing the whole jersey client and is used only in 1 use case which is PUT with multipart MIME, We don't want integrator to have that library in their build too, the goal is to make our SDK as lightweight as possible

<dependency>
<groupId>com.sun.jersey</groupId>
<artifactId>jersey-client</artifactId>
<version>1.18</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this library used only for multipart mime? can we have our implementation that does that part only?
the reason being is we are adding library that is only used in 1 use case, and it will make the SDK heavy for the integrators cause the will be added on top of their current library deps

Response response = new Response();
try {
webResource = getWebResource(url);
ClientResponse clientResponse = webResource.type(MediaType.MULTIPART_FORM_DATA_TYPE).put(ClientResponse.class, formDataMultiPart);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can have our own small implementation of pushing multipart http thru normal io streams, can we do that instead? rather than importing the whole jersey client?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hw2 app is consuming the documents with FormDataMultiPart

public Response uploadDocuments(@UserToken @PathParam("userToken") String userToken, final FormDataMultiPart multiPart,
        @Context UriInfo uriInfo)
        throws Exception {

That is the reason why we went with this approach. Please let me know if still we need to change it.

Copy link
Contributor

@peter-joseph peter-joseph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or If it takes a lot of time can we create a ticket to make a decision to use jersey client and drop protea http library? or use protea and implement our own multipart implementation

@akalichety-hw akalichety-hw merged commit 7f135f6 into V4 Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants