-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(api): allow to get scenarios solutions results #264
Conversation
This pull request is being automatically deployed with Vercel (learn more). marxan – ./app🔍 Inspect: https://vercel.com/vizzuality1/marxan/DxdVYKuw1v3W64KWq5hVkn6Tmw7Q marxan-storybook – ./app🔍 Inspect: https://vercel.com/vizzuality1/marxan-storybook/Cd8BQRSssXtYh6yvV8j1wt2rJE7b |
9a6e65d
to
3e318b1
Compare
3e318b1
to
78a43b9
Compare
78a43b9
to
a9e761f
Compare
a9e761f
to
66e90cd
Compare
api/apps/api/src/modules/scenarios/dto/scenario-solution-result.dto.ts
Outdated
Show resolved
Hide resolved
@Query('best', ParseBoolPipe) selectOnlyBest?: boolean, | ||
@Query('most-different', ParseBoolPipe) selectMostDifferent?: boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they seem to be mutually exclusive
maybe it's not a problem, though
also a dto could be used here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, they are exclusive (and returning different DTO). Any idea how to show it in code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
honestly I'd create another method and route for it, trying to show it in the code with one method is tricky (especially when you'd like to keep it in the openapi spec) and can be not worth the effort
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd go with @Dyostiq 's suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or make bestSolution
and oneOfFiveMostDifferentSolutions
(or something like this) column-based props of the entity, which would allow to filter via ?filter[bestSolution]=true
or ?filter[oneOfBlaBla]=true
.
but semantically I prefer separate endpoints - if we expose these props as part of the entity then API consumers could as well just filter client-side, IMO.
:id/marxan/run/:runId/solutions/best
:id/marxan/run/:runId/solutions/most-different
at the expense of some controller handler boilerplate duplication, though a minor evil here IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kgajowy should we remove in a new PR the best
and most-different
queries and relative code paths in this controller, or did you want to leave these in to let API clients query for best and most different solutions in either way? (I am fine either way, though I'd prefer to have only one way to query for best and most efficient, via their own endpoint).
@Query('best', ParseBoolPipe) selectOnlyBest?: boolean, | ||
@Query('most-different', ParseBoolPipe) selectMostDifferent?: boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd go with @Dyostiq 's suggestion
@Query('best', ParseBoolPipe) selectOnlyBest?: boolean, | ||
@Query('most-different', ParseBoolPipe) selectMostDifferent?: boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or make bestSolution
and oneOfFiveMostDifferentSolutions
(or something like this) column-based props of the entity, which would allow to filter via ?filter[bestSolution]=true
or ?filter[oneOfBlaBla]=true
.
but semantically I prefer separate endpoints - if we expose these props as part of the entity then API consumers could as well just filter client-side, IMO.
:id/marxan/run/:runId/solutions/best
:id/marxan/run/:runId/solutions/most-different
at the expense of some controller handler boilerplate duplication, though a minor evil here IMO.
This PR mainly touches base implementation of an API layer and leaves space for actual implementation; includes:
It may happen that some
runId
or other fields aren't correct (according to discussion on slack) but I would opt in for leaving it for future PR once it is well settled.