Build details page: normalize/trim command paths (second attempt) #9831
Conversation
Continuation of the work done in #9815 that didn't work as we expected. We weren't saving the commands properly when hitting the API since `serializers.SerializerMethodField` is a read-only field. This commit fixes this problem by using a different Serializer depending if it's a GET request or not, and if it's made by an Admin or not. It overrides the method `BuildViewSet.get_serializer_class` to manage these different cases and return the proper serializer. Done on top of #9827 to be sure the tests for saving commands are passing.
humitos
force-pushed
the
humitos/build-trim-get-commands
branch
from
7c8c4f3 to
a40a329
Compare
humitos
changed the title
There was a problem hiding this comment.
Should this also be implemented for the command API? https://readthedocs.org/api/v2/command/174704149/
Don't think we use that from our dashboard, but it will make the API consistent.
There was a problem hiding this comment.
This seems like it will fix the issue, along with @stsewd's feedback 👍
I wasn't sure about this since we are not using it anywhere AFAIK, APIv2 is already deprecated and users shouldn't be using it. The main idea was to make the dashboard UI better for now without going too deep. If we want to follow that road, we should do it for APIv3 as well (but it will change the behavior, so 🤷🏼 ) and review all the places where they are used. I think it may be fine to not implement it on the command API for now; but I'm open to other arguments. |
- rename serializer to mention "ReadOnly" on it's class name - add docstrings to these serializers
|
Thanks for the feedback! I think I've addressed all of it. Let me know. @stsewd I renamed the serializer class to mention "ReadOnly" on its name. @ericholscher I added docstring to the classes --hopefully, it's a little more clear now. |
Old builds don't have the builder hash in the docroot, since that was implmented recently.
Co-authored-by: Benjamin Balder Bach <[email protected]>
@humitos Much clearer what is happening. Thanks! 💯 |
* API V2: test that command is actually saved * Build details page: normalize/trim command paths (second attempt) (#9831) Continuation of the work done in #9815 that didn't work as we expected. We weren't saving the commands properly when hitting the API since `serializers.SerializerMethodField` is a read-only field. This commit fixes this problem by using a different Serializer depending if it's a GET request or not, and if it's made by an Admin or not. It overrides the method `BuildViewSet.get_serializer_class` to manage these different cases and return the proper serializer. Done on top of #9827 to be sure the tests for saving commands are passing. * Feedback from review addressed - rename serializer to mention "ReadOnly" on it's class name - add docstrings to these serializers * Typo when accessing the action * Optional builder hash on local instance Old builds don't have the builder hash in the docroot, since that was implmented recently. Co-authored-by: Manuel Kaufmann <[email protected]> Co-authored-by: Benjamin Balder Bach <[email protected]>
Continuation of the work done in
#9815 that didn't work as we
expected.
We weren't saving the commands properly when hitting the API since
serializers.SerializerMethodFieldis a read-only field. This commit fixes thisproblem by using a different Serializer depending if it's a GET request or not,
and if it's made by an Admin or not.
It overrides the method
BuildViewSet.get_serializer_classto manage thesedifferent cases and return the proper serializer.
Done on top of #9827 to be sure the
tests for saving commands are passing.
Closes #9833