[Fixed #872] Filter Builds according to commit #3544
Conversation
Alig1493
changed the title
Alig1493
changed the title
|
That looks good @Alig1493. Can you plase add some test for this endpoint filtering? |
There was a problem hiding this comment.
Thank for your contribution!
I don't really know why, but sometime ago RTD removed django-filter because they stopped using it.
Ref:
It seems that RTD is using just get_queryset methods to do simple filtering now. For example,
Should we go in that direction? What do you think?
|
Alright, since most of the readthedocs code is done that way I think it's also better for me to follow that convention and avoid the django-filters dependency. I will do the necessary corrections. |
|
I also dont know why |
| @@ -205,6 +205,13 @@ class BuildViewSetBase(UserSelectViewSet): | |||
| admin_serializer_class = BuildAdminSerializer | |||
| model = Build | |||
|
|
|||
| def get_queryset(self): | |||
There was a problem hiding this comment.
Can you call super instead of making the queryset?
Something like
queryset = super(BuildViewSetBase, self).get_queryset()
commit = self.request.query_params.get('commit', None)
if commit is not None:
query = query.filter(commit=commit)
return queryThere was a problem hiding this comment.
I understand and I have looked into it as to why you asked me to do so as well. Your assistance was very well appreciated and I have made the requested changes. You can review it if you wish @safwanrahman.
|
@Alig1493 Changes looks good. Can you please add some test for this? Here you can add test |
humitos
added
PR: work in progress
safwanrahman
added
PR: ready for review
and removed
PR: work in progress
|
@humitos Can you please take a look and have a review? |
| resp = client.get('/api/v2/build/', {'commit': 'test'}, format='json') | ||
| self.assertEqual(resp.status_code, 200) | ||
| build = resp.data | ||
| self.assertEqual(len(build['results']), 1) |
There was a problem hiding this comment.
I didn't realize this the other day but, is it OK to return a list of one element when we already know that it's going to be always one element?
I suppose it shouldn't be possible to have two different commits with the same hash, right? Although, what would happen for projects that are not git-based? SVN for example which are just plain numbers?
In that case, this endpoint will break (not because the code itself but in meaning, I think)
There was a problem hiding this comment.
@humitos As per (build model)[https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/builds/models.py#L430], the commit field is not unique. So theoritically there can be more than one build for same commit. Also, there can be multiple build for same commit like commit get deleted and forced pushed. So I think returning list of builds will be a better idea.
Regarding SVN, I actually do not know whats save in the commit field. Can you please elaborate?
|
@humitos What should we do at this moment? Do you have any idea in your mind? |
Added filter fields to api/v2/build/ endpoint to filter according to the commit hashes (Using filter backends).