-
Notifications
You must be signed in to change notification settings - Fork 0
[TM-2096] Delete polygon function #335
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
base: staging
Are you sure you want to change the base?
Conversation
@ExceptionResponse(UnauthorizedException, { description: "Authentication failed." }) | ||
@ExceptionResponse(NotFoundException, { description: "Site polygon not found." }) | ||
async deleteOne(@Param("uuid") uuid: string) { | ||
await this.policyService.authorize("deleteAll", SitePolygon); |
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.
It's weird for this delete one function to require deleteAll permissions. Is there no way at the policy level to narrow this down so that some folks can delete specific polygons without needing to be able to delete all of them? How will this endpoint be used?
Also, I'm surprised to see no updates to the site polygon policy. Is this deleteAll
permission already defined from some other feature?
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've updated the implementation to use instance-based authorization instead of "deleteAll". The endpoint deletes all versions of a polygon (via primaryUuid) plus associated data, in the Polygon Review tab we have the option to delete a polygon, so we are planning to use this endpoint there.
|
||
const sitePolygonIds = relatedSitePolygons.map(sp => sp.id); | ||
const polygonUuids = relatedSitePolygons.map(sp => sp.polygonUuid).filter((uuid): uuid is string => uuid != null); | ||
const pointUuids = relatedSitePolygons.map(sp => sp.pointUuid).filter((uuid): uuid is string => uuid != null); |
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.
Why is uuid is string
needed 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.
I used this cause sp.polygonUuid
and sp.pointUuid
are typed as string
but we have the decorator @AllowNull
(we have records with point_id
as null
in site_polygon
table) and this predicate tells if a uuid
is definitely a string
preventing errors when passed to Sequelize queries.
https://gfw.atlassian.net/browse/TM-2096