Allow modifying version slugs #4505

davidfischer · 2018-08-10T23:05:04Z

This change allows version slugs to be modified.

This very explicitly breaks out the Version.slug (the thing in the URL, mutable) from Version.identifier (version control identifier, immutable). The explanations on the Version model are helpful.
When a version slug is changed, symlinks are rebuilt and a build is triggered. I suppose we could move the version on the filesystem but triggering a build is probably cleaner and works across the multiple filesystems we have.
The version slugs "latest" and "stable" are not editable

Screenshot

Alternatives

Another approach to this problem is to have an alias (eg. the readthedocs branch's docs could be accessed at /en/readthedocs/ or /en/rtd/). However, after considering this option, I decided that having one location is probably best. If there were multiple paths, in my opinion, one should redirect to the other (eg. /en/rtd/ -> /en/readthedocs/). Furthermore, the point of #4167 is to use the alias in favor of the actual version control branch name. I guess we could make the alias the canonical path or make that an option, but it seemed a bit like overengineering.

humitos · 2018-08-10T23:30:56Z

I like the idea. I need to check it in more details and make a proper review. Although, I want to anticipate a question: what would happen if a slug is exactly the same as another identifier?

For example, I have the branches my-branch and another-branch. I go to the another-branch Edit page and I put my-branch as slug. Is this a problem? How RTD will handle this?

With a quick look at the code, I think we don't handle that. We may need to add this check in the validator of the slug field.

humitos

I like this 🎉 I think it's something we need to support.

There are some edge cases that we need to think a little more. It would be good to have some tests on the normal flow but also for these weird (and maybe dumb) cases.

humitos · 2018-08-10T23:32:48Z

readthedocs/builds/forms.py

@@ -29,9 +33,22 @@ def __init__(self, instance=None, *args, **kwargs):  # noqa

 class VersionForm(forms.ModelForm):

+    slug = forms.CharField(


nit: there is a RegexField: https://docs.djangoproject.com/es/1.9/ref/forms/fields/#regexfield

humitos · 2018-08-10T23:34:19Z

readthedocs/builds/forms.py

+    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


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

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

humitos · 2018-08-10T23:39:02Z

readthedocs/projects/views/private.py

+
+                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:


What would happen if we de-active a version and change the slug in the same request?

Since this is triggering async task something weird may happen. Not sure yet, though.

I'll check this edge case.

humitos · 2018-08-10T23:41:30Z

readthedocs/projects/views/private.py

+                # Rebuild symlinks to the new slug, remove the old slug's symlink
+                # Latest and Stable would appear here because they are disabled on the form
+                log.info('Rebuilding symlinks for %s. A version slug changed', version.project)
+                broadcast(type='app', task=tasks.symlink_project, args=[version.project.pk])


We are already calling this on Version.save which is called from line 157 (form.save()).

I'm not 100% sure, but I think that if the version is set as active a build is triggered for that version when it's saved also.

In case of activate the version and changing the slug we will be doing this twice. I, think :)

We are already calling this on Version.save which is called from line 157

Good catch

I'm not 100% sure, but I think that if the version is set as active a build is triggered for that version when it's saved also.

I don't see this anywhere and in my testing this did not occur.

humitos · 2018-08-10T23:49:05Z

readthedocs/projects/forms.py

-            label = version.verbose_name
+            label = version.slug
+            if version.slug not in version.identifier:
+                label += ' ({})'.format(version.identifier)


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

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

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

davidfischer · 2018-08-11T05:27:05Z

Is this a problem? How RTD will handle this?

In my tests, there was a unique key on this field. However, it might be best to have a test for this.

davidism · 2018-08-11T18:34:32Z

Definitely want to be able to handle conflicting slugs. For my use case:

There is a 1.0 tag.
There is a 1.0.1 tag.
There is a 1.0-maintenance branch.
The 1.0 and 1.0.1 builds are not active.
The 1.0-maintenance build should have the "1.0" slug.

davidfischer · 2018-08-13T20:38:11Z

Definitely want to be able to handle conflicting slugs

I might have to rethink some of this. There is a unique key on the the project/slug pair.

davidfischer · 2018-08-13T21:16:54Z

I might have to rethink some of this. There is a unique key on the the project/slug pair.

Maybe this isn't a huge deal. You could always edit the tag 1.0's slug and then edit the branch 1.0-maintenance's slug to be 1.0. It's a little roundabout but I suppose it would work. It does seem to be a bit of misdirection as opposed to just naming the branch 1.0.

- Check for existing versions - Use a RegexField - Double resyncing of symlinks

davidfischer · 2018-08-13T21:56:05Z

I made some changes based on feedback. I added a check for existing versions with the same slug and fixed the double syncing of symlinks.

timgraham · 2018-08-14T16:50:21Z

readthedocs/builds/forms.py

+        version = self.instance
+        if version:
+            if Version.objects.filter(project=version.project, slug=slug).exclude(
+                    pk=version.pk).count() > 0:


Use .exists() rather than .count() > 0?

Thanks. I feel like I do this all the time.

humitos · 2018-08-14T18:35:17Z

New changes look good! Besides the comment about the exists, if we add some tests for the new feature, I'd say that we are ready to merge.

Also, the current tests are failing for some weird reason, I think.

davidism · 2018-08-14T18:53:02Z

I can work around unique slugs by renaming problematic ones. It shouldn't come up too often, and I think that next time I'll make the tag 2.0.0 to avoid ambiguity.

That's going to be confusing for users unless it's well documented though.

davidfischer · 2018-08-14T19:35:07Z

That's going to be confusing for users unless it's well documented though.

Well, it will throw an error message that should make it reasonably obvious: "There is already a version for this project with that slug"

stsewd · 2018-08-15T06:27:15Z

readthedocs/templates/projects/project_version_detail.html

    <form method="post" action=".">
      {% csrf_token %}
+
+      <p>
+      <label>Version control identifier:</label>


blocktrans here too?

Probably just regular {% trans %} but yes.

stsewd · 2018-08-15T06:28:12Z

readthedocs/templates/projects/project_version_detail.html

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


The close tag is not at the same level

ericholscher

I'm +1 on allowing editing versions.

I do worry about the logic that happens when the versions are synced. Will a tag get named 2.0_1 or similar if there already exists a Version named 2.0 in the database? I guess that's not the end of the world, but it definitely could be confusing for users to have the Versions not line up with the VCS elements.

I'm also curious if there are other places where we're using the slug for something when we really want the identifier. I looked through the sync_versions code and it looks OK, so we're probably safe there.

ericholscher · 2018-08-21T19:44:45Z

readthedocs/builds/forms.py

@@ -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,


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

There is some discussion around that here #3464

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

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.

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?

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

Ah, good call. Ignore me :)

