From 5be42cec124fe0bf311a9413c4ef6c4c24d07aaa Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 23 Jul 2024 19:19:36 -0500 Subject: [PATCH 1/6] Project: allow connecting a project to a remote repo after it has been created Close https://github.com/readthedocs/readthedocs.org/issues/9437 --- readthedocs/projects/forms.py | 108 +++++++++++--------------- readthedocs/projects/models.py | 1 + readthedocs/projects/views/private.py | 4 + 3 files changed, 49 insertions(+), 64 deletions(-) diff --git a/readthedocs/projects/forms.py b/readthedocs/projects/forms.py index 1df6cb54c7a..2a3aedb1b1d 100644 --- a/readthedocs/projects/forms.py +++ b/readthedocs/projects/forms.py @@ -52,6 +52,13 @@ def __init__(self, *args, **kwargs): self.user = kwargs.pop("user", None) super().__init__(*args, **kwargs) + self.fields["repo"].widget.attrs["placeholder"] = self.placehold_repo() + self.fields["repo"].widget.attrs["required"] = True + + self.fields["remote_repository"].queryset = RemoteRepository.objects.filter( + users=self.user, + ) + def save(self, commit=True): project = super().save(commit) if commit: @@ -59,6 +66,39 @@ def save(self, commit=True): project.users.add(self.user) return project + def clean_name(self): + name = self.cleaned_data.get("name", "") + if not self.instance.pk: + potential_slug = slugify(name) + if Project.objects.filter(slug=potential_slug).exists(): + raise forms.ValidationError( + _("Invalid project name, a project already exists with that name"), + ) # yapf: disable # noqa + if not potential_slug: + # Check the generated slug won't be empty + raise forms.ValidationError( + _("Invalid project name"), + ) + + return name + + def clean_repo(self): + repo = self.cleaned_data.get("repo", "") + return repo.rstrip("/") + + def placehold_repo(self): + return choice( + [ + "https://bitbucket.org/cherrypy/cherrypy", + "https://bitbucket.org/birkenfeld/sphinx", + "https://bitbucket.org/hpk42/tox", + "https://github.com/zzzeek/sqlalchemy.git", + "https://github.com/django/django.git", + "https://github.com/fabric/fabric.git", + "https://github.com/ericholscher/django-kong.git", + ] + ) + class ProjectTriggerBuildMixin: @@ -313,74 +353,13 @@ class ProjectBasicsForm(ProjectForm): class Meta: model = Project - fields = ("name", "repo", "default_branch", "language") - - remote_repository = forms.IntegerField( - widget=forms.HiddenInput(), - required=False, - ) + fields = ("name", "repo", "default_branch", "language", "remote_repository") def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.fields["repo"].widget.attrs["placeholder"] = self.placehold_repo() self.fields["repo"].widget.attrs["required"] = True - - def save(self, commit=True): - """Add remote repository relationship to the project instance.""" - instance = super().save(commit) - remote_repo = self.cleaned_data.get("remote_repository", None) - if remote_repo: - if commit: - remote_repo.projects.add(self.instance) - remote_repo.save() - else: - instance.remote_repository = remote_repo - return instance - - def clean_name(self): - name = self.cleaned_data.get("name", "") - if not self.instance.pk: - potential_slug = slugify(name) - if Project.objects.filter(slug=potential_slug).exists(): - raise forms.ValidationError( - _("Invalid project name, a project already exists with that name"), - ) # yapf: disable # noqa - if not potential_slug: - # Check the generated slug won't be empty - raise forms.ValidationError( - _("Invalid project name"), - ) - - return name - - def clean_repo(self): - repo = self.cleaned_data.get("repo", "") - return repo.rstrip("/") - - def clean_remote_repository(self): - remote_repo = self.cleaned_data.get("remote_repository", None) - if not remote_repo: - return None - try: - return RemoteRepository.objects.get( - pk=remote_repo, - users=self.user, - ) - except RemoteRepository.DoesNotExist as exc: - raise forms.ValidationError(_("Repository invalid")) from exc - - def placehold_repo(self): - return choice( - [ - "https://bitbucket.org/cherrypy/cherrypy", - "https://bitbucket.org/birkenfeld/sphinx", - "https://bitbucket.org/hpk42/tox", - "https://github.com/zzzeek/sqlalchemy.git", - "https://github.com/django/django.git", - "https://github.com/fabric/fabric.git", - "https://github.com/ericholscher/django-kong.git", - ] - ) + self.fields["remote_repository"].widget = forms.HiddenInput() class ProjectConfigForm(forms.Form): @@ -395,7 +374,7 @@ def __init__(self, *args, **kwargs): class UpdateProjectForm( ProjectTriggerBuildMixin, - ProjectBasicsForm, + ProjectForm, ProjectPRBuildsMixin, ): @@ -413,6 +392,7 @@ class Meta: "versioning_scheme", "default_branch", "readthedocs_yaml_path", + "remote_repository", # Meta data "programming_language", "project_url", diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 52feefd2d45..6fd0852c246 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -540,6 +540,7 @@ class Project(models.Model): remote_repository = models.ForeignKey( "oauth.RemoteRepository", + help_text=_("Remote repository linked to this project"), on_delete=models.SET_NULL, related_name="projects", null=True, diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index d72719c92b0..ded5d25477d 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -200,6 +200,10 @@ class ProjectUpdate(ProjectMixin, UpdateView): def get_success_url(self): return reverse("projects_detail", args=[self.object.slug]) + def get_form(self, data=None, files=None, **kwargs): + kwargs["user"] = self.request.user + return super().get_form(data, files, **kwargs) + class ProjectDelete(UpdateChangeReasonPostView, ProjectMixin, DeleteViewWithMessage): success_message = _("Project deleted") From 7ea22d81d39ec77b9de3cae25d941048921e9f19 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 24 Jul 2024 17:00:40 -0500 Subject: [PATCH 2/6] Add migration --- ...126_alter_remote_repository_description.py | 39 +++++++++++++++++++ readthedocs/projects/models.py | 2 +- 2 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 readthedocs/projects/migrations/0126_alter_remote_repository_description.py diff --git a/readthedocs/projects/migrations/0126_alter_remote_repository_description.py b/readthedocs/projects/migrations/0126_alter_remote_repository_description.py new file mode 100644 index 00000000000..a2e92289aa6 --- /dev/null +++ b/readthedocs/projects/migrations/0126_alter_remote_repository_description.py @@ -0,0 +1,39 @@ +# Generated by Django 4.2.13 on 2024-07-24 21:57 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("oauth", "0016_deprecate_old_vcs"), + ("projects", "0125_update_naming"), + ] + + operations = [ + migrations.AlterField( + model_name="historicalproject", + name="remote_repository", + field=models.ForeignKey( + blank=True, + db_constraint=False, + help_text="Repository linked to this project", + null=True, + on_delete=django.db.models.deletion.DO_NOTHING, + related_name="+", + to="oauth.remoterepository", + ), + ), + migrations.AlterField( + model_name="project", + name="remote_repository", + field=models.ForeignKey( + blank=True, + help_text="Repository linked to this project", + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="projects", + to="oauth.remoterepository", + ), + ), + ] diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 6fd0852c246..d399a1448a0 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -540,7 +540,7 @@ class Project(models.Model): remote_repository = models.ForeignKey( "oauth.RemoteRepository", - help_text=_("Remote repository linked to this project"), + help_text=_("Repository linked to this project"), on_delete=models.SET_NULL, related_name="projects", null=True, From 478acb282fbb2c5bb0694a890b82f5c72052e982 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 24 Jul 2024 17:06:47 -0500 Subject: [PATCH 3/6] Mark migration as safe --- .../migrations/0126_alter_remote_repository_description.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/readthedocs/projects/migrations/0126_alter_remote_repository_description.py b/readthedocs/projects/migrations/0126_alter_remote_repository_description.py index a2e92289aa6..6d56c761603 100644 --- a/readthedocs/projects/migrations/0126_alter_remote_repository_description.py +++ b/readthedocs/projects/migrations/0126_alter_remote_repository_description.py @@ -2,9 +2,11 @@ import django.db.models.deletion from django.db import migrations, models +from django_safemigrate import Safe class Migration(migrations.Migration): + safe = Safe.before_deploy dependencies = [ ("oauth", "0016_deprecate_old_vcs"), ("projects", "0125_update_naming"), From 4c1277c0fe341d63f7d89bd3b38afba5e76c32ca Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 31 Jul 2024 16:16:03 -0500 Subject: [PATCH 4/6] Tests --- readthedocs/projects/forms.py | 14 ++++- .../rtd_tests/tests/test_project_forms.py | 53 ++++++++++++++++++- 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/readthedocs/projects/forms.py b/readthedocs/projects/forms.py index 25aa1d03e20..0b764ac25b8 100644 --- a/readthedocs/projects/forms.py +++ b/readthedocs/projects/forms.py @@ -55,7 +55,17 @@ def __init__(self, *args, **kwargs): self.fields["repo"].widget.attrs["placeholder"] = self.placehold_repo() self.fields["repo"].widget.attrs["required"] = True - self.fields["remote_repository"].queryset = RemoteRepository.objects.for_project_linking(self.user) + queryset = RemoteRepository.objects.for_project_linking(self.user) + current_remote_repo = ( + self.instance.remote_repository if self.instance.pk else None + ) + # If there is a remote repo attached to the project, add it to the queryset, + # since the current user might not have access to it. + if current_remote_repo: + queryset |= RemoteRepository.objects.filter( + pk=current_remote_repo.pk + ).distinct() + self.fields["remote_repository"].queryset = queryset def save(self, commit=True): project = super().save(commit) @@ -384,13 +394,13 @@ class Meta: # Basics and repo settings "name", "repo", + "remote_repository", "language", "default_version", "privacy_level", "versioning_scheme", "default_branch", "readthedocs_yaml_path", - "remote_repository", # Meta data "programming_language", "project_url", diff --git a/readthedocs/rtd_tests/tests/test_project_forms.py b/readthedocs/rtd_tests/tests/test_project_forms.py index dddc6e6bb56..c424be0ab8a 100644 --- a/readthedocs/rtd_tests/tests/test_project_forms.py +++ b/readthedocs/rtd_tests/tests/test_project_forms.py @@ -11,6 +11,7 @@ from readthedocs.builds.constants import EXTERNAL, LATEST, STABLE from readthedocs.builds.models import Version from readthedocs.core.forms import RichValidationError +from readthedocs.oauth.models import RemoteRepository, RemoteRepositoryRelation from readthedocs.organizations.models import Organization, Team from readthedocs.projects.constants import ( ADDONS_FLYOUT_SORTING_CALVER, @@ -156,7 +157,8 @@ def test_strip_repo_url(self): class TestProjectAdvancedForm(TestCase): def setUp(self): - self.project = get(Project, privacy_level=PUBLIC) + self.user = get(User) + self.project = get(Project, privacy_level=PUBLIC, users=[self.user]) get( Version, project=self.project, @@ -333,6 +335,55 @@ def test_trigger_build_on_save(self, trigger_build): version=default_branch, ) + def test_set_remote_repository(self): + data = { + "name": "Project", + "repo": "https://github.com/readthedocs/readthedocs.org/", + "repo_type": self.project.repo_type, + "default_version": LATEST, + "language": self.project.language, + "versioning_scheme": self.project.versioning_scheme, + } + + remote_repository = get( + RemoteRepository, + full_name="rtfd/template", + clone_url="https://github.com/rtfd/template", + html_url="https://github.com/rtfd/template", + ssh_url="git@github.com:rtfd/template.git", + private=False, + ) + + # No remote repository attached. + form = UpdateProjectForm(data, instance=self.project, user=self.user) + self.assertTrue(form.is_valid()) + + # Remote repository attached, but it doesn't belong to the user. + data["remote_repository"] = remote_repository.pk + form = UpdateProjectForm(data, instance=self.project, user=self.user) + self.assertFalse(form.is_valid()) + self.assertIn("remote_repository", form.errors) + + # Remote repository attached, it belongs to the user now. + remote_repository_rel = get( + RemoteRepositoryRelation, + remote_repository=remote_repository, + user=self.user, + admin=True, + ) + data["remote_repository"] = remote_repository.pk + form = UpdateProjectForm(data, instance=self.project, user=self.user) + self.assertTrue(form.is_valid()) + + # The project has the remote repository attached. + # And the user doesn't have access to it anymore, but still can use it. + self.project.remote_repository = remote_repository + self.project.save() + remote_repository_rel.delete() + data["remote_repository"] = remote_repository.pk + form = UpdateProjectForm(data, instance=self.project, user=self.user) + self.assertTrue(form.is_valid()) + class TestProjectAdvancedFormDefaultBranch(TestCase): def setUp(self): From cbeb7d8f24b830c6c9106b47a62e28afbdf510f8 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 31 Jul 2024 16:28:07 -0500 Subject: [PATCH 5/6] Fix tests --- readthedocs/oauth/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/oauth/models.py b/readthedocs/oauth/models.py index e81f34df1f2..4810943d046 100644 --- a/readthedocs/oauth/models.py +++ b/readthedocs/oauth/models.py @@ -186,7 +186,7 @@ class Meta: db_table = "oauth_remoterepository_2020" def __str__(self): - return self.html_url + return self.html_url or self.full_name def matches(self, user): """Existing projects connected to this RemoteRepository.""" From 47ce483d50406e03645b62b175804c1743dc0ca6 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 1 Aug 2024 16:46:51 -0500 Subject: [PATCH 6/6] Updates from review --- readthedocs/projects/forms.py | 1 + .../migrations/0126_alter_remote_repository_description.py | 6 ++++-- readthedocs/projects/models.py | 3 ++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/readthedocs/projects/forms.py b/readthedocs/projects/forms.py index 0b764ac25b8..e0f4031d454 100644 --- a/readthedocs/projects/forms.py +++ b/readthedocs/projects/forms.py @@ -66,6 +66,7 @@ def __init__(self, *args, **kwargs): pk=current_remote_repo.pk ).distinct() self.fields["remote_repository"].queryset = queryset + self.fields["remote_repository"].empty_label = _("No connected repository") def save(self, commit=True): project = super().save(commit) diff --git a/readthedocs/projects/migrations/0126_alter_remote_repository_description.py b/readthedocs/projects/migrations/0126_alter_remote_repository_description.py index 6d56c761603..482b810b2f2 100644 --- a/readthedocs/projects/migrations/0126_alter_remote_repository_description.py +++ b/readthedocs/projects/migrations/0126_alter_remote_repository_description.py @@ -19,7 +19,8 @@ class Migration(migrations.Migration): field=models.ForeignKey( blank=True, db_constraint=False, - help_text="Repository linked to this project", + verbose_name="Connected repository", + help_text="Repository connected to this project", null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name="+", @@ -31,7 +32,8 @@ class Migration(migrations.Migration): name="remote_repository", field=models.ForeignKey( blank=True, - help_text="Repository linked to this project", + verbose_name="Connected repository", + help_text="Repository connected to this project", null=True, on_delete=django.db.models.deletion.SET_NULL, related_name="projects", diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index d399a1448a0..b3f9a9b9aba 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -540,7 +540,8 @@ class Project(models.Model): remote_repository = models.ForeignKey( "oauth.RemoteRepository", - help_text=_("Repository linked to this project"), + verbose_name=_("Connected repository"), + help_text=_("Repository connected to this project"), on_delete=models.SET_NULL, related_name="projects", null=True,