Skip to content
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

return freshly updated GeoFeature in response to PATCH request [MRXN23-255] #1392

Conversation

hotzevzl
Copy link
Member

Overview

Return the updated GeoFeature in PATCH responses when users update a feature's name

Feature relevant tickets

https://vizzuality.atlassian.net/browse/MRXN23-255


Checklist before submitting

  • Meaningful commits and code rebased on develop.
  • If this PR adds feature that should be tested for regressions when
    deploying to staging/production, please add brief testing instructions
    to the deploy checklist (docs/deployment-checklist.md)
  • Update CHANGELOG file

@vercel
Copy link

vercel bot commented Jul 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
marxan ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 26, 2023 9:59am

@hotzevzl hotzevzl force-pushed the chore/api/MRXN23-255_return-updated-feature-in-patch-responses branch from 4394b78 to 6829f3a Compare July 25, 2023 15:56
@hotzevzl
Copy link
Member Author

@KevSanchez this is basically changing the response payload for PATCH request to edit feature names, but I took a chance to also:

  • move this endpoint to the geo-features controller - I don't remember why we decided to put this into the projects controller but I think that it's simpler in the geo-features controller (no need to supply a projectId) and also documentation-wise it's grouped with feature stuff in the OpenAPI/Swagger page
  • document that in general we should return created/updated entities in responses to PATCH/PUT requests too
  • refactor error handling to use the infamous mapAclDomainToHttpError() strategy
  • and of course I've added a small test to check that the response payload is now in JSON:API format (kind of test this, I'm only asserting a couple of things, not really using a JSON:API parser for this)
  • i've also removed a test that is now obsolete (as we don't want a projectId from the API consumer anymore, there's no point in asserting that the related project exists, so I've removed that check from the code and done it kind of indirectly by "just" asserting that a feature with the gived id exists and it is not a platform-wide one, which means it must be in a project, and we then check that the user can do stuff in that project)

@KevSanchez
Copy link
Collaborator

@hotzevzl Pretty much spot on
I also don't remember exactly the motivation we had for putting the endpoint there in the projects controller. Maybe because editing features wouldn't make sense outside the context of a project (as only custom features are editable, and you would always need the corresponding projectId)?
In any case, this is good to go 👍

@hotzevzl hotzevzl merged commit 07d51c3 into develop Jul 27, 2023
52 checks passed
@hotzevzl hotzevzl deleted the chore/api/MRXN23-255_return-updated-feature-in-patch-responses branch July 27, 2023 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants