Skip to content

Promote the YAML config on project import #5444

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Promote the YAML config on project import #5444

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c58c2e1
Promote the YAML config on project import
davidfischer Mar 13, 2019
d833514
Remove unnecessary template
davidfischer Mar 13, 2019
a4e9f64
Small updates to the sample config file
davidfischer Mar 13, 2019
736a6e7
Return a dict literal
davidfischer Mar 13, 2019
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
25 changes: 18 additions & 7 deletions docs/config-file/v2.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,26 @@ Here is an example of how this file looks like:

.. code:: yaml
Copy link
Member

Choose a reason for hiding this comment

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

Instead of the above line saying,

Here is an example of how this file looks like:

what about saying something like "Here is a minimum example configuration file:" or some better phrase than that one but making it clear that it's the minimum inviting the user to copy and paste as is other than keep reading the whole page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there's a few changes so that the YAML docs are better "at a glance". I think that should be a separate PR but I can take care of this small change here.


# .readthedocs.yml
# .readthedocs.yml
# Read the Docs configuration file
# See https://docs.readthedocs.io/en/stable/config-file/v2.html for details

version: 2
# Required
version: 2

# Build documentation in the docs/ directory with Sphinx
sphinx:
configuration: docs/conf.py

# Optionally build your docs in additional formats such as PDFs and ePubs
formats: all

# Optionally set the version of Python and requirements required to build your docs
python:
version: 3.7
install:
- requirements: docs/requirements.txt

python:
version: 3.7
install:
- method: pip
path: .

Supported settings
------------------
Expand Down
6 changes: 0 additions & 6 deletions readthedocs/projects/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,13 +95,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

Expand Down
17 changes: 1 addition & 16 deletions readthedocs/projects/views/private.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
ProjectAdvancedForm,
ProjectAdvertisingForm,
ProjectBasicsForm,
ProjectExtraForm,
ProjectRelationshipForm,
RedirectForm,
TranslationForm,
Expand Down Expand Up @@ -255,16 +254,11 @@ class ImportWizardView(ProjectSpamMixin, PrivateViewMixin, SessionWizardView):

form_list = [
('basics', ProjectBasicsForm),
('extra', ProjectExtraForm),
Copy link
Member

Choose a reason for hiding this comment

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

If we ending up with only one form in this list, we may remove the SessionWizardView and handle this as a normal view.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The corporate code actually relies on self.form_list. That's why I didn't completely refactor this view.

Copy link
Member

Choose a reason for hiding this comment

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

Why not refactor the corporate code for this as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm trying to make the scope of the change smaller rather than larger.

]
condition_dict = {'extra': lambda self: self.is_advanced()}

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
kwargs = {'user': self.request.user}
Copy link
Member

Choose a reason for hiding this comment

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

This ImportWizardView view is overridden in the corporate site, we probably need to make some changes there together with this PR and double check that this process still works there. Can you test it and apply the same changes there?

return kwargs

def get_template_names(self):
Expand All @@ -280,7 +274,6 @@ def done(self, form_list, **kwargs):
finish by added the members to the project and saving.
"""
form_data = self.get_all_cleaned_data()
extra_fields = ProjectExtraForm.Meta.fields
# expect the first form; manually wrap in a list in case it's a
# View Object, as it is in Python 3.
basics_form = list(form_list)[0]
Expand All @@ -290,9 +283,6 @@ def done(self, form_list, **kwargs):
tags = form_data.pop('tags', [])
for tag in tags:
project.tags.add(tag)
for field, value in list(form_data.items()):
if field in extra_fields:
setattr(project, field, value)
project.save()

# TODO: this signal could be removed, or used for sync task
Expand All @@ -316,11 +306,6 @@ def trigger_initial_build(self, project):
async_result = task_promise.apply_async()
return async_result

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 ImportDemoView(PrivateViewMixin, View):

Expand Down
109 changes: 0 additions & 109 deletions readthedocs/rtd_tests/tests/test_project_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,115 +145,6 @@ def test_form_missing(self):
self.assertWizardFailure(resp, 'repo_type')


class TestAdvancedForm(TestBasicsForm):

def setUp(self):
super().setUp()
self.step_data['basics']['advanced'] = True
self.step_data['extra'] = {
'description': 'Describe foobar',
'language': 'en',
'documentation_type': 'sphinx',
'tags': 'foo, bar, baz',
}

def test_form_pass(self):
"""Test all forms pass validation."""
resp = self.post_step('basics')
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/')

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, '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, users=[self.user])
self.step_data['basics']['remote_repository'] = remote_repo.pk
resp = self.post_step('basics')
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/')

proj = Project.objects.get(name='foobar')
self.assertIsNotNone(proj)
self.assertEqual(proj.remote_repository, remote_repo)

@patch(
'readthedocs.projects.views.private.ProjectExtraForm.clean_description',
create=True,
)
def test_form_spam(self, mocked_validator):
"""Don't add project on a spammy description."""
self.user.date_joined = timezone.now() - timedelta(days=365)
self.user.save()
mocked_validator.side_effect = ProjectSpamError

with self.assertRaises(Project.DoesNotExist):
proj = Project.objects.get(name='foobar')

resp = self.post_step('basics')
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'], '/')

with self.assertRaises(Project.DoesNotExist):
proj = Project.objects.get(name='foobar')
self.assertFalse(self.user.profile.banned)

@patch(
'readthedocs.projects.views.private.ProjectExtraForm.clean_description',
create=True,
)
def test_form_spam_ban_user(self, mocked_validator):
"""Don't add spam and ban new user."""
self.user.date_joined = timezone.now()
self.user.save()
mocked_validator.side_effect = ProjectSpamError

with self.assertRaises(Project.DoesNotExist):
proj = Project.objects.get(name='foobar')

resp = self.post_step('basics')
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'], '/')

with self.assertRaises(Project.DoesNotExist):
proj = Project.objects.get(name='foobar')
self.assertTrue(self.user.profile.banned)


class TestImportDemoView(MockBuildTestCase):
"""Test project import demo view."""

Expand Down
7 changes: 3 additions & 4 deletions readthedocs/templates/projects/import_basics.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,9 @@ <h3>{% trans "Project Details" %}</h3>

<p class="info">
{% blocktrans trimmed %}
To import a project,
start by entering a few details about your repository.
More advanced project options can be configured
if you select <b>Edit advanced project options</b>.
To import a project, start by entering a few details about your repository.
You can set additional configuration options for your documentation in a
<a href="https://docs.readthedocs.io/en/stable/config-file/v2.html">.readthedocs.yml</a> file.
{% endblocktrans %}
</p>

Expand Down
14 changes: 0 additions & 14 deletions readthedocs/templates/projects/import_extra.html
View file
Open in desktop

This file was deleted.