-
Notifications
You must be signed in to change notification settings - Fork 45
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
fix(registry): fix openid identifier match (case-insensitive) #551
Conversation
.where({ | ||
provider, | ||
}) | ||
.andWhereRaw('LOWER(identifier) = LOWER(?)', [identifier]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't it break mysql?
Also is it effective?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests on sqlite and mysql pass
Additional test for case-insensitive identifier added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding effectiveness https://stackoverflow.com/a/7005332/5825564
Can add an index on lower(identity column) though I'm not sure that this query and table is used very much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, lets see how it will affect DB load
e5f1ea5
to
4a539e1
Compare
e2e/spec/news.spec.e2e.ts
Outdated
@@ -12,7 +12,7 @@ Scenario('should open a news page and show news sources', async ({ I, newsPage: | |||
I.see('Pick a news source', newsPage.bannerHeadline); | |||
}); | |||
|
|||
Scenario.skip('should open an article page from a direct link', async ({ I, newsPage: newsPage }) => { | |||
Scenario.only('should open an article page from a direct link', async ({ I, newsPage: newsPage }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is only expected here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, not ready yet
4a539e1
to
b7d4abf
Compare
b7d4abf
to
0e7b58c
Compare
Coverage ReportIlc/serverCommit SHA:77ac0917200e316cd3098818c7893108d27a894a Test coverage results 🧪
File details
Ilc/clientCommit SHA:77ac0917200e316cd3098818c7893108d27a894a Test coverage results 🧪
File details
RegistryCommit SHA:77ac0917200e316cd3098818c7893108d27a894a Test coverage results 🧪
File details
|
No description provided.