Merge branch 'unstable' into complete-icons-migration

Author: MisRob

contentcuration/contentcuration/forms.py

@@ -1,12 +1,12 @@
 import json
-from builtins import object
 
 from django import forms
 from django.conf import settings
 from django.contrib.auth.forms import PasswordResetForm
 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

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')"
         />
         <TextField
           v-model="form.last_name"
           maxlength="100"
           counter
           :label="$tr('lastNameLabel')"
+          :error-messages="errors.last_name"
+          @input="resetErrors('last_name')"
         />
         <EmailField
           v-model="form.email"
           maxlength="100"
           counter
           :disabled="Boolean($route.query.email)"
-          :error-messages="emailErrors"
-          @input="emailErrors = []"
+          :error-messages="errors.email"
+          @input="resetErrors('email')"
         />
         <PasswordField
           v-model="form.password1"
+          :additionalRules="passwordValidationRules"
           :label="$tr('passwordLabel')"
+          :error-messages="errors.password1"
+          @input="resetErrors('password1')"
         />
         <PasswordField
           v-model="form.password2"
           :additionalRules="passwordConfirmRules"
           :label="$tr('confirmPasswordLabel')"
+          :error-messages="errors.password2"
+          @input="resetErrors('password2')"
         />
 
         <!-- Usage -->
@@ -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 => {
@@ -413,12 +435,11 @@
     },
     methods: {
       ...mapActions('account', ['register']),
-      ...mapActions('policies', ['openPolicy']),
       showTermsOfService() {
-        this.openPolicy(policies.TERMS_OF_SERVICE);
+        this.$router.push({ query: { showPolicy: policies.TERMS_OF_SERVICE } });
       },
       showPrivacyPolicy() {
-        this.openPolicy(policies.PRIVACY);
+        this.$router.push({ query: { showPolicy: policies.PRIVACY } });
       },
       showStorageField(id) {
         return id === uses.STORING && this.form.uses.includes(id);
@@ -438,8 +459,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 +492,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 +512,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)',

contentcuration/contentcuration/frontend/accounts/pages/Main.vue

@@ -153,12 +153,11 @@
     },
     methods: {
       ...mapActions(['login']),
-      ...mapActions('policies', ['openPolicy']),
       showTermsOfService() {
-        this.openPolicy(policies.TERMS_OF_SERVICE);
+        this.$router.push({ query: { showPolicy: policies.TERMS_OF_SERVICE } });
       },
       showPrivacyPolicy() {
-        this.openPolicy(policies.PRIVACY);
+        this.$router.push({ query: { showPolicy: policies.PRIVACY } });
       },
       submit() {
         if (this.$refs.form.validate()) {

contentcuration/contentcuration/frontend/accounts/pages/__tests__/create.spec.js

@@ -17,8 +17,8 @@ const defaultData = {
   first_name: 'Test',
   last_name: 'User',
   email: '[email protected]',
-  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) });

contentcuration/contentcuration/frontend/accounts/pages/__tests__/resetPassword.spec.js

@@ -39,8 +39,10 @@ describe('resetPassword', () => {
   });
   it('should call setPassword on submit if password data is valid', () => {
     wrapper.setData({ new_password1: 'pass', new_password2: 'pass' });
-    wrapper.find({ ref: 'form' }).trigger('submit');
-    expect(setPassword).toHaveBeenCalled();
+    wrapper.vm.$nextTick(() => {
+      wrapper.find({ ref: 'form' }).trigger('submit');
+      expect(setPassword).toHaveBeenCalled();
+    });
   });
   it('should retain data from query params so reset credentials are preserved', () => {
     router.replace({
@@ -50,7 +52,9 @@ describe('resetPassword', () => {
       },
     });
     wrapper.setData({ new_password1: 'pass', new_password2: 'pass' });
-    wrapper.find({ ref: 'form' }).trigger('submit');
-    expect(setPassword.mock.calls[0][0].test).toBe('testing');
+    wrapper.vm.$nextTick(() => {
+      wrapper.find({ ref: 'form' }).trigger('submit');
+      expect(setPassword.mock.calls[0][0].test).toBe('testing');
+    });
   });
 });

contentcuration/contentcuration/frontend/accounts/pages/resetPassword/ResetPassword.vue

@@ -9,6 +9,7 @@
       <PasswordField
         v-model="new_password1"
         :label="$tr('passwordLabel')"
+        :additionalRules="passwordValidationRules"
         autofocus
       />
       <PasswordField
@@ -52,6 +53,9 @@
       passwordConfirmRules() {
         return [value => (this.new_password1 === value ? true : this.$tr('passwordMatchMessage'))];
       },
+      passwordValidationRules() {
+        return [value => (value.length >= 8 ? true : this.$tr('passwordValidationMessage'))];
+      },
     },
     methods: {
       ...mapActions('account', ['setPassword']),
@@ -80,6 +84,7 @@
       resetPasswordPrompt: 'Enter and confirm your new password',
       passwordLabel: 'New password',
       passwordConfirmLabel: 'Confirm password',
+      passwordValidationMessage: 'Password should be at least 8 characters long',
       passwordMatchMessage: "Passwords don't match",
       submitButton: 'Submit',
       resetPasswordFailed: 'Failed to reset password. Please try again.',