search

smartsheet / smartsheet-java-sdk

Library that uses Java to connect to Smartsheet services.
Apache License 2.0
5 stars 13 forks source link

Add Support for Pagination for listUsers #86

Closed apederson94 closed 1 month ago

apederson94 commented 6 months ago

Adds support for using listUsers with pagination. Minor refactoring work in order to clean up the way that the endpoint was being called internally. Should be more consistent now in the way things are done. Addresses #40

zromano commented 6 months ago

Is it possible for us to add some new tests for this?

apederson94 commented 6 months ago

Is it possible for us to add some new tests for this?

Sure. I'm not sure what kind of tests you're looking for so if you could tell me what you're looking for, that'd be great 😄

ronreynolds commented 6 months ago

we have Jacoco - why not make it based on a minimum test-coverage percent greater than whatever we have today? (specific to this area/new code of course).

zromano commented 6 months ago

Yeah, I was thinking some new unit tests in UserResourcesImplTest to cover these conditions.

If you run ./gradlew clean build jacocoTestReport you'll be able to see the jacoco html report and you can see the coverage of that individual file

We could probably add a story to make it easier to see the coverage of new code

apederson94 commented 3 months ago

LGTM. Do we have to update any documentation anywhere to indicate that we now have support for this ?

Since this isn't a huge change to functionality but just adding a new way to call a method that already exists, I don't think we really need to do much outside of just updating the changelog. @zromano and @ronreynolds may have thoughts about this though and I would defer to their judgement

ronreynolds commented 3 months ago

LGTM. Do we have to update any documentation anywhere to indicate that we now have support for this ?

Since this isn't a huge change to functionality but just adding a new way to call a method that already exists, I don't think we really need to do much outside of just updating the changelog. @zromano and @ronreynolds may have thoughts about this though and I would defer to their judgement

so DID the API contract change (and thus the SDK is changing to keep the client in sync with the behavior of the server)? the API documentation seems related (tho very loosely coupled) to the SDK functionality (tho having the SDK implement something NOT in the API documentation seems a bit funky IMHO).

apederson94 commented 2 months ago

It looks like this is just a use case that we didn't support but that the API would support since there are default values for the pagination parameters according to the docs here: https://smartsheet.redoc.ly/tag/users#operation/list-users

Given this, I don't think there are any documentation changes that need to be made and this should be good to merge. Unless there is documentation in this project that I am unaware of that would need to be updated

apederson94 commented 2 months ago

@ronreynolds What do you think?

It looks like this is just a use case that we didn't support but that the API would support since there are default values for the pagination parameters according to the docs here: https://smartsheet.redoc.ly/tag/users#operation/list-users

Given this, I don't think there are any documentation changes that need to be made and this should be good to merge. Unless there is documentation in this project that I am unaware of that would need to be updated