ericholscher · 2018-08-21T20:06:12Z

This will also end up effectively deleting the old slug, so there should be a really big warning about this for users. I wonder if we can add a redirect automatically to the project that will fix the old URL's and turn them into the new URL's, to make this less of a bad UX.

davidfischer · 2018-08-21T22:55:06Z

I guess that's not the end of the world, but it definitely could be confusing for users to have the Versions not line up with the VCS elements.

It's the same when somebody has a 1.0 branch and then they create a 1.0 tag.

This will also end up effectively deleting the old slug, so there should be a really big warning about this for users. I wonder if we can add a redirect automatically to the project that will fix the old URL's and turn them into the new URL's, to make this less of a bad UX.

I don't think we can add a long term redirect because at least in the Pallets use case they plan to repurpose that. Flask for example, has a 1.0 tag but they want the version 1.0 to correspond to the 1.0-maintenance branch. Perhaps since this is a dangerous operation maybe it should be a staff-only thing. That's pretty inconvenient but it avoids people shooting themselves in the foot.

ericholscher · 2018-08-22T09:53:10Z

I don't think we can add a long term redirect because at least in the Pallets use case they plan to repurpose that. Flask for example, has a 1.0 tag but they want the version 1.0 to correspond to the 1.0-maintenance branch. Perhaps since this is a dangerous operation maybe it should be a staff-only thing. That's pretty inconvenient but it avoids people shooting themselves in the foot.

That is currently how it already works. We can edit slugs in the admin, but we generally don't because it's destructive if any URL's exist to the version. I still think if we expose this to users, there's a good chance they will shoot themselves in the foot by renaming a slug that is actively used, and then 404 a bunch of links.

I think creating redirects is a good solution to this. Redirects will only fire on 404's, so if the project reuses the slug, they will mostly just get ignored. Though that could also lead to some weird behavior where 404's on the slug will redirect, but not active pages, which isn't great UX either. So I think probably the best thing to do is make sure it's really explicit that it will break links and shouldn't be done with existing versions.

