Skip to content

Conversation

@thewheat
Copy link
Contributor

@thewheat thewheat commented May 30, 2018

Fixes: #196

Added

  • ability to read more attributes:
    • away mode enabled
    • away mode reassignment
    • team_ids (for admins)
    • admin_ids (for teammates)
    • avatar
  • tests for
    • listing all admins
    • viewing an admin
    • viewing a team
  • ability to view admin by id
  • ability to set away mode and reassignment

@thewheat thewheat force-pushed the timlim/update-admin-model branch 2 times, most recently from 1410e42 to bfa8715 Compare May 30, 2018 16:25
@thewheat thewheat changed the title Add support for new Admin endpoints and attributes (Closed: #196) Add support for new Admin endpoints and attributes (Closes: #196) May 30, 2018
@thewheat thewheat force-pushed the timlim/update-admin-model branch from bfa8715 to 3998441 Compare August 13, 2018 08:27
@choran choran added the java label Sep 13, 2018
@choran choran self-assigned this Sep 14, 2018
return DataResource.list(SENTINEL, "admins", AdminCollection.class);
}

public static Admin find(String id)
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we need a quick test for the find?
Just wanted to check if you think this is needed or not

@choran
Copy link
Member

choran commented Sep 14, 2018

Yo Tim,
I looked over this. Firstly, I did not realise there was so little functionality for the admin endpoint in the Java SDK For example. I saw you added find functionality, I did not realise that was missing here. So it is great to have that.

I think it needs a readme update with some simple examples since you are adding the setting and finding options. If you have those handy it might be good to add them. I can merge it then. Alternatively if you dont have time we can merge it now and I can add readme examples later?

The only comment was whether we needed a find test as well.
Lots of great stuff here, nice work @thewheat

@thewheat
Copy link
Contributor Author

@choran definitely agree with respect to the readme. If you're looking to do a release soon I think we should merge and add examples later.

With regards to the tests on find. I don't think we do any testing for find operations in this SDK yet. Should we just look at the requests it makes and verify that it is properly formed e.g. when Admin.find("1234") is called and ensure a request to https://api.intercom.io/admins/1234 is made?

@thewheat thewheat force-pushed the timlim/update-admin-model branch from 3998441 to bb4738e Compare September 28, 2018 08:44
@thewheat
Copy link
Contributor Author

@choran updated readme 👍

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 fb068de into master Oct 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants