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

API: fix subprojects creation when organizaions are enabled #8393

Merged
merged 4 commits into from
Sep 1, 2021

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Aug 4, 2021

There is a test for this on .com.
We were overriding this object in .com,
but we are also inheriting from this class,
our overrides don't work when they are inherited.

There is a test for this on .com.
We were overriding this object in .com,
but we are also inheriting from this class,
our overrides don't work when they are inherited.
@stsewd stsewd marked this pull request as draft August 4, 2021 20:05
@stsewd stsewd marked this pull request as ready for review August 4, 2021 22:35
@@ -594,6 +627,9 @@ def get_subproject_of(self, obj):
return None


# FIXME: this override isn't needed, but tests will fail if removed.
# We may have been relying on a weird behavior of using this class
# as a base class of another.
Copy link
Member Author

Choose a reason for hiding this comment

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

This wrong behavior is described in #8394

@@ -579,6 +595,23 @@ class Meta:
)
}

if settings.RTD_ALLOW_ORGANIZATIONS:
Copy link
Member Author

Choose a reason for hiding this comment

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

all these was migrated from .com

@stsewd stsewd added the Status: blocked Issue is blocked on another issue label Aug 4, 2021
@stsewd stsewd requested a review from a team August 4, 2021 22:55
@stsewd
Copy link
Member Author

stsewd commented Aug 4, 2021

This is ready for review, but it can't be merged yet, we need to fix another bug on subproject creation first (#8395)

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Reading the PR's description and the diff I'm not sure to understand how the users= field is related to a problem on subproject creation. Can you expand on that?

Also, this PR could include a test that checks for this behavior.

readthedocs/api/v3/serializers.py Show resolved Hide resolved
@stsewd
Copy link
Member Author

stsewd commented Aug 16, 2021

Also, this PR could include a test that checks for this behavior.

There is a test included in the PR from .com, we should move all organization tests here, but that's another PR.

I'm not sure to understand how the users= field is related to a problem on subproject creation.

When trying to use this serializer you'll get

The field 'users' was declared on serializer ProjectSerializerBase, but has not been included in the 'fields' option.

Basically, we only want to include it on .org, not .com.

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation.

LGTM! However, you have marked it as Blocked but not sure why.

@stsewd
Copy link
Member Author

stsewd commented Aug 18, 2021

We need to fix this #8395 first, otherwise we will be allowing to create cross-organization subprojects (and maybe from orgs where you aren't an owner, but a member, haven't tested this). Thanks to this bug creating subprojects wasn't possible at all on .com.

@stsewd stsewd merged commit 3949a63 into master Sep 1, 2021
@stsewd stsewd deleted the api-fix-subprojects branch September 1, 2021 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: blocked Issue is blocked on another issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants