Feature/writable api #20
Conversation
annashamray
force-pushed
the
feature/writable-api
branch
from
7a591ad to
50f3e8f
Compare
Codecov Report
@@ Coverage Diff @@
## master #20 +/- ##
==========================================
+ Coverage 89.87% 92.16% +2.28%
==========================================
Files 44 54 +10
Lines 889 1148 +259
==========================================
+ Hits 799 1058 +259
Misses 90 90
Continue to review full report at Codecov.
|
|
|
||
|
|
||
| class ObjectVersionAPITests(TokenAuthMixin, APITestCase): | ||
| def test_get_versions(self): |
There was a problem hiding this comment.
Can you add a test where there are 2 Objecttypes, with each 1 version. Then, check that if you retrieve /objecttypes/<uuid>/versions and the inline version URLs in /objecttypes/<uuid> only contain 1 version (and not also the version object from the other objecttype).
I had this issue quite often when working with nested viewsets.
There was a problem hiding this comment.
test_get_objecttypes_with_versions is added
src/objecttypes/api/views.py
Outdated
|
|
||
| @swagger_auto_schema(request_body=no_body) | ||
| @action(detail=True, methods=["post"]) | ||
| def publish(self, request, *args, **kwargs): |
There was a problem hiding this comment.
I understand that this is an action BUT we have 3 statusses: draft, published and deprecated. There is no status field that can be modified on a version object. So, how do we mark a version as deprecated?
I think we should just add the status field to a writable field so you can set published like that, and also set deprecated. We should only block changing these 2 back to draft.
There was a problem hiding this comment.
Actually I quite like publish endpoint, it makes API consistent with admin page, and also with our Catalogi API.
Do we really need deprecated status? We don't use it anywhere now. Maybe we can just get rid of it.
Or we can add DELETE action to the Version resource and switch status to depricated in this case?
There was a problem hiding this comment.
Actually I quite like publish endpoint, it makes API consistent with admin page
That's not really a good reason. The admin can just as well set the status field (in fact, it does that ofcourse, but via a publish function).
and also with our Catalogi API.
Hmm, that's a valid argument: Consistency.
Since we disagree, I'll ask a third party :)
Do we really need deprecated status? We don't use it anywhere now.
It was added because of DCAT/Metadata catalogus
Or we can add DELETE action to the Version resource and switch status to depricated in this case?
No, this is unexpected behaviour. Plus, if we ever introduce a new status it will be weird.
There was a problem hiding this comment.
I like the idea of a DELETE call:
- on draft resources -> actual DB delete
- on published resources -> mark deprecated. Mistakes can then be correct be publishing it again, if need be?
There was a problem hiding this comment.
I don't like that "draft" is set automatically, "publish" is set by an action and "deprecated" is set with a HTTP method.
- I'll make the decision to set the status with PUT/PATCH only. Setting non-draft to draft is not allowed.
- DELETE should be allowed on versions that are in draft.
- DELETE should also be allowed on objecttypes that have no versions
There was a problem hiding this comment.
Ok, I'll change the implementation
There was a problem hiding this comment.
the implementation is changed:
- DELETE is supported now
- "publish" action is removed
|
|
||
|
|
||
| class ObjectTypeSerializer(serializers.HyperlinkedModelSerializer): | ||
| versions = ObjectVersionSerializer(many=True) | ||
| versions = NestedHyperlinkedRelatedField( |
There was a problem hiding this comment.
I've just realized that this change breaks compatibility with Objects API validator. But I like that it's consistent with Zaken API now.
@joeribekker Should we display all versions data here? Or I can make a small change in Objects API
There was a problem hiding this comment.
I like this as well. I don't see the need to expand this property by default.
If you manually saw that this breaks Objects API validation, then please change yes, BUT it also means its a blind spot in our CI-setup. You can make an issue for that :)
There was a problem hiding this comment.
hmm, we don't have integration tests and we don't even have docker-compose with two APIs now. Perhaps we can set up a separate repo with integration tests?
There was a problem hiding this comment.
I like it, but lets make an issue for now so it won't be missed again.
There was a problem hiding this comment.
Fixes maykinmedia/objects-api#95