add language dropdown at registration #1660
Conversation
|
Hi @laura-barluzzi! Thank you for this PR. I have an initial question. Why didn't you also include the preferred measurement system field as well in this PR? |
|
Hi @seav, simply because it was not asked in the issue #1654. Further, the |
There was a problem hiding this comment.
Looks good, just one small thing.
We shouldn't add any other fields to the sign-up form after the language field and the profile picture. I think the sign-up process should be quick and easy. Selecting the language is ok because the new user can use the platform using their preferred language from the start. Other settings like measurement can be set later.
cadasta/accounts/forms.py
Outdated
| @@ -17,11 +17,18 @@ class RegisterForm(SanitizeFieldsForm, forms.ModelForm): | |||
| email = forms.EmailField(required=True) | |||
| password = forms.CharField(widget=forms.PasswordInput()) | |||
| MIN_LENGTH = 10 | |||
| language = forms.ChoiceField( | |||
There was a problem hiding this comment.
Like in your previous PR, you don't need to define the field for the form explicitly; unless you want to use a form field different from the form field corresponding to the model field. For example, if you want to make a field that is not required on the model required through the form, or if you want to use a different widget (say the model defines a CharField but you only want to accept numbers in the form).
There was a problem hiding this comment.
Hi @oliverroick thanks for the feedback. About adding extra things to the signup, Chandra and I were planning to add only the language. We don't plan to put the measurement either the avatar choice.
About the form field comment, does this mean I should also remove the form field declaration in ProfileForm? We may have added the form.field to override the error message. I'm still not sure how to write a custom error message otherwise.
There was a problem hiding this comment.
I think you could remove the declaration in the Profile form as well.
Regarding custom error messages; it's possible like (Example is shamelessly stolen from the Django Docs:
from django.utils.translation import ugettext_lazy as _
class AuthorForm(ModelForm):
class Meta:
model = Author
fields = ('name', 'title', 'birth_date')
error_messages = {
'name': {
'max_length': _("This writer's name is too long."),
},
}
There was a problem hiding this comment.
Hi @laura-barluzzi. To add to what @oliverroick has already said, I don't think you really need to provide a custom error message for the language field in the registration form. A normal user will never encounter an error because this is a dropdown field with no normal way to select an invalid language. The only way you can force an error is if you manipulate the HTML or mess around with the HTTP communications. These are not something a normal user will do. So there is really no need to provide a custom error message. Besides, the default Django error message ("Select a valid choice. xx is not one of the available choices.") for the abnormal user that does mess things around is already quite good.
laura-barluzzi
force-pushed
the
feature/add_languange_in_registration
branch
from
6fd0b36 to
ebca6aa
Compare
|
Thank you @seav and @oliverroick for your feedback. I combined all your comments and I finally decided to simplify everything so that:
|
| @@ -310,42 +310,6 @@ def test_update_with_restricted_username(self): | |||
| assert serializer2.errors['username'] == [ | |||
| _("Username cannot be “add” or “new”.")] | |||
|
|
|||
| def test_update_with_invalid_language(self): | |||
There was a problem hiding this comment.
As I discussed with @oliverroick: We agreed that we still need tests for the serializers because this goes through the API and the user can provide any manner of input formatted any which way, so error checking is still needed. (The error-checking tests for the forms are OK to be deleted.)
|
@seav I added the serializer tests back, I just removed the |
amplifi
force-pushed
the
feature/add_languange_in_registration
branch
from
ae43b32 to
f13a3bb
Compare
Proposed changes in this pull request
Address issue #1654 for applying existing feature at the registration. Part of User Dashboard #1491 epic.
signup.htmltemplate and in theRegisterForm.languageandmeasurementfields by deleting respectiveform.fields, custom messages and testsWhen should this PR be merged
Risks
Follow-up actions
Checklist (for reviewing)
General
migrationlabel if a new migration is added.Functionality
Code
Tests
Security
Documentation