Skip to content

Allow modifying version slugs #4505

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

Closed
wants to merge 4 commits into from
Closed
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
32 changes: 30 additions & 2 deletions readthedocs/builds/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@
from __future__ import absolute_import
from builtins import object
from django import forms
from django.utils.translation import ugettext_lazy as _, ugettext

from readthedocs.builds.models import VersionAlias, Version
from .constants import STABLE, LATEST
from .models import VersionAlias, Version
from .version_slug import VERSION_SLUG_REGEX
from readthedocs.projects.models import Project
from readthedocs.core.utils import trigger_build

Expand All @@ -29,9 +32,34 @@ def __init__(self, instance=None, *args, **kwargs): # noqa

class VersionForm(forms.ModelForm):

slug = forms.RegexField(
'^{pattern}$'.format(pattern=VERSION_SLUG_REGEX),
max_length=255,
Copy link
Member

Choose a reason for hiding this comment

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

I believe for DNS this can only contain 63 characters. We should probably enforce this on the model though: https://stackoverflow.com/questions/10552665/names-and-maximum-lengths-of-the-parts-of-a-url

Copy link
Member

Choose a reason for hiding this comment

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

There is some discussion around that here #3464

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#3464 is about project slugs, not version slugs. Correct?

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 regexfield is built to match the model (https://github.com/rtfd/readthedocs.org/blob/8282885/readthedocs/builds/models.py#L78). I'm happy to change it if you think that's appropriate but the two should match in my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, while I think a 255 character version slug is long, we don't use the version for anything related to DNS correct? We use project slugs for that but not versions, correct?

Copy link
Member

Choose a reason for hiding this comment

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

yes! I was confused, project slugs are used as subdomains, versions I don't think so

Copy link
Member

Choose a reason for hiding this comment

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

Ah, good call. Ignore me :)

help_text=_("Used in this version's URL"),
)

class Meta(object):
model = Version
fields = ['active', 'privacy_level', 'tags']
fields = ['slug', 'active', 'privacy_level', 'tags']

def __init__(self, *args, **kwargs):
super(VersionForm, self).__init__(*args, **kwargs)
if self.instance and self.instance.slug in (LATEST, STABLE):
self.fields['slug'].disabled = True
Copy link
Member

Choose a reason for hiding this comment

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

Besides making the field gray in the UI, does this validates this field in the backend also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. However, our CSS doesn't make the field grey.

self.fields['slug'].help_text += ugettext(' - read only for special versions')

def clean_slug(self):
slug = self.cleaned_data['slug']

version = self.instance
if version:
if Version.objects.filter(project=version.project, slug=slug).exclude(
pk=version.pk).exists():
raise forms.ValidationError(
_('There is already a version for this project with that slug'),
)

return slug

def save(self, commit=True):
obj = super(VersionForm, self).save(commit=commit)
Expand Down
4 changes: 3 additions & 1 deletion readthedocs/projects/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,9 @@ def build_versions_form(project):
version.identifier[:8],
)
else:
label = version.verbose_name
label = version.slug
if version.slug not in version.identifier:
label += ' ({})'.format(version.identifier)
Copy link
Member

Choose a reason for hiding this comment

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

This will show something like my-slug (a1b2c3d4)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only occurs where a version is not a tag so generally no. Looking a few lines above might clear this up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will look like my-slug (origin/branchname) typically.

attrs[field_name] = forms.BooleanField(
label=label,
widget=DualCheckboxWidget(version),
Expand Down
8 changes: 8 additions & 0 deletions readthedocs/projects/views/private.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
from django.views.generic import ListView, TemplateView, View
from formtools.wizard.views import SessionWizardView
from vanilla import CreateView, DeleteView, DetailView, GenericView, UpdateView

from readthedocs.builds.constants import STABLE, LATEST
from readthedocs.builds.forms import AliasForm, VersionForm
from readthedocs.builds.models import Version, VersionAlias
from readthedocs.core.mixins import ListViewWithForm, LoginRequiredMixin
Expand Down Expand Up @@ -154,6 +156,12 @@ def project_version_detail(request, project_slug, version_slug):
if request.method == 'POST' and form.is_valid():
version = form.save()
if form.has_changed():
if (version.active and 'slug' in form.changed_data and
version.slug not in (STABLE, LATEST)):
# Latest and Stable appear "changed" because they are disabled on the form
log.info('Triggering a build of the moved version')
trigger_build(version.project, version)

if 'active' in form.changed_data and version.active is False:
log.info('Removing files for version %s', version.slug)
broadcast(
Expand Down
9 changes: 8 additions & 1 deletion readthedocs/templates/projects/project_version_detail.html
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,16 @@
{% block content-header %}<h1>{% blocktrans with version.name as version_name %}{{ version_name }}{% endblocktrans %}</h1>{% endblock %}

{% block content %}
<h2> Editing {{ version.slug }} </h2>
<h2>{% blocktrans with version_name=version.verbose_name %}Editing version {{ version_name }}{% endblocktrans %}</h2>

<form method="post" action=".">
{% csrf_token %}

<p>
<label>{% trans 'Version control identifier' %}:</label>
<strong style="color: black">{{ version.identifier }}</strong>
</p>

{{ form.as_p }}
<p>
<input style="display: inline;" type="submit" value="{% trans "Save" %}">
Expand Down