Support include=planName on List User Plans - #189
Open
adecounter wants to merge 2 commits into
Open
Conversation
GET /2.0/users/{userId}/plans gained an optional `include` query
parameter whose only accepted value is `planNames`. When requested, each
returned plan carries the name of its owning organization.
- A new `UserResources.listUserPlans` overload accepts
`EnumSet<UserPlanInclusion>`, serialized comma-separated by
`QueryUtil.generateCommaSeparatedList` like every other `include`
parameter in the SDK. The existing four-argument signature is
unchanged and delegates to the overload, so callers are unaffected.
- `UserPlan` gains `planName`. It is optional: absent unless the
enrichment is requested, and absent for a plan whose owning
organization has no name, so it deserializes to null in both cases.
Organization names are cached server side for several hours, which the
javadoc notes since a recently renamed organization may briefly report
its previous name.
Mock tests cover the query parameter, a plan with a name, and a plan
without one. They depend on `planName` being added to the List User
Plans all-properties mapping in smartsheet-sdk-tests, which must merge
first.
📝 WalkthroughWalkthroughThe SDK adds ChangesUser plan inclusion
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant UserResources
participant UserResourcesImpl
participant ListUserPlansAPI
Caller->>UserResources: listUserPlans(..., PLAN_NAME)
UserResources->>UserResourcesImpl: Forward inclusion set
UserResourcesImpl->>ListUserPlansAPI: Request include=planName
ListUserPlansAPI-->>UserResourcesImpl: Return UserPlan records
UserResourcesImpl-->>Caller: Return paginated results with planName
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The public API's include value is singular `planName`, matching the response field it controls. The gateway rewrites it to the downstream's plural spelling, so no SDK-side bridging is needed.
| String lastKey, | ||
| Long maxItems, | ||
| Boolean displayContributorSeatType, | ||
| EnumSet<UserPlanInclusion> includes |
Contributor
There was a problem hiding this comment.
nit:
Suggested change
| EnumSet<UserPlanInclusion> includes | |
| EnumSet<UserPlanInclusion> include |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Summary
GET /2.0/users/{userId}/plansgained an optionalincludequery parameter whose only accepted value isplanName, matching the response field it controls. When requested, each returned plan carries the name of the organization that owns it. This adds SDK support for it.New
listUserPlansoverload.UserResources.listUserPlansgains a five-argument overload takingEnumSet<UserPlanInclusion>, serialized comma-separated viaQueryUtil.generateCommaSeparatedListlike every otherincludeparameter in the SDK. The existing four-argument signature is untouched and simply delegates to the overload passingnull, so source compatibility is preserved and existing callers need no changes.Omitting the parameter is byte-identical to today. The
includekey is only added to the parameter map whenincludes != null, so callers on the original signature produce exactly the same request URL as before this change.New
planNamefield onUserPlan. It is optional and deserializes tonullin two distinct cases: when the enrichment was not requested, and when the owning organization has no name. Anulltherefore does not by itself mean the enrichment was not requested, which the javadoc calls out.Design note
The inclusion enum follows existing precedent rather than introducing a new pattern: 23 inclusion enums already exist in
models/enums/, andlistUsersin this same interface already takes anEnumSet<ListUserInclusion>.Caching caveat
Organization names are cached server side for roughly four hours. A recently renamed organization may briefly report its previous name through this field. This is documented in the javadoc on both the overload and
UserPlan.getPlanName()so callers are not surprised.Files changed
src/main/java/com/smartsheet/api/UserResources.javalistUserPlansoverloadsrc/main/java/com/smartsheet/api/internal/UserResourcesImpl.javasrc/main/java/com/smartsheet/api/models/UserPlan.javaplanNameproperty with getter/settersrc/main/java/com/smartsheet/api/models/enums/UserPlanInclusion.javaPLAN_NAME->planNamesrc/test/java/com/smartsheet/api/sdktest/users/TestListUserPlans.javaplanNamedeserializationCHANGELOG.mdTesting
Run against the shared WireMock mappings from
smartsheet/smartsheet-sdk-testsmainline, which now includesplanNameon the List User Plans all-properties mapping../gradlew sdkTest- 181 tests, 179 passed, 0 failures, 2 skipped./gradlew test- 662 tests, 661 passed, 0 failures, 1 skipped./gradlew clean build- BUILD SUCCESSFULAll 3 skips are pre-existing and unrelated to this change (
AutomationRulesTest.updateAutomationRule,SightsTest.copySight,UserResourcesImplTest.testPromoteAlternateEmail).Test coverage in
TestListUserPlansassertsinclude=planNameappears in the request query params, thatgetPlanName()returns"Acme Corporation"for the plan that has one, and that it isnullboth for a plan omitting it and in the required-properties case.One note for reviewers: this change adds a fourth
checkstyleMainviolation,MultipleStringLiteralsfor the string"include"now appearing 3 times inUserResourcesImpl.java. The baseline onmainlineis 3 violations andcheckstyleMainallows up to 5, so the build still passes. Happy to extract a constant if you would prefer to keep the count flat.Checklist
./gradlew clean buildSummary by CodeRabbit
planNamevalue when requested; it remains unavailable when not included.