Added reportOrder to ButtonMerchant #25
Conversation
| @Nullable | ||
| private Map<String, String> headers; | ||
| @Nullable | ||
| private JSONObject body; |
There was a problem hiding this comment.
Can we default to an empty header map and empty JSON body? This would remove the need for null checks in ConnectionManagerImpl.
| public class ApiRequestTest { | ||
|
|
||
| @Test | ||
| public void apiRequest_verifyDefaultValues() { |
There was a problem hiding this comment.
What do you think about dropping apiRequest_ prefix from all tests? Seems unnecessary given the file is called ApiRequestTest 😂
| @@ -151,4 +161,107 @@ public void postUserActivity_validateRequest() throws Exception { | |||
| assertEquals("AUG", requestBody.getString("currency")); | |||
| assertEquals("merchant-library", requestBody.getString("source")); | |||
| } | |||
|
|
|||
| @Test | |||
| public void postOrder_validateRequest() throws Exception { | |||
There was a problem hiding this comment.
What do you think about breaking this down into smaller tests for different elements in the payload?
| connectionManager.post("/", null); | ||
| connectionManager.executeRequest(new ApiRequest.Builder(ApiRequest.RequestMethod.POST, | ||
| "/") | ||
| .build() |
There was a problem hiding this comment.
This seems like an unfortunate line wrap..
| messageDigest.update(bytes, 0, bytes.length); | ||
| return new String(messageDigest.digest()); | ||
| } catch (NoSuchAlgorithmException ignored) { | ||
|
|
| import java.util.Map; | ||
|
|
||
| /** | ||
| * Api request model for {@link ConnectionManager} |
| /** | ||
| * Util class with helper methods | ||
| */ | ||
| final class ButtonUtil { |
There was a problem hiding this comment.
Since it's a utility class, you may want to add a private default constructor to prevent instantiation.
There was a problem hiding this comment.
Not really needed because you can't call any of the methods on the class even if it was instantiated
| messageDigest.update(bytes, 0, bytes.length); | ||
| return new String(messageDigest.digest()); | ||
| } catch (NoSuchAlgorithmException ignored) { | ||
|
|
| /** | ||
| * Asynchronous task used to report order to the Button API. | ||
| */ | ||
| class PostOrderTask extends Task { |
There was a problem hiding this comment.
I think you misspelled AsyncTask 😛
/s
| private final Order order; | ||
| private final DeviceManager deviceManager; | ||
|
|
||
| PostOrderTask(@Nullable Listener listener, ButtonApi buttonApi, Order order, |
There was a problem hiding this comment.
nit: I would move the nullable listener to end of the argument list
There was a problem hiding this comment.
Just being consistent at this point with the other tasks
| * Represents an order placed by the user to be tracked using ButtonMerchant.trackOrder(order). | ||
| * Represents an order placed by the user to be tracked using | ||
| * {@link ButtonMerchant#trackOrder(Context, Order, UserActivityListener)} and | ||
| * {@link ButtonMerchant#reportOrder(Context, Order, OrderListener)} | ||
| */ | ||
| public class Order { |
There was a problem hiding this comment.
Methods with javadocs that have the @Deprecated annotation need a pairing @deprecated javadoc annotation. Otherwise they will not be marked deprecated when we generate the docs.
| /** | ||
| * Callbacks for report order | ||
| * | ||
| * @see ButtonMerchant#reportOrder(android.content.Context, Order, OrderListener) |
There was a problem hiding this comment.
nit: For consistency with other javadoc headers, replace the full package qualification of Context with an import
There was a problem hiding this comment.
Checkstyle will throw an unused import error
|
|
||
| @Nullable | ||
| @WorkerThread | ||
| Void postOrder(Order order, String applicationId, String sourceToken, |
There was a problem hiding this comment.
Why postOrder and not reportOrder?
Edit: I see reportOrder else where so I don't know what this method is
There was a problem hiding this comment.
The naming convention throughout the entire ButtonApi class is requestMethod + endpoint. reportOrder is the public facing method
Goals ⚽
Implementation 🚧
ApiRequestto allow a better way to build requestspostfromConnectionManagerin favor ofexecuteButtonUtilfor utility methods used in multiple placesPostOrderTaskto report ordersTesting 🔍