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

Create subproject relationship via APIv3 endpoint #6176

Conversation

humitos
Copy link
Member

@humitos humitos commented Sep 12, 2019

The main goal of this PR is to add the ability to create a subproject relationship between two Project objects.

This PR is based on #5952 (https://github.com/readthedocs/readthedocs.org/tree/humitos/apiv3/project-update-endpoint) because it needs the "per object permissions check" changes that PR contains.

There are some things to note about this PR:

  • /api/v3/projects/pip/subprojects/ has changed its response. It was returning a list of Project but we found it was not 100% accurate and it was better to return a list of ProjectRelationship objects that includes the alias and the child (Project) rendered itself.
  • a subproject relantionship can be create only if the user is maintainer of both projects (parent and child)
  • a subproject relationship can be deleted only if the user is maintainer of the parent project

As a side note, while working on the tests for this PR, I found a but in the RelatedProjectQuerySetBase (see commit 0d42c1b).

Closes #6157

@humitos humitos requested review from stsewd and a team September 12, 2019 21:05
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.

I think you are missing some more checks:

  • Adding the project as subproject of itself
  • Nesting subprojects: adding as subproject a project that already has subprojects.
  • Unique alias across subprojects of project

def clean_parent(self):
if self.project.superprojects.exists():
# This validation error is mostly for testing, users shouldn't see
# this in normal circumstances
raise forms.ValidationError(
_('Subproject nesting is not supported'),
)
return self.project
def clean_child(self):
child = self.cleaned_data['child']
if child == self.project:
raise forms.ValidationError(
_('A project can not be a subproject of itself'),
)
return child
def clean_alias(self):
alias = self.cleaned_data['alias']
subproject = self.project.subprojects.filter(
alias=alias).exclude(id=self.instance.pk)
if subproject.exists():
raise forms.ValidationError(
_('A subproject with this alias already exists'),
)
return alias

I think 1 and 3 should be done at the db level.

Also, do we allow to add translations? There are some similar checks to be done too.

@ghost
Copy link

ghost commented Sep 19, 2019

Would it also make sense to prevent project deletion if a project has one or more subprojects? I'm not sure if I think having to delete each subproject first would be terribly cumbersome or not, but it may be a good safeguard to have.

@stsewd
Copy link
Member

stsewd commented Sep 19, 2019

@dwt2 currently if a project (main project) has submodules, and it is deleted, all its subprojects become normal projects.

@ghost
Copy link

ghost commented Sep 19, 2019

@dwt2 currently if a project (main project) has submodules, and it is deleted, all its subprojects become normal projects.

Hmm, I think I would be easier on users to have the current default as an option, but also the option to pass a param to delete all subprojects with the top-level project too.

Let's say I intentionally want to delete a project and its 100 subprojects. With the current default I then have to do 100 more API operations to delete each (new) top-level project. Doable, but tedious.

@humitos humitos added the PR: work in progress Pull request is not ready for full review label Sep 30, 2019
- Adding the project as subproject of itself
- Nesting subprojects is not supported
- Unique alias across subprojects of project
@humitos
Copy link
Member Author

humitos commented Oct 1, 2019

I think you are missing some more checks

I added these checks and tests for them as well. Please, review the PR when you have some time.

Also, do we allow to add translations? There are some similar checks to be done too.

Currently, no. We are only listing translations under /projects/<slug>/translations/

@humitos humitos removed the PR: work in progress Pull request is not ready for full review label Oct 1, 2019
@humitos humitos requested review from stsewd and a team October 1, 2019 10:45
@humitos humitos changed the base branch from humitos/apiv3/project-update-endpoint to master October 1, 2019 11:17
@humitos humitos changed the base branch from master to humitos/apiv3/project-update-endpoint October 1, 2019 11:18
@stsewd
Copy link
Member

stsewd commented Oct 1, 2019

@humitos can you resolve the conflicts before review?

…adthedocs/readthedocs.org into humitos/subproject-api-projectrelationship
@humitos
Copy link
Member Author

humitos commented Oct 2, 2019

@stsewd done!

readthedocs/api/v3/serializers.py Outdated Show resolved Hide resolved
readthedocs/projects/querysets.py Outdated Show resolved Hide resolved
docs/api/v3.rst Outdated Show resolved Hide resolved
For GET it returns a serializer with many fields and on POST,
it return a serializer to validate just a few fields.
"""
if self.action == 'create':
Copy link
Member

Choose a reason for hiding this comment

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

aren't these constants somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've never seen them used as constants. I did a quick grep on their source code and I found that they use just the strings as well :'( : https://github.com/encode/django-rest-framework/blob/e57c1505fc4ed957332ae547a35c8713acfdf30c/rest_framework/schemas/coreapi.py#L37

@humitos humitos merged commit 193fec6 into humitos/apiv3/project-update-endpoint Oct 8, 2019
@humitos humitos deleted the humitos/subproject-api-projectrelationship branch October 8, 2019 08:15
@stsewd
Copy link
Member

stsewd commented Oct 8, 2019

hmm, why wasn't this merged to master but apiv3/project-update-endpoint?

@stsewd stsewd mentioned this pull request Oct 8, 2019
humitos added a commit that referenced this pull request Oct 9, 2019
Merge #6176 to master 

Co-authored-by: Manuel Kaufmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants