Adds internationalisation support and an API to spatialviews #11101
Conversation
…se list of all perm spatial views re archesproject#10704
…om request object re archesproject#10704
…t method of spatial view api re archesproject#10704
… also fixes accessing description from payload re archesproject#10704
…ial view api methods re archesproject#10704
… flug param in API methods re archesproject#10704
merges latest from 7.6.x into branch
merges latest from dev/7.6.x
…oject#10705 - addes specific file-list node label function - fixes broken file-list support - now uses resource descriptors for resource instances - only allows langugeid if belonging to published graph - gives en as default languageid
|
@whatisgalen - added you as reviewer as you did a chunk of this. |
There was a problem hiding this comment.
Hey @aj-he 👋, just following up on the conversation we had about clean(). One of the nice things about relying on full_clean() is that you don't have to reimplement what the model already cleans for you. Some suggestions along those lines below.
(I just had a look at the clean() and urls for now, not the views and tests.)
| language = models.ForeignKey( | ||
| Language, db_column="languageid", to_field="code", on_delete=models.CASCADE | ||
| ) |
There was a problem hiding this comment.
Is there a good reason for us to consider something safer like SET_NULL or PROTECT on these two fields, or are these SpatialView objects pretty ephemeral, and should be OK to wipe out without much worry?
There was a problem hiding this comment.
@jacobtylerwalls - While they are just views on the data, they may be serving key business services so I think they should use PROTECT to ensure they are managed before a Language is removed.
| f"Attribute nodes must belong to the same graph as the geometry node (error nodeid:{str(node.id)})" | ||
| ) | ||
|
|
||
| # language must be be a valid language code beloging to the current publication |
| re_path( | ||
| r"^api/spatialview/(?P<identifier>[a-f0-9\-]+|[\w-]+)/?$", | ||
| api.SpatialView.as_view(), | ||
| name="spatialview_api", | ||
| ), | ||
| re_path(r"^api/spatialview$", api.SpatialView.as_view(), name="spatialview_list"), |
There was a problem hiding this comment.
I'm wondering if you considered using the newer path syntax instead of re_path? AFAICT the regex here is only checking against a UUID? One significant benefit of using path() is that you get a real UUID python type at the top of your view instead of needing to hand-validate that.
| re_path( | |
| r"^api/spatialview/(?P<identifier>[a-f0-9\-]+|[\w-]+)/?$", | |
| api.SpatialView.as_view(), | |
| name="spatialview_api", | |
| ), | |
| re_path(r"^api/spatialview$", api.SpatialView.as_view(), name="spatialview_list"), | |
| path( | |
| "api/spatialview/<uuid:identifier>/", | |
| api.SpatialView.as_view(), | |
| name="spatialview_api", | |
| ), | |
| path("api/spatialview/", api.SpatialView.as_view(), name="spatialview_list"), |
There was a problem hiding this comment.
@jacobtylerwalls - much prefer this as clearer what identifier actually is! The trailing slash on the url with the identifier "feels" unpleasant - it doesn't seem to fit others with a uuid identifier that accesses a single resource. I can understand it for get and post which interact with the collection.
There was a problem hiding this comment.
Good question, we should see what others think, and then I'll be sure to do the same in the controlled list manager.
I guess the first thing is to decide is whether we want to support both forms (for single resource routes). If we do nothing, and just take the example I fleshed out, we get support for both, only just with a 301 for GETs and DELETEs and an error for anything else to the route without a slash. That doesn't bother me so much.
To support routes without a slash (without a 301 for GETs or an error), we could consider:
- subclassing
CommonMiddlewareto turn the 301 redirect into a 307 redirect that will also work for POST/PUT/PATCH - duplicating the urls here, so we have one with a slash and one without
- go back to
re_path, but then we lose the expressiveness of the<uuid>syntax and the auto-conversion in the view - add a custom path converter for optional trailing slashes so we don't have to go back to
re_pathor duplicate the routes - decide not to support a route with a trailing slash
To me, this sounds like overkill for a pretty minor usability/purity thing when manually invoking the API, but I'm open to folks' ideas here.
There was a problem hiding this comment.
Personally, I don't like redirects and forcing a trailing slash at least for individual resources seems non-standard (for example consider this very page or an example Mozilla page https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/301). Neither has a trailing slash. Although I do prefer the "cleanness" of using the newer "path" syntax, I think my suggestion below balances most of what we need, with the only loss being the type conversion, although I'll say that's not really been an issue in our views in the past (models that use UUIDs as fields accept a string representing a UUID). What my suggestion does provide:
- It negates the need for redirects
- It allows for an optional trailing slash
- It also makes the identifier optional allowing for bulk data requests
- it removes the "ugliness" of using regex string in the url directly (and in fact is what we're using everywhere else in urls.py)
- It allows for all http verbs
| re_path( | |
| r"^api/spatialview/(?P<identifier>[a-f0-9\-]+|[\w-]+)/?$", | |
| api.SpatialView.as_view(), | |
| name="spatialview_api", | |
| ), | |
| re_path(r"^api/spatialview$", api.SpatialView.as_view(), name="spatialview_list"), | |
| re_path( | |
| r"^api/spatialview/(?P<identifier>%s|())/?$" % uuid_regex, | |
| api.SpatialView.as_view(), | |
| name="spatialview_api", | |
| ), |
There was a problem hiding this comment.
Nice, that makes good sense if we decide to support both forms without a redirect.
While here, I noticed with that MDN example that if you navigate to the 301 page with a trailing slash, it 301's back to the route without one, so they seem like they're in the camp of "have one route".
There was a problem hiding this comment.
Just following up on our conversation about model validation, that really does unlock a lot of possibilities to simplify the views if you decide to pursue it (and have validation centralized in one place). Had a look at the API views and left comments with that in mind; happy to discuss further when you have a cycle to put to it.
| """ | ||
| Validate the spatial view before saving it to the database as the database triggers have proved hard to test. | ||
| """ | ||
| super().clean() |
There was a problem hiding this comment.
Small nit, but since the base implementation is abstract, the docs model not calling super() in your own.
| spatialview_id = identifier | ||
| if not models.SpatialView.objects.filter(pk=identifier).exists(): | ||
| return JSONErrorResponse( | ||
| _("No Spatial View Exists with this id"), status=404 |
There was a problem hiding this comment.
I think it would create less work for translators if we just used the localized validation errors we get from full_clean().
I found myself doing something like this recently:
except ValidationError as ve:
return JSONErrorResponse(
message="\n".join(ve.messages), status=HTTPStatus.BAD_REQUEST
)| "isactive": spatialview.isactive, | ||
| } | ||
|
|
||
| def create_spatialview_from_json_data(self, json_data): |
There was a problem hiding this comment.
Are there significant differences in this implementation versus just SpatialView(**json_data), namely fields that are being renamed or removing fields that shouldn't be edited (i.e. protecting against overposting)? If so, I wonder if we can just transform the json first to show that explicitly, then run SpatialView(**json_data) to make this both shorter and more self-documenting.
| uuid_pattern = re.compile(settings.UUID_REGEX, re.IGNORECASE) | ||
| return uuid_pattern.match(identifier) | ||
|
|
||
| def create_json_data_object_from_spatialview(self, spatialview): |
There was a problem hiding this comment.
You might consider moving this out of the view and into the model.
| ] | ||
| return spatialview | ||
|
|
||
| def create_json_data_object_from_spatialview(self, spatialview): |
There was a problem hiding this comment.
Suggest importing this instead of reimplementing. Or, if you move this to the model, accessing it from the instance.
| spatialview = validate_json_data_content_result | ||
|
|
||
| try: | ||
| spatialview.clean() |
There was a problem hiding this comment.
I think we discussed this offline already, but prefer full_clean() to get everything including model field validation etc.
| def post(self, request): | ||
|
|
||
| try: | ||
| json_data = json.loads(request.body.decode("utf-8")) |
There was a problem hiding this comment.
Elsewhere we seem to be doing, but I don't know if there is a substantial difference offhand:
JSONDeserializer().deserialize(request.body)|
|
||
| return self.create_spatialview_from_json_data(json_data) | ||
|
|
||
| @method_decorator(group_required("Application Administrator")) |
There was a problem hiding this comment.
As is, without raise_exception=True, this will 302 to some place else. That makes sense for a GET request. But what kind of client are you envisioning calling this POST? Would you rather just return JSON with raise_exception=True?
| self.create_json_data_object_from_spatialview(spatialview), status=200 | ||
| ) | ||
| return JSONErrorResponse( | ||
| _("Spatialview update falied"), _("No json request payload"), status=400 |
| ), | ||
| status=400, | ||
| ) | ||
| return JSONResponse(status=200) |
There was a problem hiding this comment.
Suggest NO_CONTENT instead of OK for a delete without any content
Types of changes
Description of Change
Allows spatial views to have a language code, ensure the correct values are for node with datatypes that support internationalisation.
Adds an API endpoint for managing spatialview configuration
Issues Solved
Closes #10704 and #10705
Checklist
Accessibility Checklist
Developer Guide
Ticket Background
Further comments
Arches-docs PR (intl support): archesproject/arches-docs#435
Arches-docs PR (api definition): archesproject/arches-docs#439