Skip to content

adding additional check of string for case insentive emails - #2471

Closed
geekguy wants to merge 1 commit into
strongloop:masterfrom
geekguy:case-senstive-email-bug
Closed

geekguy wants to merge 1 commit into
strongloop:masterfrom
geekguy:case-senstive-email-bug

Conversation

@geekguy

@geekguy geekguy commented Jun 26, 2016 •

Copy link
Copy Markdown
Contributor

Issue:

This line doesn't work when we have caseSensitiveEmail set to false and we make queries like,

User.findOne({where: {email: {inq: ["hello@test.com"]}}})

Additional check for string is required.

@slnode

slnode commented Jun 26, 2016

Copy link
Copy Markdown

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@slnode

slnode commented Jun 26, 2016

Copy link
Copy Markdown

Can one of the admins verify this patch?

3 similar comments
@slnode

slnode commented Jun 26, 2016

Copy link
Copy Markdown

Can one of the admins verify this patch?

@slnode

slnode commented Jun 26, 2016

Copy link
Copy Markdown

Can one of the admins verify this patch?

@slnode

slnode commented Jun 26, 2016

Copy link
Copy Markdown

Can one of the admins verify this patch?

@geekguy
geekguy force-pushed the case-senstive-email-bug branch from 06c05aa to 6cd6bff Compare June 27, 2016 17:30
@davidcheung

Copy link
Copy Markdown
Contributor

@geekguy thanks for your contribution! would you be able to add a test case to prevent regression on this fix?

@slnode

slnode commented Aug 31, 2016

Copy link
Copy Markdown

Can one of the admins verify this patch?

3 similar comments
@slnode

slnode commented Aug 31, 2016

Copy link
Copy Markdown

Can one of the admins verify this patch?

@slnode

slnode commented Aug 31, 2016

Copy link
Copy Markdown

Can one of the admins verify this patch?

@slnode

slnode commented Aug 31, 2016

Copy link
Copy Markdown

Can one of the admins verify this patch?

@bajtos

bajtos commented Oct 17, 2016

Copy link
Copy Markdown
Member

@slnode ok to test

@bajtos

bajtos commented Oct 17, 2016

Copy link
Copy Markdown
Member

@geekguy thank you for the pull request. As @davidcheung already mentioned, we need a unit test to verify your change.

BTW while the solution proposed here is a good short-term fix, for long term we should improve this code to correctly convert email values to lower case even for complex queries like the one shown above.

User.findOne({where: {email: {inq: ["HELLO@test.com"]}}})
// should be converted to
{where: {email: {inq: ["hello@test.com"]}}}

@geekguy

geekguy commented Oct 17, 2016

Copy link
Copy Markdown
Contributor Author

@bajtos : Can you please point me to a file where I should add test cases ?

I ll also check regarding converting complex queries to lower case.

@davidcheung

Copy link
Copy Markdown
Contributor

@geekguy perhaps like this test case, but instead of using the returned object, it makes another call using the where-inq query, then assert the object returned?

@davidcheung

Copy link
Copy Markdown
Contributor

fixes #2522

@bajtos

bajtos commented Oct 18, 2016

Copy link
Copy Markdown
Member

@geekguy Can you please point me to a file where I should add test cases ?

Sure. You can start by looking at #1804 which added the "access" hook and a test suite describe('Access-hook for queries with email NOT case-sensitive'), see

loopback/test/user.test.js

Lines 437 to 457 in ed76a34

describe('Access-hook for queries with email NOT case-sensitive', function() {
it('Should not throw an error if the query does not contain {where: }', function(done) {
User.find({}, function(err) {
if (err) done(err);
done();
});
});
it('Should be able to find lowercase email with mixed-case email query', function(done) {
User.settings.caseSensitiveEmail = false;
User.find({ where: { email: validMixedCaseEmailCredentials.email }}, function(err, result) {
if (err) done(err);
assert(result[0], 'The query did not find the user');
assert.equal(result[0].email, validCredentialsEmail);
done();
});
});
});

@davidcheung

Copy link
Copy Markdown
Contributor

taking over to rebase + add tests @geekguy on #2912
cc/ @bajtos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants