Allow users to change version slug #11930

stsewd · 2025-01-15T22:36:59Z

Basically what I had in my old design doc #6273

When changing the slug, we check that it's a valid one (lowercase, only number, etc), in case the slug is not valid, we suggest a valid slug based on the invalid one. Why not just transform the value to a valid slug? Changing the slug is a breaking operation, we want to avoid surprises for the user, if they set a slug to "foo/bar", and then the actual slug is "foo-bar", that's unexpected.
project is used as s hidden field, following the same pattern we already have for other forms (this way all normal validations like the unique constraint work instead of getting a 500).
Editing the slug is disabled for machine created versions (stable, latest).
I'm not restricting the usage of latest or stable, since those will be non-machine created, similar to what a user does already if they name a version stable/latest.
If a version was active and its slug was changed, the resources attached to the old slug are removed before triggering a new build.
Cache is always purged after deleting resources from the version.

I was playing with using the "pattern" attribute for the input, so it validates the slug before submitting the form.

But I think showing the error with the suggested slug works better, maybe we can port that to JS instead as a future improvement.

Decisions

Expose this as a feature flag first, or just document it right away?

TODO

Allow changing the slug using the API.
Suggest a redirect from the old slug to the new one.

The idea implemented here comes from this comment: https://github.com/readthedocs/meta/discussions/147#discussioncomment-11789455

ref https://github.com/readthedocs/meta/discussions/147

humitos

These changes look very promising, in particular because they are pretty small. I did a quick test locally and I was able to change a slug and serve the documentation under the new URL 🎉

There are some things to consider still:

