Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Resend verification email functionality #120

Merged
merged 39 commits into from
Jun 11, 2018

Conversation

minhlongdo
Copy link
Contributor

No description provided.

@minhlongdo minhlongdo changed the title Add Resend verification email functionality #62 Add Resend verification email functionality Apr 26, 2018
private String id;

@JsonCreator
public Job(@JsonProperty("status") String status, @JsonProperty("type") String type, @JsonProperty("id") String id) {
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 for the time being, this doesn't need to be public. Can you try making it private or package protected at least? See if Jackson complains.

}

@JsonProperty("created_at")
public void setCreatedAt(String createdAt) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any sample values for this created_at field in the api docs. But probably would be better to just use a Date and make Jackson convert between the types. I think the API already makes use of the ISO-8601 format. See the User's class created_at method. If this is the case, then you'll also need to update the JSON samples used in the introduced tests.

* @param recipient the email verification recipient.
* @return a Request to execute.
*/
public Request<Job> sendVerificationEmail(EmailRecipient recipient) {
Copy link
Contributor

Choose a reason for hiding this comment

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

2 parameters seems fine to have them expanded on the signature. Why do you suggest to use this new EmailRecipient class? Maybe it's more straight forward to validate the user id value (non-null) and let the client id one be optional (nullable). Java-docs would clarify each one's validation respectively.

@lbalmaceda
Copy link
Contributor

@minhlongdo are you still interested on this?

@minhlongdo
Copy link
Contributor Author

@lbalmaceda Yes, I am looking at it now. Sorry for the delay.

@minhlongdo
Copy link
Contributor Author

@lbalmaceda could you try to kick off the CI again please? It compiles on my laptop. Thanks! :)

minhlongsmbp2:auth0-java minhlongdo$ ./gradlew test
Using version 1.6.0-SNAPSHOT for auth0
:compileJava UP-TO-DATE
:processResources UP-TO-DATE
:classes UP-TO-DATE
:compileTestJava UP-TO-DATE
:processTestResources UP-TO-DATE
:testClasses UP-TO-DATE
:test UP-TO-DATE

BUILD SUCCESSFUL

Total time: 4.151 secs```

@lbalmaceda
Copy link
Contributor

@minhlongdo because we still support JDK 7 you need to change this line https://github.com/auth0/auth0-java/pull/120/files#diff-a5e9c9a379e91d2a7b2790bae000ed6bR35 and cast the second parameter to Object, like we do in many other tests.

assertThat(body, hasEntry("user_id", (Object) "google-oauth2|1234"));

@minhlongdo
Copy link
Contributor Author

@lbalmaceda ok thank you for pointing it out, I am unable to test it with JDK 7 :(

@minhlongdo
Copy link
Contributor Author

@lbalmaceda Do you want me to do some squashing?

@lbalmaceda
Copy link
Contributor

I'll review this PR again tomorrow. You can squash them if you want or I can do that myself when I merge it. ⚡️

Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

I left some final comments. Most of them are just string or docs changes.

* @return a Request to execute.
*/
public Request<Job> sendVerificationEmail(String userId, String clientId) {
Asserts.assertNotNull(userId, "recipient");
Copy link
Contributor

Choose a reason for hiding this comment

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

I get what you mean by recipient, but the parameter is called userId so this exception message is a bit miss-leading. Either change the string in this line to "user id" or clarify in the param's javadoc that the value is the recipient of the email.

CustomRequest<Job> request = new CustomRequest<>(client, url, "POST", new TypeReference<Job>() {
});
request.addHeader("Authorization", "Bearer " + apiToken);
request.setBody((Object) requestBody);
Copy link
Contributor

Choose a reason for hiding this comment

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

this one doesn't need to be casted to Object.

@@ -231,4 +231,13 @@ public TicketsEntity tickets() {
public ResourceServerEntity resourceServers() {
return new ResourceServerEntity(client, baseUrl, apiToken);
}

/**
* Getter for Jobs entity.
Copy link
Contributor

Choose a reason for hiding this comment

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

Getter for the Jobs entity


@Test
public void shouldSendUserAVerificationEmailWithClientId() throws Exception {
Request<Job> request = api.jobs().sendVerificationEmail("google-oauth2|1234", "google-oauth2|client-id");
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is just a test, but a client id value looks different. Try something similar to AaiyAPdpYdesoKnqjj8HJqRn4T5titww as used in another test please.

}

@Test
public void shouldThrowOnNullEmailRecipient() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on what you choose to fix the other comment, this test might need to be renamed.

@lbalmaceda lbalmaceda added this to the v1-Next milestone Jun 11, 2018
@lbalmaceda lbalmaceda merged commit 3c408c0 into auth0:master Jun 11, 2018
@lbalmaceda lbalmaceda modified the milestones: v1-Next, 1.7.0 Jun 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants