Skip to content

users-email - Lowercase the email value on CRUD operations for deta and USERS kv - #49

Open
ashishjullia wants to merge 1 commit into
portunus-dev:mainfrom
ashishjullia:lowercase-user-email
Open

users-email - Lowercase the email value on CRUD operations for deta and USERS kv#49
ashishjullia wants to merge 1 commit into
portunus-dev:mainfrom
ashishjullia:lowercase-user-email

Conversation

@ashishjullia

@ashishjullia ashishjullia commented Jul 20, 2023

Copy link
Copy Markdown
Contributor

This fixes #48

Comment thread handlers.js
}

let kvUser = await USERS.get(userEmail, { type: 'json' })
let kvUser = await USERS.get(userEmail.toLowerCase(), { type: 'json' })

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This would ignore issues retrospectively, such that if the KV already has the user's email in different cases, USERS.get will have a miss

@ashishjullia ashishjullia Jul 20, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, it will but that can be solved by manually making the ones already in kv USERS to lowercase and in deta collection users?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how would you resolve this issue in a backward compatible way, without the batch update in the existing deta base?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, that's what I meant to solve this a bulk update is required to the existing USERS and users.

@ashishjullia ashishjullia Jul 21, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But there is one catch here, a bulk update to lowercase the email can be done but if there is a case where there are two entries for (say) ashishjullia19@gmail.com and ASHISHJULLIA19@GMAIL.COM, then if both have a few teams/stages/key:value(s) then this process can lead to a failure in such cases where either nit-pick the ones that can be discarded from the db means whether to keep ashishjullia19@gmail.com or ASHISHJULLIA19@GMAIL.COM (other making the email lowercase).

@ashishjullia

Copy link
Copy Markdown
Contributor Author

fyi @woozyking attempted to fix the issue.

Otherwise, currently the behaviour is as follows:
Although ashishjullia19@gmail.com and ASHISHJULLIA19@GMAIL.COM are same but being addressed as separate users, results in inconsistency.

image

image

image

image

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.

Lowercase Email Address before saving to DB

2 participants