I understand we don't need to touch the sync versions code because it's based on verbose_name and we are not changing that, right?
There is no need to save the "old slug" because if required it can be generated based on the verbose_name, as we are doing currently.
The dashboard uses verbose_name when mentioning versions; so there is going to be a mismatch for those versions where we change the slug. What would be a good solution for this?
Flyout Addon uses slug to sort versions, which will keep working with the changes from this PR. However, I was told that we should use verbose_name there as well. In that case, sorting will break (Addons: use verbose_name instead of slug for sorting versions #11182). Should we keep the sorting by slug then?

readthedocs/builds/forms.py

stsewd · 2025-01-22T22:50:05Z

I guess everything is clear after the meeting, just answering for the record

I understand we don't need to touch the sync versions code because it's based on verbose_name and we are not changing that, right?

No, we don't need to.

There is no need to save the "old slug" because if required it can be generated based on the verbose_name, as we are doing currently.

I don't think we need to have access to the old slug at all, we can try to guess the original slug by inferring it from the verbose_name, but that isn't necessarily true as slugs can't be duplicated, so some end with _a suffixes. In any case, if we need the old slug for auditing, using a historical record is a better alternative.

The dashboard uses verbose_name when mentioning versions; so there is going to be a mismatch for those versions where we change the slug. What would be a good solution for this?

I suggested we use another field instead (display_name) that users can change to show in the UI/version selector. That way we don't change verbose_name which is used to keep a one to one match with the versions from the repo.

Flyout Addon uses slug to sort versions, which will keep working with the changes from this PR. However, I was told that we should use verbose_name there as well. In that case, sorting will break (
#11182). Should we keep the sorting by slug then?

I still think we shouldn't use the slug for sorting (the name we show to users in the listings won't match the order), if users want further customization, then sorting should be based on the new display_name field, so the listing matches the order.

ericholscher · 2025-01-22T23:42:13Z

I don't think we need to have access to the old slug at all, we can try to guess the original slug by inferring it from the verbose_name, but that isn't necessarily true as slugs can't be duplicated, so some end with _a suffixes. In any case, if we need the old slug for auditing, using a historical record is a better alternative.

This could be useful for creating redirects, but perhaps we just do that automatically when they rename the slug?

readthedocs/builds/models.py

stsewd · 2025-01-22T23:45:37Z

This could be useful for creating redirects, but perhaps we just do that automatically when they rename the slug?

My idea would be to create a notification or show the message after the user has done the rename (about a link to create a redirect), we don't need to keep that information forever, specially if the user changes the slug again.

ericholscher · 2025-01-22T23:51:31Z

This could be useful for creating redirects, but perhaps we just do that automatically when they rename the slug?

My idea would be to create a notification or show the message after the user has done the rename (about a link to create a redirect), we don't need to keep that information forever, specially if the user changes the slug again.

Yea, I think honestly even just putting it in cache and checking for that on the redirects page for a suggested redirect. I'd like to do something similar with the filetreediff stuff and redirect suggestions as well.

…n-slug

Instead of having a custom field with some extra complexity, we can achieve the same result by using a more standard django pattern, populating the field if isn't defined and using a validator. This will help to re-use this code in #11930.

Ref readthedocs/readthedocs.org#11930

humitos

Looks good with some suggestions/changes.

readthedocs/builds/forms.py

humitos · 2025-01-29T12:17:20Z

readthedocs/builds/forms.py

+        # NOTE: we call clean_resources over the instance with the previous slug,
+        # as all resources are associated with that slug.
+        if "slug" in self.changed_data and self._was_active:
+            self.instance.slug = self._previous_slug
+            self.instance.clean_resources()
+            self.instance.slug = self.cleaned_data["slug"]
+            # We need to set the flag to False,
+            # so the post_save method triggers a new build.
+            self._was_active = False


This looks a little hacky to me: setting the previous slug to the current instance, and then re-set it to the one the user has chosen. I understand why we are doing this, but I wonder if there is a more explicit way to call this cleanup, like running clean_resources(project_slug, version_slug) instead, or similar.

Agreed, seems like a small refactor could make this nicer.

readthedocs/rtd_tests/tests/test_build_forms.py

ericholscher

This looks great to me overall, will let @humitos 👍 it once the last review items are addressed.

ericholscher · 2025-01-30T19:27:44Z

readthedocs/builds/forms.py

+        # NOTE: we call clean_resources over the instance with the previous slug,
+        # as all resources are associated with that slug.
+        if "slug" in self.changed_data and self._was_active:
+            self.instance.slug = self._previous_slug
+            self.instance.clean_resources()
+            self.instance.slug = self.cleaned_data["slug"]
+            # We need to set the flag to False,
+            # so the post_save method triggers a new build.
+            self._was_active = False


Agreed, seems like a small refactor could make this nicer.

stsewd requested a review from a team as a code owner January 15, 2025 22:37

stsewd requested a review from humitos January 15, 2025 22:37

auto-assign bot assigned stsewd Jan 15, 2025

stsewd marked this pull request as draft January 15, 2025 22:37

humitos reviewed Jan 21, 2025

View reviewed changes

readthedocs/builds/forms.py Outdated Show resolved Hide resolved

humitos requested review from ericholscher and agjohnson January 21, 2025 10:31

ericholscher reviewed Jan 22, 2025

View reviewed changes

readthedocs/builds/models.py Show resolved Hide resolved

stsewd added 2 commits January 23, 2025 14:56

Allow users to change version slug

2d8290f

Fix order

a081a95

stsewd force-pushed the allow-users-to-change-version-slug branch from 6f13b4a to a081a95 Compare January 23, 2025 20:14

stsewd added 3 commits January 27, 2025 12:28

Delete old slug

016b996

Version: use field validator instead of custom field for slug

b40a234

Migration

df70ef5

stsewd mentioned this pull request Jan 27, 2025

Version: use field validator instead of custom field for slug #11957

Merged

stsewd added 7 commits January 27, 2025 13:38

Mark as safe

d331ff3

More safe

d4d437d

Merge branch 'refactor-unique-slug' into allow-users-to-change-versio…

368afb5

…n-slug

Check for unique slug

4aee207

Just check once

7275c08

Use html5 pattern?

5bdf63f

Format

b1ccb91

humitos mentioned this pull request Jan 28, 2025

Allow users to edit Version's identifier and/or slug #11814

Closed

stsewd added 2 commits January 28, 2025 10:10

Merge branch 'main' into allow-users-to-change-version-slug

e7b4fd1

Fix tests

a6a9a07

Dup

cc86c7b

readthedocs deleted a comment from Allenqel Jan 28, 2025

stsewd added 3 commits January 28, 2025 12:13

Tests

d1c15cb

Fixes

8f11e3d

Final updates

c153311

stsewd marked this pull request as ready for review January 28, 2025 18:32

stsewd added a commit to readthedocs/ext-theme that referenced this pull request Jan 28, 2025

Warn about changing a version slug

cdc5935

Ref readthedocs/readthedocs.org#11930

stsewd mentioned this pull request Jan 28, 2025

Warn about changing a version slug readthedocs/ext-theme#561

Open

stsewd requested review from humitos and ericholscher January 28, 2025 18:34

agjohnson linked an issue Jan 29, 2025 that may be closed by this pull request

Versions: allow editable versions #11818

Open

2 tasks

humitos reviewed Jan 29, 2025

View reviewed changes

Updates from review

941934b

ericholscher reviewed Jan 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users to change version slug #11930

Allow users to change version slug #11930

stsewd commented Jan 15, 2025 •

edited

Loading

humitos left a comment

stsewd commented Jan 22, 2025

ericholscher commented Jan 22, 2025

stsewd commented Jan 22, 2025 •

edited

Loading

ericholscher commented Jan 22, 2025

humitos left a comment

humitos Jan 29, 2025

ericholscher Jan 30, 2025

ericholscher left a comment

ericholscher Jan 30, 2025

Allow users to change version slug #11930

Are you sure you want to change the base?

Allow users to change version slug #11930

Conversation

stsewd commented Jan 15, 2025 • edited Loading

Decisions

TODO

humitos left a comment

Choose a reason for hiding this comment

stsewd commented Jan 22, 2025

ericholscher commented Jan 22, 2025

stsewd commented Jan 22, 2025 • edited Loading

ericholscher commented Jan 22, 2025

humitos left a comment

Choose a reason for hiding this comment

humitos Jan 29, 2025

Choose a reason for hiding this comment

ericholscher Jan 30, 2025

Choose a reason for hiding this comment

ericholscher left a comment

Choose a reason for hiding this comment

ericholscher Jan 30, 2025

Choose a reason for hiding this comment

stsewd commented Jan 15, 2025 •

edited

Loading

stsewd commented Jan 22, 2025 •

edited

Loading