feat: add support for update report definition endpoint#157
feat: add support for update report definition endpoint#157smcgowan-smartsheet wants to merge 10 commits intosmartsheet:mainlinefrom
Conversation
Latest commit removed
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
Review complete. Found 2 minor issues related to formatting and naming conventions.
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
![roomote-v0[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
remove invalid options
add update filters query param support
fix prettier linting issue
| updateFilters: true, | ||
| customProperties: { | ||
| 'x-request-id': requestId, | ||
| 'x-test-name': '/reports/update-report-definition/all-response-body-properties' |
There was a problem hiding this comment.
The x-test-name value should be unique for this test since it's specifically testing query parameter handling. Using the same test name as other tests (/reports/update-report-definition/all-response-body-properties) will cause this test to use the same WireMock mapping as the all-properties test, which may not have the expected query parameters configured. Consider using a more specific name like /reports/update-report-definition/with-update-filters-query-param.
Fix it with Roo Code or mention @roomote and request a fix.
change aggregationCriteria to summarizingCriteria to match spec add returns report definition
| updateReportDefinition: ( | ||
| options: UpdateReportDefinitionOptions, | ||
| callback?: RequestCallback<BaseResponseStatus> | ||
| ) => Promise<BaseResponseStatus>; |
There was a problem hiding this comment.
The return type BaseResponseStatus doesn't match the actual API response structure. The test expects a result property containing the updated ReportDefinition, but BaseResponseStatus only has message and resultCode. This creates a type contract violation where the actual response includes properties not reflected in the type definition. Consider creating a specific response type like UpdateReportDefinitionResponse extends BaseResponseStatus with a result?: ReportDefinition property, similar to how SetReportPublishStatusResponse is structured.
Fix it with Roo Code or mention @roomote and request a fix.
changed to PUT from PATCH no longer returning changed definition no longer needs query param
| expect(parsedUrl.pathname).toEqual(`/2.0/reports/${TEST_REPORT_ID}/definition`); | ||
| }); | ||
|
|
||
| it('updateReportDefinition uses PATCH method', async () => { |
There was a problem hiding this comment.
The test name claims to verify PATCH method usage, but the test expects PUT and the implementation uses requestor.put(). This creates confusion about which HTTP method is actually being used. Either update the test name to reflect PUT, or change the implementation to use PATCH if that's what the API requires.
Fix it with Roo Code or mention @roomote and request a fix.
| updateFilters: patchOptions.updateFilters, | ||
| }, | ||
| }; | ||
| return requestor.put({ ...optionsToSend, ...urlOptions, ...patchOptions }, callback); |
There was a problem hiding this comment.
The implementation uses requestor.put() which sends a PUT request, but the API documentation in the JSDoc (line 193 of types.ts) explicitly states this mirrors PATCH /reports/{reportId}/definition. This HTTP method mismatch means the SDK is calling the wrong endpoint method. The implementation should use requestor.patch() to match the documented API contract.
| return requestor.put({ ...optionsToSend, ...urlOptions, ...patchOptions }, callback); | |
| return requestor.patch({ ...optionsToSend, ...urlOptions, ...patchOptions }, callback); |
Fix it with Roo Code or mention @roomote and request a fix.
1 participant
Add support for PATCH /reports/{reportId}/definition endpoint