humitos · 2018-08-22T16:30:22Z

I originally liked the idea of the ability to have alias and modify version's slug, but it's bringing some potential problems where we should be careful.

Adding this as a "staff only" thing it doesn't worth for me. Core team could do this very easily from the Django shell. I did exactly this for the Django folks some days ago at #4519

dversion = Version.objects.create(
  slug='2.1.x',
  active=True,
  project=django,
  identifier='origin/stable/2.1.x',
  machine=False,
  type='branch',
  privacy_level='public',
  verbose_name='2.1.x',
)

I didn't change the slug or remove a Version, I created a new one with the proper slug and the reference they need to point to.

We can do this for Pallets folk also if that's what they need.

I'd keep thinking on making this more User Friendly if we have plenty of users with this needings. Otherwise, we can help Pallets folks to onboard by creating these alias manually and keep thinking what's the best way to solve the general problem.

davidfischer · 2018-08-22T17:57:30Z

We can do this for Pallets folk also if that's what they need.

Well, we need to modify 2 versions because they already have a 1.0 tag. See the comment above. We can do it, but if they continue to use branches of the form 1.1-maintenance, 1.2-maintenance then every time they create a new branch, they'll need to contact us.

If we believe the best way forward is to leave modifying version slugs to staff and @davidism is happy with that then I'm happy to close this PR and move forward in that direction.

davidfischer · 2018-08-25T22:34:11Z

I am going to close this PR as I believe we are just going to either rename branches or edit version slugs in the admin to handle this.

ericholscher · 2018-09-04T07:35:59Z

Perhaps we should update this FAQ item to say you can ask us to change it? https://docs.readthedocs.io/en/latest/faq.html#how-do-i-change-my-slug-the-url-your-docs-are-served-at

humitos · 2018-09-04T08:45:18Z

@ericholscher that's a good idea, yeah.

Allow modifying version slugs

9fd95f2

davidfischer requested a review from a team August 10, 2018 23:05

davidfischer mentioned this pull request Aug 10, 2018

Add version automation rules #4001

Closed

3 tasks

humitos reviewed Aug 10, 2018

View reviewed changes

Updates based on feedback

532de0b

- Check for existing versions - Use a RegexField - Double resyncing of symlinks

stsewd mentioned this pull request Aug 14, 2018

Add 2.1.x version alias for Django docs #4519

Closed

timgraham reviewed Aug 14, 2018

View reviewed changes

Get the PR merge ready

b260814

stsewd reviewed Aug 15, 2018

View reviewed changes

Updates based on feedback - good catches!

a010105

ericholscher reviewed Aug 21, 2018

View reviewed changes

davidfischer closed this Aug 25, 2018

stsewd deleted the davidfischer/change-version-slugs branch September 14, 2018 17:37

stsewd mentioned this pull request Sep 14, 2018

Add change versions slug to faq #4633

Merged

humitos mentioned this pull request Oct 10, 2019

Allow users to change version slug #6204

Closed

stsewd mentioned this pull request Oct 10, 2019

Design doc for changing version's slug #6273

Closed

astrojuanlu mentioned this pull request Aug 28, 2023

Rename latest #10674

Closed

		@@ -29,9 +33,22 @@ def __init__(self, instance=None, args, *kwargs): # noqa

		class VersionForm(forms.ModelForm):

		slug = forms.CharField(

Allow modifying version slugs #4505

Allow modifying version slugs #4505

Conversation

davidfischer commented Aug 10, 2018

Screenshot

Alternatives

humitos commented Aug 10, 2018

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfischer commented Aug 11, 2018

davidism commented Aug 11, 2018

davidfischer commented Aug 13, 2018

davidfischer commented Aug 13, 2018

davidfischer commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Aug 14, 2018

davidism commented Aug 14, 2018

davidfischer commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericholscher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericholscher commented Aug 21, 2018

davidfischer commented Aug 21, 2018

ericholscher commented Aug 22, 2018 • edited Loading

humitos commented Aug 22, 2018

davidfischer commented Aug 22, 2018

davidfischer commented Aug 25, 2018

ericholscher commented Sep 4, 2018

humitos commented Sep 4, 2018

ericholscher commented Aug 22, 2018 •

edited

Loading