CDA-98 - Creating /v2/forecast-spec endpoint using updated spec struture - #1872
Conversation
28a2e7d to
bad214e
Compare
rma-bryson
changed the title
MikeNeilson
left a comment
MikeNeilson
left a comment
There was a problem hiding this comment.
Looks reasonable, though I agree with @adamkorynta 's comments. Good to see a screenshot of the UI.
c9a25ff to
148f8dc
Compare
|
Swagger UI screenshots: 
|
011be32 to
8c3d3b8
Compare
| + "spec id of the forecast spec whose data is to be deleted."), | ||
| }, | ||
| queryParams = { | ||
| @OpenApiParam(name = OFFICE, required = true, description = "Specifies the " |
There was a problem hiding this comment.
should we take the opportunity of the v2 to move office into a path param?
/v2/<office>/forecast/<name>
or
/v2/forecast/<office>/<name>
We started doing this on some other endpoints
There was a problem hiding this comment.
Proposed:
Office filtering is not supported on the primary resource identified by the endpoint path, as the office context is already established by the route. If multi-office is needed (which I don't believe is often the case), then subsequent requests for each office can be made. Office-based filters are only supported for related resources and are expressed as query parameters, but not on the primary resource.
Example:
/v2/SWT/forecast-spec
Example of filtering on related resource:
/v2/SWT/location-group?category-office=CWMS
There was a problem hiding this comment.
I think we should prefer /v2/forecast/<office>/<name>, as it reads like "I want the forecast, for office X, named Y. where as /v2/<office>/forecast/<name> reads "I want, for office X, the forecast named Y.... which now that I type out I'm not actually sure which one is better as they both read reasonably well.
There was a problem hiding this comment.
Updated to include /v2/forecast-spec/{office}/{name}
|
Screenshot with office in path param: 
|
…ture. Updates OpenApiDocTest to check parent controller class. Adds route configuration support for v2 endpoints.
… example for forecast location.
…iguration into utility class.
af5ac6b to
b9c6dd4
Compare
|
Need to fix failing OpenApi static analysis test - related to not properly detecting how office path param is being used. |
b9c6dd4 to
337539c
Compare
| ) | ||
| @Override | ||
| public void update(@NotNull Context ctx, @NotNull String name) { | ||
| logUnusedPathParameter(ctx, OFFICE, "Body contains information"); |
3 participants
Summary
Established use of /v2/ with forecast-spec update to use locations with sort-order
Related Issue
Closes #1648
Validation
Unit tests and Integration tests
Checklist