Skip to content

Conversation

@thewheat
Copy link
Contributor

Add support for permanent deletion https://developers.intercom.com/reference#delete-users
Deprecate delete and replace it with archive

Copy link

@eripe970 eripe970 left a comment

Choose a reason for hiding this comment

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

Just some smaller changes.

@@ -0,0 +1,3 @@
{
"id": 123456
} No newline at end of file
Copy link

Choose a reason for hiding this comment

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

Missing newline.

private String intercom_user_id;
public UserPermanentDeleteRequest() {
}
public UserPermanentDeleteRequest(String intercom_user_id) {
Copy link

Choose a reason for hiding this comment

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

Better naming to call intercom_user_id => intercomUserId?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes thanks for catching that 👍

@SuppressWarnings("UnusedDeclaration")
@JsonIgnoreProperties(ignoreUnknown = true)
@JsonInclude(JsonInclude.Include.NON_EMPTY)
public class UserPermanentDeleteRequest {
Copy link

Choose a reason for hiding this comment

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

I think this should be renamed to PermanentlyDeleteUserRequest.

@SuppressWarnings("UnusedDeclaration")
@JsonIgnoreProperties(ignoreUnknown = true)
@JsonInclude(JsonInclude.Include.NON_EMPTY)
public class UserPermanentDeleteResponse {
Copy link

Choose a reason for hiding this comment

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

I think this should be renamed to PermanentlyDeleteUserResponse.

/**
* @deprecated Replaced by {@link #archive(String)}. Renamed for consistency with API language
*/
public static User delete(String id){
Copy link

Choose a reason for hiding this comment

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

Minor, formatting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review @eripe970. Not sure I get exactly which formatting that you're referring to here so would be great if I could get a bit more details.

Will change some the issues that I've responded to but will leave the naming convention to the other dev teammates to take a look at 😄

Copy link

Choose a reason for hiding this comment

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

Cool, normally it's a space before the {, like delete(String id){ => delete(String id) {. At least I think that's the convention that is used in a lot of code-bases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah got it 👍 I do see the other files do have that space so added it in

@thewheat thewheat force-pushed the timlim/add-permanent-delete branch 2 times, most recently from f32447b to 168dfff Compare June 4, 2018 09:05
@thewheat thewheat force-pushed the timlim/add-permanent-delete branch from 168dfff to 0ec24fc Compare June 4, 2018 09:40
Copy link
Member

@choran choran left a comment

Choose a reason for hiding this comment

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

👍

@choran choran merged commit 6b8ac12 into master Jun 8, 2018
@eripe970
Copy link

Is it possible to do a new release that includes this PR @choran?

@thewheat
Copy link
Contributor Author

@eripe970 looks like version 2.4.0 does contain the code in question

image

Also available in bintray (Updated on 8th June): https://bintray.com/intercom/intercom-maven/intercom-java/2.4.0

image

Updated my dependency in my local code and I am able to make a permanent deletion request 😄

image

@eripe970
Copy link

Thanks! 👍 Missed that it was a new release out!

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.

4 participants