From 6624b987dc5662d74ba72ae2fe46a409df623d0c Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Sun, 6 Aug 2023 14:05:29 +0200 Subject: [PATCH 1/2] Import: remove extra/advanced step from project import wizard This commit removes the "Extra/Advanced" step and moves the "language" attribute to the "Basics" step -- the first one, since that field is important. Closes #10392 --- readthedocs/projects/forms.py | 8 +------- readthedocs/projects/views/mixins.py | 10 +--------- readthedocs/projects/views/private.py | 28 +++------------------------ 3 files changed, 5 insertions(+), 41 deletions(-) diff --git a/readthedocs/projects/forms.py b/readthedocs/projects/forms.py index 724f85210df..542b4eeca7f 100644 --- a/readthedocs/projects/forms.py +++ b/readthedocs/projects/forms.py @@ -86,7 +86,7 @@ class ProjectBasicsForm(ProjectForm): class Meta: model = Project - fields = ("name", "repo", "default_branch") + fields = ("name", "repo", "default_branch", "language") remote_repository = forms.IntegerField( widget=forms.HiddenInput(), @@ -94,13 +94,7 @@ class Meta: ) def __init__(self, *args, **kwargs): - show_advanced = kwargs.pop('show_advanced', False) super().__init__(*args, **kwargs) - if show_advanced: - self.fields['advanced'] = forms.BooleanField( - required=False, - label=_('Edit advanced project options'), - ) self.fields['repo'].widget.attrs['placeholder'] = self.placehold_repo() self.fields['repo'].widget.attrs['required'] = True diff --git a/readthedocs/projects/views/mixins.py b/readthedocs/projects/views/mixins.py index 396a564388a..80428799ce3 100644 --- a/readthedocs/projects/views/mixins.py +++ b/readthedocs/projects/views/mixins.py @@ -100,12 +100,11 @@ class ProjectImportMixin: """Helpers to import a Project.""" - def finish_import_project(self, request, project, tags=None): + def finish_import_project(self, request, project): """ Perform last steps to import a project into Read the Docs. - Add the user from request as maintainer - - Set all the tags to the project - Send Django Signal - Trigger initial build @@ -115,18 +114,11 @@ def finish_import_project(self, request, project, tags=None): :param project: Project instance just imported (already saved) :param tags: tags to add to the project """ - if not tags: - tags = [] - project.users.add(request.user) - for tag in tags: - project.tags.add(tag) - log.info( 'Project imported.', project_slug=project.slug, user_username=request.user.username, - tags=tags, ) # TODO: this signal could be removed, or used for sync task diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index c6ef3e0a8d8..a3c2abbc2b2 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -54,7 +54,6 @@ ProjectAdvertisingForm, ProjectBasicsForm, ProjectConfigForm, - ProjectExtraForm, ProjectRelationshipForm, RedirectForm, TranslationForm, @@ -97,7 +96,6 @@ class ProjectDashboard(PrivateViewMixin, ListView): model = Project template_name = 'projects/project_dashboard.html' - # pylint: disable=arguments-differ def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) # Set the default search to search files instead of projects @@ -134,7 +132,7 @@ def get_queryset(self): def get(self, request, *args, **kwargs): self.validate_primary_email(request.user) - return super(ProjectDashboard, self).get(self, request, *args, **kwargs) + return super().get(self, request, *args, **kwargs) class ProjectMixin(PrivateViewMixin): @@ -263,9 +261,7 @@ class ImportWizardView(ProjectImportMixin, PrivateViewMixin, SessionWizardView): form_list = [ ("basics", ProjectBasicsForm), ("config", ProjectConfigForm), - ("extra", ProjectExtraForm), ] - condition_dict = {'extra': lambda self: self.is_advanced()} initial_dict_key = 'initial-data' @@ -283,7 +279,7 @@ def _set_initial_dict(self): else: self.initial_dict = self.storage.data.get(self.initial_dict_key, {}) - def post(self, *args, **kwargs): # pylint: disable=arguments-differ + def post(self, *args, **kwargs): self._set_initial_dict() log.bind(user_username=self.request.user.username) @@ -299,8 +295,6 @@ def get_form_kwargs(self, step=None): """Get args to pass into form instantiation.""" kwargs = {} kwargs['user'] = self.request.user - if step == 'basics': - kwargs['show_advanced'] = True return kwargs def get_template_names(self): @@ -315,32 +309,17 @@ def done(self, form_list, **kwargs): other side effects for now, by signalling a save without commit. Then, finish by added the members to the project and saving. """ - form_data = self.get_all_cleaned_data() - extra_fields = ProjectExtraForm.Meta.fields basics_form = form_list[0] # Save the basics form to create the project instance, then alter # attributes directly from other forms project = basics_form.save() - # Remove tags to avoid setting them in raw instead of using ``.add`` - tags = form_data.pop('tags', []) - - for field, value in list(form_data.items()): - if field in extra_fields: - setattr(project, field, value) - project.save() - - self.finish_import_project(self.request, project, tags) + self.finish_import_project(self.request, project) return HttpResponseRedirect( reverse('projects_detail', args=[project.slug]), ) - def is_advanced(self): - """Determine if the user selected the `show advanced` field.""" - data = self.get_cleaned_data_for_step('basics') or {} - return data.get('advanced', True) - class ImportView(PrivateViewMixin, TemplateView): @@ -931,7 +910,6 @@ class IntegrationWebhookSync(IntegrationMixin, GenericView): """ def post(self, request, *args, **kwargs): - # pylint: disable=unused-argument if 'integration_pk' in kwargs: integration = self.get_integration() update_webhook(self.get_project(), integration, request=request) From fb1c5b5ffcec8901957d603a5a43fde7f80bac43 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 7 Aug 2023 11:58:13 +0200 Subject: [PATCH 2/2] Update tests --- .../rtd_tests/tests/test_project_forms.py | 41 +++++++------ .../rtd_tests/tests/test_project_views.py | 61 ++----------------- 2 files changed, 30 insertions(+), 72 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_project_forms.py b/readthedocs/rtd_tests/tests/test_project_forms.py index e0ecd895d8b..c44bd4398e0 100644 --- a/readthedocs/rtd_tests/tests/test_project_forms.py +++ b/readthedocs/rtd_tests/tests/test_project_forms.py @@ -74,9 +74,10 @@ def test_import_repo_url(self): with override_settings(ALLOW_PRIVATE_REPOS=False): for url, valid in public_urls: initial = { - 'name': 'foo', - 'repo_type': 'git', - 'repo': url, + "name": "foo", + "repo_type": "git", + "repo": url, + "language": "en", } form = ProjectBasicsForm(initial) self.assertEqual(form.is_valid(), valid, msg=url) @@ -84,18 +85,20 @@ def test_import_repo_url(self): with override_settings(ALLOW_PRIVATE_REPOS=True): for url, valid in private_urls: initial = { - 'name': 'foo', - 'repo_type': 'git', - 'repo': url, + "name": "foo", + "repo_type": "git", + "repo": url, + "language": "en", } form = ProjectBasicsForm(initial) self.assertEqual(form.is_valid(), valid, msg=url) def test_empty_slug(self): initial = { - 'name': "''", - 'repo_type': 'git', - 'repo': 'https://github.com/user/repository', + "name": "''", + "repo_type": "git", + "repo": "https://github.com/user/repository", + "language": "en", } form = ProjectBasicsForm(initial) self.assertFalse(form.is_valid()) @@ -112,9 +115,10 @@ def test_changing_vcs_should_not_change_latest_is_not_none(self): form = ProjectBasicsForm( { - 'repo': 'http://github.com/test/test', - 'name': 'name', - 'repo_type': REPO_TYPE_GIT, + "repo": "http://github.com/test/test", + "name": "name", + "repo_type": REPO_TYPE_GIT, + "language": "en", }, instance=project, ) @@ -144,11 +148,14 @@ def test_length_of_tags(self): self.assertDictEqual(form.errors, {'tags': [error_msg]}) def test_strip_repo_url(self): - form = ProjectBasicsForm({ - 'name': 'foo', - 'repo_type': 'git', - 'repo': 'https://github.com/rtfd/readthedocs.org/' - }) + form = ProjectBasicsForm( + { + "name": "foo", + "repo_type": "git", + "repo": "https://github.com/rtfd/readthedocs.org/", + "language": "en", + } + ) self.assertTrue(form.is_valid()) self.assertEqual( form.cleaned_data['repo'], diff --git a/readthedocs/rtd_tests/tests/test_project_views.py b/readthedocs/rtd_tests/tests/test_project_views.py index c0aa6b1ef19..f398bf94e9c 100644 --- a/readthedocs/rtd_tests/tests/test_project_views.py +++ b/readthedocs/rtd_tests/tests/test_project_views.py @@ -69,10 +69,11 @@ class TestBasicsForm(WizardTestCase): def setUp(self): self.user = get(User) - self.step_data['basics'] = { - 'name': 'foobar', - 'repo': 'http://example.com/foobar', - 'repo_type': 'git', + self.step_data["basics"] = { + "name": "foobar", + "repo": "http://example.com/foobar", + "repo_type": "git", + "language": "en", } self.step_data["config"] = { "confirm": True, @@ -199,10 +200,6 @@ def setUp(self): } def test_initial_params(self): - extra_initial = { - 'description': 'An amazing project', - 'project_url': "https://foo.bar", - } config_initial = { "confirm": True, } @@ -213,7 +210,7 @@ def test_initial_params(self): 'default_branch': 'main', 'remote_repository': '', } - initial = dict(**extra_initial, **config_initial, **basic_initial) + initial = dict(**config_initial, **basic_initial) self.client.force_login(self.user) # User selects a remote repo to import. @@ -223,61 +220,17 @@ def test_initial_params(self): form = resp.context_data['form'] self.assertEqual(form.initial, basic_initial) - # User selects advanced. - basic_initial['advanced'] = True - step_data = { - f'basics-{k}': v - for k, v in basic_initial.items() - } - step_data[f'{self.wizard_class_slug}-current_step'] = 'basics' - resp = self.client.post(self.url, step_data) - - step_data = {f"config-{k}": v for k, v in config_initial.items()} - step_data[f"{self.wizard_class_slug}-current_step"] = "config" - resp = self.client.post(self.url, step_data) - - # The correct initial data for the advanced form is set. - form = resp.context_data['form'] - self.assertEqual(form.initial, extra_initial) - def test_form_pass(self): """Test all forms pass validation.""" resp = self.post_step("basics") self.assertWizardResponse(resp, "config") resp = self.post_step("config", session=list(resp._request.session.items())) - self.assertWizardResponse(resp, "extra") - self.assertEqual(resp.status_code, 200) - resp = self.post_step('extra', session=list(resp._request.session.items())) self.assertIsInstance(resp, HttpResponseRedirect) self.assertEqual(resp.status_code, 302) self.assertEqual(resp['location'], '/projects/foobar/') proj = Project.objects.get(name='foobar') self.assertIsNotNone(proj) - data = self.step_data['basics'] - del data['advanced'] - del self.step_data['extra']['tags'] - self.assertCountEqual( - [tag.name for tag in proj.tags.all()], - ['bar', 'baz', 'foo'], - ) - data.update(self.step_data['extra']) - for (key, val) in list(data.items()): - self.assertEqual(getattr(proj, key), val) - - def test_form_missing_extra(self): - """Submit extra form with missing data, expect to get failures.""" - # Remove extra data to trigger validation errors - self.step_data['extra'] = {} - - resp = self.post_step("basics") - self.assertWizardResponse(resp, "config") - resp = self.post_step("config", session=list(resp._request.session.items())) - self.assertWizardResponse(resp, "extra") - resp = self.post_step("extra", session=list(resp._request.session.items())) - - self.assertWizardFailure(resp, 'language') - self.assertWizardFailure(resp, 'documentation_type') def test_remote_repository_is_added(self): remote_repo = get(RemoteRepository, default_branch="default-branch") @@ -292,8 +245,6 @@ def test_remote_repository_is_added(self): resp = self.post_step("basics") self.assertWizardResponse(resp, "config") resp = self.post_step("config", session=list(resp._request.session.items())) - self.assertWizardResponse(resp, "extra") - resp = self.post_step("extra", session=list(resp._request.session.items())) self.assertIsInstance(resp, HttpResponseRedirect) self.assertEqual(resp.status_code, 302) self.assertEqual(resp['location'], '/projects/foobar/')