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

Import: remove extra/advanced step from project import wizard #10603

Merged
merged 2 commits into from
Aug 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions readthedocs/projects/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,21 +86,15 @@ 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(),
required=False,
)

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

Expand Down
10 changes: 1 addition & 9 deletions readthedocs/projects/views/mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand Down
28 changes: 3 additions & 25 deletions readthedocs/projects/views/private.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@
ProjectAdvertisingForm,
ProjectBasicsForm,
ProjectConfigForm,
ProjectExtraForm,
ProjectRelationshipForm,
RedirectForm,
TranslationForm,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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'

Expand All @@ -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)
Expand All @@ -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):
Expand All @@ -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):

Expand Down Expand Up @@ -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)
Expand Down
41 changes: 24 additions & 17 deletions readthedocs/rtd_tests/tests/test_project_forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,28 +74,31 @@ 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)

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())
Expand All @@ -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,
)
Expand Down Expand Up @@ -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'],
Expand Down
61 changes: 6 additions & 55 deletions readthedocs/rtd_tests/tests/test_project_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
}
Expand All @@ -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.
Expand All @@ -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")
Expand All @@ -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/')
Expand Down