Skip to content
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

[Fixed #872] Filter Builds according to commit #3544

Merged
merged 6 commits into from Oct 31, 2018
Merged

[Fixed #872] Filter Builds according to commit #3544

merged 6 commits into from Oct 31, 2018

Conversation

@Alig1493
Copy link
Contributor

@Alig1493 Alig1493 commented Jan 23, 2018

Added filter fields to api/v2/build/ endpoint to filter according to the commit hashes (Using filter backends).

@Alig1493 Alig1493 changed the title Commit filter [Fixed #872 Filter Builds according to commit] Jan 24, 2018
@Alig1493 Alig1493 changed the title [Fixed #872 Filter Builds according to commit] [Fixed #872] Filter Builds according to commit Jan 24, 2018
@safwanrahman safwanrahman self-requested a review Jan 24, 2018
@safwanrahman
Copy link
Member

@safwanrahman safwanrahman commented Jan 24, 2018

That looks good @Alig1493. Can you plase add some test for this endpoint filtering?

Copy link
Member

@humitos humitos left a 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,

https://github.com/rtfd/readthedocs.org/blob/935aa8b37b1b241dff08b968a9bf7dd4ba8347fd/readthedocs/restapi/views/model_views.py#L258-L265

Should we go in that direction? What do you think?

@Alig1493
Copy link
Contributor Author

@Alig1493 Alig1493 commented Jan 25, 2018

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.

@safwanrahman
Copy link
Member

@safwanrahman safwanrahman commented Jan 25, 2018

I also dont know why django-filters were removed. I am using it quite daily basis in my DRF projects and it seems to be very helpful.
@agjohnson Can you please elaborate why we decided to not use django-filters anymore?

@@ -205,6 +205,13 @@ class BuildViewSetBase(UserSelectViewSet):
admin_serializer_class = BuildAdminSerializer
model = Build

def get_queryset(self):
Copy link
Member

@safwanrahman safwanrahman Feb 13, 2018

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 query

Copy link
Contributor Author

@Alig1493 Alig1493 Feb 14, 2018

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.

@safwanrahman
Copy link
Member

@safwanrahman safwanrahman commented Feb 15, 2018

@Alig1493 Changes looks good. Can you please add some test for this? Here you can add test

Copy link
Member

@safwanrahman safwanrahman left a comment

Looks good. r+ from my end

@safwanrahman
Copy link
Member

@safwanrahman safwanrahman commented Feb 18, 2018

@humitos Can you please take a look and have a review?

Copy link
Member

@humitos humitos left a comment

Realized that maybe we need to do more work on this regarding the filtering. I don't know what is the correct solution yet (maybe adding the project slug to the endpoint?)

I think we need more opinions here.

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)
Copy link
Member

@humitos humitos Feb 20, 2018

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)

Copy link
Member

@safwanrahman safwanrahman Feb 26, 2018

@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?

@safwanrahman
Copy link
Member

@safwanrahman safwanrahman commented Mar 11, 2018

@humitos What should we do at this moment? Do you have any idea in your mind?

@ericholscher ericholscher merged commit f56cbd1 into readthedocs:master Oct 31, 2018
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants