-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Allow users to change version slug #6204
Conversation
Machine created versions aren't allowed to be changed, since we use their harcoded slug to identify them as latest and stable. Ref readthedocs#5318
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this PR addresses only this part from #5318
we might need to refactor how we are managing version alias, and potentially more widely support alias management in our UI.
Overall, it looks good to me. I left some comments to improve small parts of the code, though.
@@ -15,7 +15,7 @@ | |||
{% block content-header %}<h1>{% blocktrans with version.name as version_name %}{{ version_name }}{% endblocktrans %}</h1>{% endblock %} | |||
|
|||
{% block content %} | |||
<h2> Editing {{ version.slug }} </h2> | |||
<h2> Editing {{ version.verbose_name }} </h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this so the user can see the original name of the version. They already see the slug now.
Yeah, before automating anything, it should be possible to make it manually This is how it looks with the new changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm 👍 with this PR.
Left two small suggestion, but it's ready to be merged to me.
@humitos if this is merged, API v3 should be updated to support this too. |
I'm a little worried that this is a confusing change. It allows users to change slugs, but not verbose_name? How exactly are we using verbose_name and slug now? Is slug now actually an alias, not a slug? Similarly, what happens to the files on the disk that have the old slug? This feels like a half-solution that doesn't really address all the problems that might be caused from this change. |
if not slugified: | ||
return self.fallback_slug | ||
return slugified | ||
class VersionSlugField(models.CharField): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did these files change? There's nothing in the PR description about what logic changes are here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes reusable the code to create valid slugs, logic is the same
@ericholscher raised a good point today: what has changed from #4505 that we now decided that we want to support changing a version slug? I think the main reason is that our code needs to support this because of #5318 --although, we may don't want to expose this feature to users in a simple form. I'm not sure, but it may be helpful to review the discussion in the original PR from @davidfischer and write a design document that goes through the benefits and the problems (together with their solutions). |
There is a design document that needs review first #6273 |
Machine created versions aren't allowed to be changed,
since we use their harcoded slug to identify them as latest and stable.
Ref #5318