-
-
Notifications
You must be signed in to change notification settings - Fork 72
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
Reduce hardcoding of consent labels in UI #2361
Reduce hardcoding of consent labels in UI #2361
Conversation
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 left some comments where I'm not sure the solution is needed, but hey I may be wrong so let's have a discussion :) The rest looks good.
migrations.AddField( | ||
model_name="term", | ||
name="short_description", | ||
field=models.CharField(null=True, max_length=100), |
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.
You should be able to create the field with ""
as default value, therefore there should be no need switch null=True
and then null=False
in a migration.
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.
Wouldn't you then have to do the same thing with blank=True
and blank=False
instead? I don't think we want this field to be permitted to be blank as it may appear in the UI through lines like this https://github.com/carpentries/amy/pull/2361/files#diff-a97c97acc5cd76b1c48999bacdc2b399b62f678806ab48b559dcc0944c366536R278
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 think blank
is only applied when creating model instances. You definitely should be fine if you provide one-off value for the migration (default value of ""
). makemigrations
may even ask you for it.
@@ -37,15 +37,15 @@ | |||
</th><td>{{ user.secondary_email|default:"—" }}</td></tr> | |||
<tr><th>Gender:</th><td>{{ user.get_gender_display }}{{ user.gender_other }}</td></tr> | |||
<tr> | |||
<th>Can we contact you?</th> | |||
<th>{{ consents_content.may_contact }}</th> |
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.
Is this the same as consents.public_profile.term.content
? If yes, then I'm not sure we need consents_content
object.
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.
The content is the same, but if the user hasn't set that consent, consents.public_profile
won't exist.
Merging now. I've also made that change to the content of |
Fixes #2302 .
Also propose updating the
content
field of thepublic-profile
consent to "Do you consent to publish your profileon The Carpentries website?" in the AMY Django admin interface.