Skip to content

Conversation

@andreiruse
Copy link

The use case for adding this is being able to iterate over the recent events triggered by a given user, for reporting purposes. The REST API is already able to do so using a single GET request, given the user ID/e-mail/intercom ID.

I was not able to figure a quick way to write a unit test, without triggering an external HTTP connection (ideally, this would happen with a mock web server).

This addition is corresponding to this section of the API docs: https://developers.intercom.com/v2.0/reference#list-user-events

@thewheat
Copy link
Contributor

thewheat commented Nov 4, 2017

Thanks so much for this @andreiruse 😄 I'll review and test the code in the next week.

For tests you can always take a look at the current test set and create a similar one but if you're not able to, not to worry as we can add them in too 👍

Copy link
Contributor

@thewheat thewheat left a comment

Choose a reason for hiding this comment

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

Managed to take a look at this and a couple of comments below

  • Best to refactor to a list call to be consistent with current code
  • Add example code to Readme
  • Add tests if possible (maybe similar to loading conversations: could load a json file and verify number of entries as well as actual data read?)

Let me know if you're good to make those changes or if you had any other comments on this @andreiruse

return Job.listJobErrorFeed(jobID, Event.class);
}

public static EventCollection listBy(String type, String parameterName, String parameterValue) {
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 to keep things consistent with the rest of the library and our Idioms https://github.com/intercom/intercom-java/#pagination, this should be refactored to be a list call which takes an input of Map<String, String> params. Seems like we could utilise a similar pattern as the Notes and NotesCollection here 😄

Some examples of using list in the current code

It would be good to also add some sample code to the README.md file to indicate how to utilise this new endpoint

@thewheat
Copy link
Contributor

Hi @andreiruse just wanted to check in to see if you were keen and able to make the changes. If not, no worries 😄 I'll be happy to work on what you have and build from there to get this merged! 👍

@andreiruse
Copy link
Author

@thewheat Hey,
Apologies for the delay. Yes, I am keen to address this PR & look into adding tests; just didn't get the time yet. Will give it a try in the next few days.

@thewheat
Copy link
Contributor

Fantastic! If you do run into any roadblocks, let me know and we can even tag team on things if needed 😁

@thewheat
Copy link
Contributor

Hey @andreiruse , just wanted to find out how things are going? I'm looking to finalise this PR in the new year so do let me know if you're still good finishing up on this otherwise I'll just work with what you have so that we can get the functionality in the library 👍

@thewheat
Copy link
Contributor

thewheat commented Jan 2, 2018

I'm closing this PR as I have addresses the changes in #173 😄

@thewheat thewheat closed this Jan 2, 2018
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.

2 participants