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.
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
[GridDyna] Add some batch CRUD endpoints #111
[GridDyna] Add some batch CRUD endpoints #111
Changes from 13 commits
fdd3458
6a29931
3680536
2549a85
db4c530
588ae16
dcebe97
c508737
4834285
1190a4b
2251b0c
c0ac2fc
d508e0f
f551b4c
54201b9
e8236a8
2f3c535
0e8609f
1f22f0b
d11226a
cb40b78
6c2ba17
bd50593
0713dfc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Done, I have a slight change from your suggestion to "The filter with given id has been successfully duplicated"
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.
Adding
duplicate
looks like a good way to differentiate the two/filters/batch
but I don't get the fact that this has not be done for the 2@PostMapping(value = "/filters")
(the creation and the duplication have the same path).Our rest API best practices are not very precise about extra data like this. But they state that the path should only be used to identify a data.
@etiennehomer shouldn't we modify the filters duplication signature in an other PR ?
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.
We should add "duplicate" in the path of duplicate endpoint, i.e. POST /filters/duplicate but we have to changer also in gridexplorer-app. I think we can change in another TS
In fact we should have :
POST /filters
POST /filters/batch
POST /filters/duplicate
POST /filters/duplicate/batch
At least I can change from /filters/batch/duplicate to /filters/duplicate/batch, what do you think??? @etiennehomer @Mathieu-Deharbe
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.
Ok. I think the API you propose is good Thang. So let's change POST /filters/batch/duplicate to POST /filters/duplicate/batch
And then, in another PR, let's change for unitary duplication endpoint
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.
DONE in 0713dfc
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.
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.
Done, I have a slight change from your suggestion to "Filters of given ids have been successfully duplicated"
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.
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.
This method is quite complicated. As we can assume only few filters will be updated at one time, you can iterate on filtersToModifyMap and call changeFilter.
And rename changeFilter to updateFilter
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.
Done in 0e8609f