diff --git a/contentcuration/contentcuration/forms.py b/contentcuration/contentcuration/forms.py index d9dc781f61..8e9320d85f 100644 --- a/contentcuration/contentcuration/forms.py +++ b/contentcuration/contentcuration/forms.py @@ -1,5 +1,4 @@ import json -from builtins import object from django import forms from django.conf import settings @@ -7,6 +6,7 @@ from django.contrib.auth.forms import UserChangeForm from django.contrib.auth.forms import UserCreationForm from django.core import signing +from django.core.exceptions import ValidationError from django.db.models import Q from django.template.loader import render_to_string @@ -16,23 +16,16 @@ REGISTRATION_SALT = getattr(settings, 'REGISTRATION_SALT', 'registration') -class ExtraFormMixin(object): - - def check_field(self, field, error): - if not self.cleaned_data.get(field): - self.errors[field] = self.error_class() - self.add_error(field, error) - return False - return self.cleaned_data.get(field) - - # LOGIN/REGISTRATION FORMS ################################################################# -class RegistrationForm(UserCreationForm, ExtraFormMixin): +class RegistrationForm(UserCreationForm): + CODE_ACCOUNT_ACTIVE = 'account_active' + CODE_ACCOUNT_INACTIVE = 'account_inactive' + first_name = forms.CharField(required=True) last_name = forms.CharField(required=True) - email = forms.CharField(required=True) - password1 = forms.CharField(required=True) + email = forms.EmailField(required=True) + password1 = forms.CharField(required=True, min_length=8) password2 = forms.CharField(required=True) uses = forms.CharField(required=True) other_use = forms.CharField(required=False) @@ -45,22 +38,18 @@ class RegistrationForm(UserCreationForm, ExtraFormMixin): locations = forms.CharField(required=True) def clean_email(self): - email = self.cleaned_data['email'].strip().lower() - if User.objects.filter(Q(is_active=True) | Q(deleted=True), email__iexact=email).exists(): - raise UserWarning + # ensure email is lower case + email = self.cleaned_data["email"].strip().lower() + user_qs = User.objects.filter(email__iexact=email) + if user_qs.exists(): + if user_qs.filter(Q(is_active=True) | Q(deleted=True)).exists(): + raise ValidationError("Account already active", code=self.CODE_ACCOUNT_ACTIVE) + else: + raise ValidationError("Already registered.", code=self.CODE_ACCOUNT_INACTIVE) return email - def clean(self): - super(RegistrationForm, self).clean() - - # Errors should be caught on the frontend - # or a warning should be thrown if the account exists - self.errors.clear() - return self.cleaned_data - def save(self, commit=True): - user = super(RegistrationForm, self).save(commit=commit) - user.set_password(self.cleaned_data["password1"]) + user = super(RegistrationForm, self).save(commit=False) user.first_name = self.cleaned_data["first_name"] user.last_name = self.cleaned_data["last_name"] user.information = { @@ -165,7 +154,7 @@ def save(self, user): return user -class StorageRequestForm(forms.Form, ExtraFormMixin): +class StorageRequestForm(forms.Form): # Nature of content storage = forms.CharField(required=True) kind = forms.CharField(required=True) @@ -194,7 +183,7 @@ class Meta: "audience", "import_count", "location", "uploading_for", "organization_type", "time_constraint", "message") -class IssueReportForm(forms.Form, ExtraFormMixin): +class IssueReportForm(forms.Form): operating_system = forms.CharField(required=True) browser = forms.CharField(required=True) channel = forms.CharField(required=False) @@ -204,7 +193,7 @@ class Meta: fields = ("operating_system", "browser", "channel", "description") -class DeleteAccountForm(forms.Form, ExtraFormMixin): +class DeleteAccountForm(forms.Form): email = forms.CharField(required=True) def __init__(self, user, *args, **kwargs): @@ -214,5 +203,5 @@ def __init__(self, user, *args, **kwargs): def clean_email(self): email = self.cleaned_data['email'].strip().lower() if self.user.is_admin or self.user.email.lower() != self.cleaned_data['email']: - raise UserWarning + raise ValidationError("Not allowed") return email diff --git a/contentcuration/contentcuration/frontend/accounts/pages/Create.vue b/contentcuration/contentcuration/frontend/accounts/pages/Create.vue index ba9f8bf338..6e6562ad5b 100644 --- a/contentcuration/contentcuration/frontend/accounts/pages/Create.vue +++ b/contentcuration/contentcuration/frontend/accounts/pages/Create.vue @@ -31,31 +31,40 @@ v-model="form.first_name" maxlength="100" counter - :label="$tr('firstNameLabel')" autofocus + :label="$tr('firstNameLabel')" + :error-messages="errors.first_name" + @input="resetErrors('first_name')" /> @@ -200,6 +209,7 @@ import Checkbox from 'shared/views/form/Checkbox'; import { policies } from 'shared/constants'; import DropdownWrapper from 'shared/views/form/DropdownWrapper'; + import commonStrings from 'shared/translator'; export default { name: 'Create', @@ -219,7 +229,6 @@ return { valid: true, registrationFailed: false, - emailErrors: [], form: { first_name: '', last_name: '', @@ -237,6 +246,13 @@ accepted_policy: false, accepted_tos: false, }, + errors: { + first_name: [], + last_name: [], + email: [], + password1: [], + password2: [], + }, }; }, computed: { @@ -247,6 +263,9 @@ passwordConfirmRules() { return [value => (this.form.password1 === value ? true : this.$tr('passwordMatchMessage'))]; }, + passwordValidationRules() { + return [value => (value.length >= 8 ? true : this.$tr('passwordValidationMessage'))]; + }, acceptedAgreement: { get() { return this.form.accepted_tos && this.form.accepted_policy; @@ -294,10 +313,12 @@ ]; }, usageRules() { - return [() => (this.form.uses.length ? true : this.$tr('fieldRequiredMessage'))]; + /* eslint-disable-next-line kolibri/vue-no-undefined-string-uses */ + return [() => (this.form.uses.length ? true : commonStrings.$tr('fieldRequired'))]; }, locationRules() { - return [() => (this.form.locations.length ? true : this.$tr('fieldRequiredMessage'))]; + /* eslint-disable-next-line kolibri/vue-no-undefined-string-uses */ + return [() => (this.form.locations.length ? true : commonStrings.$tr('fieldRequired'))]; }, sources() { return sources; @@ -359,7 +380,8 @@ ]; }, sourceRules() { - return [() => (this.form.source.length ? true : this.$tr('fieldRequiredMessage'))]; + /* eslint-disable-next-line kolibri/vue-no-undefined-string-uses */ + return [() => (this.form.source.length ? true : commonStrings.$tr('fieldRequired'))]; }, clean() { return data => { @@ -438,8 +460,27 @@ .catch(error => { if (error.message === 'Network Error') { this.$refs.top.scrollIntoView({ behavior: 'smooth' }); + } else if (error.response.status === 400) { + for (const field of error.response.data) { + if (!Object.prototype.hasOwnProperty.call(this.errors, field)) { + continue; + } + let message = ''; + switch (field) { + case 'password1': + message = this.$tr('passwordValidationMessage'); + break; + default: + /* eslint-disable-next-line kolibri/vue-no-undefined-string-uses */ + message = commonStrings.$tr('fieldHasError'); + break; + } + this.errors[field] = [message]; + } + this.registrationFailed = true; + this.valid = false; } else if (error.response.status === 403) { - this.emailErrors = [this.$tr('emailExistsMessage')]; + this.errors.email = [this.$tr('emailExistsMessage')]; } else if (error.response.status === 405) { this.$router.push({ name: 'AccountNotActivated' }); } else { @@ -452,12 +493,14 @@ } return Promise.resolve(); }, + resetErrors(field) { + this.errors[field] = []; + }, }, $trs: { backToLoginButton: 'Sign in', createAnAccountTitle: 'Create an account', - fieldRequiredMessage: 'Field is required', errorsMessage: 'Please fix the errors below', registrationFailed: 'There was an error registering your account. Please try again', registrationFailedOffline: @@ -470,6 +513,7 @@ passwordLabel: 'Password', confirmPasswordLabel: 'Confirm password', passwordMatchMessage: "Passwords don't match", + passwordValidationMessage: 'Password should be at least 8 characters long', // Usage question usageLabel: 'How do you plan on using Kolibri Studio (check all that apply)', diff --git a/contentcuration/contentcuration/frontend/accounts/pages/__tests__/create.spec.js b/contentcuration/contentcuration/frontend/accounts/pages/__tests__/create.spec.js index f2a201b560..143520329b 100644 --- a/contentcuration/contentcuration/frontend/accounts/pages/__tests__/create.spec.js +++ b/contentcuration/contentcuration/frontend/accounts/pages/__tests__/create.spec.js @@ -17,8 +17,8 @@ const defaultData = { first_name: 'Test', last_name: 'User', email: 'test@test.com', - password1: 'pass', - password2: 'pass', + password1: 'tester123', + password2: 'tester123', uses: ['tagging'], storage: '', other_use: '', @@ -126,6 +126,11 @@ describe('create', () => { expect(register).not.toHaveBeenCalled(); }); }); + it('should fail if password1 is too short', () => { + const wrapper = makeWrapper({ password1: '123' }); + wrapper.vm.submit(); + expect(register).not.toHaveBeenCalled(); + }); it('should fail if password1 and password2 do not match', () => { const wrapper = makeWrapper({ password1: 'some other password' }); wrapper.vm.submit(); @@ -155,7 +160,7 @@ describe('create', () => { it('should say account with email already exists if register returns a 403', async () => { wrapper.setMethods({ register: makeFailedPromise(403) }); await wrapper.vm.submit(); - expect(wrapper.vm.emailErrors).toHaveLength(1); + expect(wrapper.vm.errors.email).toHaveLength(1); }); it('should say account has not been activated if register returns 405', async () => { wrapper.setMethods({ register: makeFailedPromise(405) }); diff --git a/contentcuration/contentcuration/frontend/shared/translator.js b/contentcuration/contentcuration/frontend/shared/translator.js index b52eecaf59..7aac180a36 100644 --- a/contentcuration/contentcuration/frontend/shared/translator.js +++ b/contentcuration/contentcuration/frontend/shared/translator.js @@ -2,6 +2,7 @@ import { createTranslator } from 'shared/i18n'; const MESSAGES = { fieldRequired: 'This field is required', + fieldHasError: 'This field has an error', titleRequired: 'Title is required', licenseRequired: 'License is required', copyrightHolderRequired: 'Copyright holder is required', diff --git a/contentcuration/contentcuration/frontend/shared/views/form/EmailField.vue b/contentcuration/contentcuration/frontend/shared/views/form/EmailField.vue index fe0e641c42..ad28d0b91e 100644 --- a/contentcuration/contentcuration/frontend/shared/views/form/EmailField.vue +++ b/contentcuration/contentcuration/frontend/shared/views/form/EmailField.vue @@ -17,6 +17,8 @@