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

Update tastypie #4325

Merged
merged 5 commits into from Oct 23, 2018
Merged

Update tastypie #4325

merged 5 commits into from Oct 23, 2018

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Jul 3, 2018

Ref to #4319 (comment) and #3570 (comment)

Closes #3570

@@ -12,7 +12,7 @@
from django.core.cache import cache
from django.shortcuts import get_object_or_404
from tastypie import fields
from tastypie.authorization import DjangoAuthorization
from tastypie.authorization import DjangoAuthorization, ReadOnlyAuthorization
Copy link
Member Author

@stsewd stsewd Jul 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other resources using DjangoAuthorization, but tests were failing only for the ProjectResource

Loading

Copy link
Member Author

@stsewd stsewd Jul 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't have tests for the other resources, that's why

Loading

@@ -39,7 +39,7 @@ class Meta(object):
allowed_methods = ['get', 'post', 'put']
queryset = Project.objects.api()
authentication = PostAuthentication()
authorization = DjangoAuthorization()
authorization = ReadOnlyAuthorization()
Copy link
Member Author

@stsewd stsewd Jul 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Authorization is already default to ReadOnlyAuthorization, I'm just being explicit here.

Loading

def test_make_project(self):
"""Test that a superuser can use the API."""
def test_cant_make_project(self):
"""Test that a can't use the API to create projects."""
Copy link
Member Author

@stsewd stsewd Jul 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We aren't using this in our code, I'm not sure if we allow this in the external API, we don't have that documented either https://docs.readthedocs.io/en/latest/api/v1.html

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jul 3, 2018

Ok, this is weird, tests are passing for 2d59d29 on travis, but locally (I just checked again!) are falling. Anyone else can check that? (yes, I'm running my tests with tox -r)

Loading

@agjohnson agjohnson added this to the Cleanup milestone Jul 9, 2018
@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Jul 9, 2018

This looks tied to a version 3.0 release, along with django 1.10. Targeting our 3.0 release for this, I think we can de-prioritize this work until we've freed up some more resources.

Loading

@agjohnson agjohnson removed this from the Cleanup milestone Jul 9, 2018
@agjohnson agjohnson added this to the 3.0 milestone Jul 9, 2018
@stsewd stsewd force-pushed the update-tastypie branch from b769b3a to bef0670 Aug 20, 2018
@stsewd stsewd force-pushed the update-tastypie branch 2 times, most recently from b344168 to 400859f Sep 13, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 10, 2018

Last time I check #4325 (comment) tests were passing locally btw

Loading

Copy link
Contributor

@davidfischer davidfischer left a comment

This looks good to me and I think we can update this independently of the Django upgrade. We never documented the API for creating projects so I'm +1 on just removing it.

Loading

@@ -15,7 +15,7 @@ six==1.11.0
future==0.16.0

# django-tastypie 0.13.x and 0.14.0 are not compatible with our code
Copy link
Contributor

@davidfischer davidfischer Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's update this comment. From the tastypie changelog, it looks like 0.13.3 is still compatible with Django 1.9.

Loading

Copy link
Member Author

@stsewd stsewd Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, 0.13.3 looks like just one bugfix
screenshot_2018-10-10 django-tastypie django-tastypie

Loading

@stsewd stsewd force-pushed the update-tastypie branch from bfeb5e7 to 58b452b Oct 10, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 10, 2018

I have updated tastypie to 0.13.3 and rebased, tests should pass (hope p:)

Loading

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Oct 11, 2018

I ran a manual test against the endpoints in the v1 API docs and they looked ok to me. The only exception was the File Search API which doesn't actually appear to be implemented correctly. This wasn't an issue with this PR though. It isn't working in production or with the old tastypie.

Loading

Copy link
Member

@humitos humitos left a comment

Looks good!

Loading

@humitos
Copy link
Member

@humitos humitos commented Oct 18, 2018

@stsewd @davidfischer Is there anything missing or blocking this PR to be merged? It has the label "Work in progress" but I think it's completed and ready to merge. Also, the milestone is set to 3.0. Not sure why...

We can upgrade to 0.14.0 once we upgrade Django to 1.11 in another PR.

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 18, 2018

@humitos this PR is already included in #4750

Loading

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Oct 18, 2018

This is included in #4750 but I believe it could be merged anytime. The Django 1.11 upgrade relies on this but this can be merged and used with our current setup.

Loading

@ericholscher ericholscher merged commit 58b452b into readthedocs:master Oct 23, 2018
1 check passed
Loading
@stsewd stsewd deleted the update-tastypie branch Oct 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants