Skip to content
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

Use a case-insensitive email field. (Requires Django 1.11.) #169

Merged
merged 5 commits into from Jun 21, 2017

Conversation

adam-thomas
Copy link
Contributor

@adam-thomas adam-thomas commented Jun 21, 2017

@incuna/backend review please?

@coveralls
Copy link

coveralls commented Jun 21, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 9d016c9 on email-case-insensitivity into a4506a1 on master.

@meshy
Copy link
Contributor

meshy commented Jun 21, 2017

This is cool, but does introduce a requirement on Postgres over other (lesser?) databases.

While I'm not sure it's foolproof, I've had success with the following, which I present as an alternative:

class User(...):
    ...

    def validate_unique(self, exclude=None):
        """
        Checks unique constraints on the model.

        Overridden to validate email in a case insensitive manner. Some of the
        logic has been derived from _perform_unique_checks().

        https://github.com/django/django/blob/1.9.7/django/db/models/base.py#L1014-L1030
        """
        # Validate email unless explicitly excluded.
        exclude = exclude or []
        errors = {}
        if 'email' not in exclude:
            qs = User.objects.filter(email__iexact=self.email)
            if not self._state.adding and self.pk is not None:
                qs = qs.exclude(pk=self.pk)
            if qs.exists():
                errors['email'] = self.unique_error_message(User, ('email',))
            exclude.append('email')

        try:
            super().validate_unique(exclude=exclude)
        except ValidationError as error:  # pragma: no cover
            # email is currently the only unique field, so impossible to test
            # this. Skipping from coverage (sorry)!
            errors.update(error.error_dict)

        if errors:
            raise ValidationError(errors)

@coveralls
Copy link

coveralls commented Jun 21, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 17ba7c3 on email-case-insensitivity into a4506a1 on master.

@adam-thomas
Copy link
Contributor Author

@meshy hmm, thanks! @incuna/backend opinions on Charlie's suggestion over my existing implementation?

@LilyFoote
Copy link
Contributor

Another alternative is to add a CIEmailUserMixin alongside EmailUserMixin. This is backwards compatible and allows users to move to the new postgres shiny if they desire.

@adam-thomas
Copy link
Contributor Author

That's a good idea.

@adam-thomas
Copy link
Contributor Author

Actually it kind of requires doubling up on all the mixins. If it's going to be backwards compatible it's probably better to use Charlie's suggestion. Since 1.11 is LTS it's probably ok to drop the support for older versions though.

@meshy
Copy link
Contributor

meshy commented Jun 21, 2017

If it helps to muddy the waters a little, I actually prefer your version, and would use it if I could :P

@adam-thomas
Copy link
Contributor Author

Lol. I think I prefer it as well because there's less of it (probably works more reliably) but I don't know how much this library is used on non-postgres systems.

@LilyFoote
Copy link
Contributor

It probably isn't used by anyone but Incuna (and maybe ex-Incuna people). So probably everyone uses postgres.

@Minglee01
Copy link
Contributor

my two cents here is a needs a changlog update

@adam-thomas
Copy link
Contributor Author

@Minglee01 oh I was going to do that once it's 'complete', but I can do it now

@coveralls
Copy link

coveralls commented Jun 21, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 49794d2 on email-case-insensitivity into a4506a1 on master.

@adam-thomas
Copy link
Contributor Author

Any other comments?

@adam-thomas adam-thomas merged commit 6784e33 into master Jun 21, 2017
@adam-thomas adam-thomas deleted the email-case-insensitivity branch June 21, 2017 13:06
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 17ba7c3 on email-case-insensitivity into a4506a1 on master.

4 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 17ba7c3 on email-case-insensitivity into a4506a1 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 17ba7c3 on email-case-insensitivity into a4506a1 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 17ba7c3 on email-case-insensitivity into a4506a1 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 17ba7c3 on email-case-insensitivity into a4506a1 on master.

]

operations = [
CITextExtension(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to require superuser to create the extension

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, how do we enable that? Would have thought we'd always have a superuser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants