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

Remove unused login column #212

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

willianveiga
Copy link

Remove unused login column.

Fixes #116

Copy link
Member

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

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

What was the purpose of this column originally?

@willianveiga
Copy link
Author

Maybe it's not related to the same thing, but there was a login column since day one.

Considering this comment, is it possible to consider it related to unfinished and/or delayed plans?

@jarednorman
Copy link
Member

If there's not documentation as to what those plans were and the functionality doesn't exist now, I don't think it's worth keeping.

@mamhoff
Copy link
Contributor

mamhoff commented Dec 2, 2022

I was about to submit this PR just now, and second the removal of this column.

It stores a user's email address in login upon registration, but never updates it when the user updates their email, thus leaving us with garbage data.

@willianveiga would you be up to rebasing it? Otherwise I'll put up another PR with your changes here within a week.

@fthobe
Copy link

fthobe commented Jan 6, 2025

@mamhoff did you finish this up in the end?

@tvdeyen
Copy link
Member

tvdeyen commented Jan 6, 2025

Devise can be configured to which authentication keys it uses. If this is configured to use login (speak username) instead of email, we need this column. But this is personal preference of each store and maybe should be added there instead.

@fthobe
Copy link

fthobe commented Jan 6, 2025

So to summarise:

  • we should document that possibility
  • maybe also document the configuration on password complexity given that this is not vanilla devise from what I can see

And close PRs / tickets related to this topic.

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.

User.login can’t be modified once set
5 participants