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

Feature/55 registration is a mess #61

Closed
wants to merge 21 commits into from

Conversation

franciscoabenza
Copy link
Contributor

Screenshot 2022-11-07 at 16 39 45

Screenshot 2022-11-07 at 16 38 27

@franciscoabenza franciscoabenza linked an issue Nov 7, 2022 that may be closed by this pull request
@franciscoabenza franciscoabenza added enhancement New feature or request python Pull requests that update Python code labels Nov 9, 2022
Copy link
Contributor

@emilroenn emilroenn left a comment

Choose a reason for hiding this comment

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

I am approving now - but I am having a hard time comprehending whether we are now doing stuff with users in the backend that now only relies on their first name - which can then brake if multiple users use the same first name - which is likely to happen. PM me to talk about this please :D

@zeyus
Copy link
Member

zeyus commented Nov 11, 2022

I am approving now - but I am having a hard time comprehending whether we are now doing stuff with users in the backend that now only relies on their first name - which can then brake if multiple users use the same first name - which is likely to happen. PM me to talk about this please :D

Agreed, @franciscoabenza this looks fine, but the imc / quick registration should probably use "email" instead of first name

that said, for things that are specifically linked to a user account (e.g. robots, account page etc) then it's fine to keep it as first name like you have here, you just never want first name used as a key or id

@franciscoabenza
Copy link
Contributor Author

@zeyus @emilroenn
I think that this line check for a conflict with the known first_names in the db and add an aditional id if there is conflict.

            if any(User.query.filter_by(first_name=the_quick_name)):
                the_quick_name = the_quick_name + str(fake_id)

the thing is that the def register_imc(cls): is only use for the "quickly trying" feature. This accounts are not the ones that are intended for long term use, but just for trying out the platform. I suggest:

a) to get rid of the feature. (Given that registration is fast enough) (there is also no automatic removing of this accounts)
b) do not add a form. randomly generate all neccessary credentials

@franciscoabenza
Copy link
Contributor Author

If we think that this is going to be a real feature in the final product I will fix it 100% but I think like this is ok for now.
What do you think?

@emilroenn
Copy link
Contributor

Hmm, sounds like this should be covered at the meeting :D It sounds good for now though

@zeyus
Copy link
Member

zeyus commented Nov 21, 2022

@franciscoabenza I'm happy to merge this if you can resolve the conflicts

@emilroenn
Copy link
Contributor

@zeyus and @franciscoabenza what is the status of this? :D

@franciscoabenza
Copy link
Contributor Author

let me check

@zeyus
Copy link
Member

zeyus commented Nov 24, 2022

@franciscoabenza re: the username stuff, I think we can ask @arthurhjorth if the IMC registration is necessary...because for starters, without an email / credentials server, we can't send a verification email, it would make sense to have some additional security to prevent spam accounts etc but there are many options for user administration processes.

one other alternative is to provide a sandbox account, where you can do stuff, but not upload data or delete /edit stuff, or clean up the sandbox account each day, etc...but also just having a unique user that can play around is also ok, i think it would depend a lot on how much that feature is used, and how important it is

@zeyus
Copy link
Member

zeyus commented Mar 7, 2023

I'm going to close this PR, but leave the branch, because we probably want to change from custom username to email always, and it makes sense to copy the changes made here so people can also include their first and last name, but this PR is too outdated to merge and it'll be easier to just copy things form the branch.

@zeyus zeyus closed this Mar 7, 2023
@franciscoabenza
Copy link
Contributor Author

franciscoabenza commented Mar 8, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Registration is a mess
3 participants