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

Move out authorization from FooterHTML view #6133

Merged

Conversation

@stsewd
Copy link
Member

commented Sep 4, 2019

This allow us to override the authorization from the corporate site.

Everything that makes use of request.user now can be overridden :)

stsewd added 2 commits Sep 4, 2019
Move out authorization from FooterHTML view
This allow us to override the authorization from the corporate site.
@humitos
Copy link
Member

left a comment

I think changes make sense.

I'm concern about having more extra auth logic on a different Permission class, but I think there is no other way to do it. I did the same for APIv3. The good side of this is that it uses the normal querysets.

I left some comments to improve the naming of some methods/classes.

readthedocs/api/v2/permissions.py Outdated Show resolved Hide resolved
readthedocs/api/v2/views/footer_views.py Outdated Show resolved Hide resolved
stsewd added 2 commits Sep 4, 2019
@stsewd

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2019

@humitos done. I also modified the ordered_active_versions method to accept any kwargs (this is needed to pass more stuff down to .public in .com)

@humitos
humitos approved these changes Sep 4, 2019
@@ -950,28 +950,33 @@ def active_versions(self):
versions.filter(active=True, uploaded=True)
)

def ordered_active_versions(self, user=None):
def ordered_active_versions(self, **kwargs):

This comment has been minimized.

Copy link
@humitos

humitos Sep 4, 2019

Member

It would be good to have a docstring or comment saying that this kwargs argument has to match the filter for the Version, or probably better to just use all the keyword argument that support .public directly here.

Probably the second option is better "self-documented".

This comment has been minimized.

Copy link
@stsewd

stsewd Sep 4, 2019

Author Member

We can't do 2, in .com we override .public to support more kwargs

@stsewd stsewd merged commit 46abaa8 into readthedocs:master Sep 4, 2019

2 checks passed

continuous-documentation/read-the-docs Read the Docs build succeeded!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stsewd stsewd deleted the stsewd:separate-authorization-from-footer-view branch Sep 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.