-
-
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
Better help text for privacy level #3444
Conversation
readthedocs/projects/models.py
Outdated
@@ -211,8 +211,11 @@ class Project(models.Model): | |||
privacy_level = models.CharField( | |||
_('Privacy Level'), max_length=20, choices=constants.PRIVACY_CHOICES, | |||
default=getattr(settings, 'DEFAULT_PRIVACY_LEVEL', 'public'), | |||
help_text=_('(Beta) Level of privacy that you want on the repository. ' | |||
'Protected means public but not in listings.')) | |||
help_text=_('(Beta) Level of privacy that you want on the project. ' |
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.
Here I changed the word repository to project.
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.
Maybe the "Beta" work can be removed since it's not in beta anymore and I understood that you realized that it worked properly but what's is wrong is the docs, right?
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 also think this isn't beta anymore.
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.
Nice!
I think that this will change something after running python manage.py makemigrations
because you changed the help_text
. The migration will do nothing, but I think it's still needed.
Can you run that command and check if a file is generated? In that case, check the content and add the file to the PR also.
readthedocs/projects/models.py
Outdated
@@ -211,8 +211,11 @@ class Project(models.Model): | |||
privacy_level = models.CharField( | |||
_('Privacy Level'), max_length=20, choices=constants.PRIVACY_CHOICES, | |||
default=getattr(settings, 'DEFAULT_PRIVACY_LEVEL', 'public'), | |||
help_text=_('(Beta) Level of privacy that you want on the repository. ' | |||
'Protected means public but not in listings.')) | |||
help_text=_('(Beta) Level of privacy that you want on the project. ' |
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.
Maybe the "Beta" work can be removed since it's not in beta anymore and I understood that you realized that it worked properly but what's is wrong is the docs, right?
@humitos I got 4 migration files, just one relate to the projects model, do I need to push the others too? |
I'd say that you just have to push the one that it's related to your change and maybe open another PR for the other migrations. They are probably things that someone forgot to apply but shouldn't affect the database itself, anyway. |
I don't believe we should change the privacy help_text, and I don't believe it represents the truth for readthedocs.org. I believe private projects will never work for anyone, and we should remove the settings on the .org totally. Not sure the best transition path for projects, but we should remove the option from the forms, at least, so no new projects will set them. |
@ericholscher looks like there is an issue already about this #2663. Closing this PR in favor of discussing more about this in the other issue. |
This closes #3422
Also I made a little more consistent the help texts of the project model adding a period.
Please correct me if the text is not right, English is not my native language :).