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

Changes required for APIv3 in corporate #6489

Merged
merged 27 commits into from
Nov 16, 2020
Merged

Conversation

humitos
Copy link
Member

@humitos humitos commented Dec 23, 2019

This PR makes changes required in .org to make it possible to use APIv3 in corporate by extending some permission classes.

There main changes are:

  1. Split PublicDetailPrivateListing permission class: move out the logic that checks for a user accessing /projects/ (without a project slug) to list all their projects
  2. Use a CommonPermissions class that can be overriden from corporate: we can't use the SettingsOverrideObject pattern with APIv3Settings because view classes inherit from it and so, all the classes will have the _default_class and will be overriden.
  3. Define all the views required for Organization (don't enable the URLs in community) and make them extendable from corporate
  4. Define all the serializers required for organizations models

I'm not super happy with 2) but I'm not sure what other pattern we can use to make it more extensible. With my proposal here, any change we need to do in APIv3Settings for corporate will need another override or a setting or similar.

There are no logic changes here and everything should keep working in the same way.

Move out the logic that authorize a user hitting /projects/ and create
a different class for this (it's used in corporate as well).

Leave the PublicDetailPrivateListing class only with the needed logic
for this goal: detail access is public and listing access is private.
@humitos humitos requested a review from a team December 23, 2019 16:23
@stale
Copy link

stale bot commented Feb 20, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Feb 20, 2020
@humitos humitos added PR: work in progress Pull request is not ready for full review and removed Status: stale Issue will be considered inactive soon labels Feb 21, 2020
@stale
Copy link

stale bot commented Apr 5, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Apr 5, 2020
@humitos
Copy link
Member Author

humitos commented Apr 6, 2020

This is blocked. I'm waiting for organization being migrated to .org.

@stale stale bot removed the Status: stale Issue will be considered inactive soon label Apr 6, 2020
@humitos humitos added the Status: blocked Issue is blocked on another issue label Apr 6, 2020
- /organizations/: list all the organizations for the logged in user
- /organizations/<organization-slug>/: details of organization
- /organizations/<organization-slug/projects/: projects for the organization

Note that these URLs are not enabled yet. They are registered in corporate for
now until we enable organizations in community.
@humitos humitos removed PR: work in progress Pull request is not ready for full review Status: blocked Issue is blocked on another issue labels Jul 15, 2020
@humitos
Copy link
Member Author

humitos commented Jul 15, 2020

I think this could be reviewed soon. I don't expect to too much big changes.

By using `or` instead of `any` we return True immediately if the first condition
is True without calling `PublicDetailPrivateListing`. This avoid generating 404
exception because there is not project found on /projects/ URL
@stale
Copy link

stale bot commented Sep 5, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Sep 5, 2020
@humitos humitos added Accepted Accepted issue on our roadmap and removed Status: stale Issue will be considered inactive soon labels Sep 7, 2020
readthedocs/api/v3/mixins.py Outdated Show resolved Hide resolved
readthedocs/api/v3/serializers.py Show resolved Hide resolved
Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

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

LGTM

readthedocs/api/v3/permissions.py Outdated Show resolved Hide resolved
@ericholscher ericholscher merged commit 892db33 into master Nov 16, 2020
@ericholscher ericholscher deleted the humitos/apiv3-corporate branch November 16, 2020 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants