Terms of Service - #484
Conversation
|
Verified that @carycheng77 has signed the CLA. Thanks for the pull request! |
| } | ||
|
|
||
| return new BoxResourceIterable<BoxTermsOfService.Info>(api, | ||
| ALL_TERMS_OF_SERVICES_URL_TEMPLATE.buildWithQuery(api.getBaseURL(), builder.toString()), limit) { |
There was a problem hiding this comment.
buildwith query as a separate line for better readbility.
| this.modifiedAt = BoxDateFormat.parse(value.asString()); | ||
| } | ||
| } catch (ParseException e) { | ||
| assert false : "A ParseException indicates a bug in the SDK."; |
There was a problem hiding this comment.
Please change the comment
There was a problem hiding this comment.
Hey @KarthikShanmugasundaram wondering comment we should put here. Reason being I followed how it was built in BoxRetentionPolicy, BoxDevicePin, and other files and the ParseException e is followed by: "A ParseException indicates a bug in in the SDK". Any input is greatly appreciated!
There was a problem hiding this comment.
For example: Terms of Service Parsing failed. {the message from parse exception to understand more on the error)
| } | ||
|
|
||
| return new BoxResourceIterable<BoxTermsOfServiceUserStatus.Info>(api, | ||
| ALL_TERMS_OF_SERVICE_USER_STATUSES_TEMPLATE.buildWithQuery(api.getBaseURL(), builder.toString()), |
There was a problem hiding this comment.
Please move the statement as a separate one.
| this.type = value.asString(); | ||
| } else if (memberName.equals("tos_type")) { | ||
| this.tosType = value.asString(); | ||
| } else if (memberName.equals("text")) { |
There was a problem hiding this comment.
value is mandatory field? if there is any optional fields, the null checks are done?
| String userID = userJSON.get("id").asString(); | ||
| BoxUser user = new BoxUser(getAPI(), userID); | ||
| this.user = user.new Info(userJSON); | ||
| } else if (memberName.equals("is_accepted")) { |
There was a problem hiding this comment.
if there is any optional fields, the null checks are done?
| BoxAPIConnection api = new BoxAPIConnection(TestConfig.getAccessToken()); | ||
| String newStatus = "enabled"; | ||
| String newText = "New text"; | ||
| BoxTermsOfService termsOfService = new BoxTermsOfService(api, "2778"); |
There was a problem hiding this comment.
Please add more negative test cases
| @Test | ||
| @Category(IntegrationTest.class) | ||
| public void getUserStatusInfoOnTermsOfServiceWithUserIDSucceeds() { | ||
| BoxAPIConnection api = new BoxAPIConnection(TestConfig.getAccessToken()); |
There was a problem hiding this comment.
Please add more negative test cases
| * [Create a Terms of Service](#create-a-terms-of-service) | ||
| * [Edit a Terms of Service](#edit-a-terms-of-service) | ||
| * [Get a Terms of Service](#get-a-terms-of-service) | ||
| * [Get all Terms of Service](#get-all-terms-of-service) |
There was a problem hiding this comment.
Should be Services as it's more than one
| BoxTermsOfService.Info newUserStatus = BoxTermsOfServiceUserStatus.create(api, "tos-id", true, "user-id"); | ||
| ``` | ||
|
|
||
| You can only create a new user status on a terms of service if the user has never accepted/declines a terms of service. |
There was a problem hiding this comment.
accepted/decline_d_ seems right usage
| "2778", ""); | ||
|
|
||
| } catch (Exception e) { | ||
| fail("Exception during test execution: " + e); |
There was a problem hiding this comment.
Can you validate that the exception and message is what you anticipate to be thrown
| "", ""); | ||
|
|
||
| } catch (Exception e) { | ||
| fail("Exception during test execution: " + e); |
There was a problem hiding this comment.
Can you validate that the exception and message is what you anticipate to be thrown
| tosUserStatus.updateInfo(userStatusInfo); | ||
|
|
||
| } catch (Exception e) { | ||
| fail("Exception during test execution: " + e); |
There was a problem hiding this comment.
Can you validate that the exception and message is what you anticipate to be thrown
| userStatusInfo.setIsAccepted(isAccepted); | ||
| tosUserStatus.updateInfo(userStatusInfo); | ||
|
|
||
| assertThat(userStatusInfo.getIsAccepted(), is(equalTo(isAccepted))); |
There was a problem hiding this comment.
Use assertEqual fo simplicity
| userStatusInfo.setIsAccepted(true); | ||
| toUserStatus.updateInfo(userStatusInfo); | ||
|
|
||
| Assert.assertEquals(isAccepted, userStatusInfo.getIsAccepted()); |
There was a problem hiding this comment.
Assert other important fields are being set
| api.setRequestInterceptor(JSONRequestInterceptor.respondWith(fakeJSONResponse)); | ||
| BoxTermsOfServiceUserStatus toUserStatus = new BoxTermsOfServiceUserStatus(api, "1939280"); | ||
| BoxTermsOfServiceUserStatus.Info userStatusInfo = toUserStatus.new Info(); | ||
| userStatusInfo.setIsAccepted(true); |
Thanks Karthik!