Dropdown to select Advanced Settings #4710
Conversation
|
Just pointing to previous comments in the another PR/issue We can't do that, because the original field is a charfield type.
Not sure if that is necessary, I think it only adds complexity, for example: when the project is created for first time, we don't have all the versions.
Actually, we do deletions too (when syncing the versions)
We definitely can do this, I don't think there are weird corner cases, I'll implement that tomorrow. |
readthedocs/projects/forms.py
Outdated
| default_choice = (None, '-' * 9) | ||
| versions = self.instance.versions.values_list('slug', 'verbose_name') | ||
| self.fields['default_branch'].widget = forms.Select( | ||
| choices=[default_choice] + list(versions) |
There was a problem hiding this comment.
For default branch we need to list ALL versions (branches and tags, active and inactive ones)
readthedocs/projects/forms.py
Outdated
| 'slug', 'verbose_name' | ||
| ) | ||
| self.fields['default_version'].widget = forms.Select( | ||
| choices=versions |
There was a problem hiding this comment.
Here we only need the active ones
readthedocs/builds/forms.py
Outdated
| raise forms.ValidationError( | ||
| _(msg.format(self.instance.verbose_name)) | ||
| ) | ||
| return privacy_level |
There was a problem hiding this comment.
This logic doesn't hold up on our commercial hosting. If private repositories are allowed on the instance, then we should allow any value here.
There was a problem hiding this comment.
I just fixed the test that was failing because of the privacy restrictions 😢
readthedocs/projects/forms.py
Outdated
|
|
||
| versions = ( | ||
| self.instance.all_active_versions() | ||
| .filter(privacy_level=PUBLIC) |
There was a problem hiding this comment.
This also doesn't hold up on our commercial hosting. It might be best to leave this sort of logic out of a query entirely, but at very least, it should only be checking this conditionally, if private privacy is not allowed.
|
This is ready, I also tested it locally.
|
readthedocs/projects/forms.py
Outdated
| choices=[default_choice] + list(versions) | ||
| ) | ||
|
|
||
| versions = self.instance.all_active_versions().values_list( |
There was a problem hiding this comment.
instead of reusing the versions variable name, use active_versions for easy reading.
| def __init__(self, *args, **kwargs): | ||
| super(ProjectAdvancedForm, self).__init__(*args, **kwargs) | ||
|
|
||
| default_choice = (None, '-' * 9) |
There was a problem hiding this comment.
I Pythonic way to do this would be to use something like empty_value: https://docs.djangoproject.com/en/2.1/ref/forms/fields/#django.forms.TypedChoiceField.empty_value
I'm not 100% sure that this is possible here, but I think it should be.
There was a problem hiding this comment.
We are just replacing the widget (Select), so it's not possible https://docs.djangoproject.com/en/2.1/ref/forms/widgets/#django.forms.Select
readthedocs/builds/forms.py
Outdated
| 'it should be active.' | ||
| ) | ||
| raise forms.ValidationError( | ||
| _(msg.format(self.instance.verbose_name)) |
There was a problem hiding this comment.
The message should be first translated and after that, formatted.
So, the _ function has to be added in the line 46 when the text is created.
|
@humitos done, thanks for the review! |
|
This is read to merge from my point of view. Let's wait for the @agjohnson 👍, though. |
|
Changes look great to me! |
PR on top of #3676
Closes #3610
Still wip (needs tests and validate the revert case)