NLP4ALL / nlp4all

NLP4All is a learning platform for educational institutions to help students that are not in data-oriented fields to understand natural language processing techniques and applications.
1 stars 0 forks source link

Feature/55 registration is a mess #61

Closed franciscoabenza closed 1 year ago

franciscoabenza commented 1 year ago
Screenshot 2022-11-07 at 16 39 45 Screenshot 2022-11-07 at 16 38 27
zeyus commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

zeyus commented 1 year ago

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

emilroenn commented 1 year ago

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

franciscoabenza commented 1 year ago

let me check

zeyus commented 1 year ago

@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 commented 1 year ago

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.

franciscoabenza commented 1 year ago

fair enough :)

On Tue, 7 Mar 2023 at 16:41, zeyus @.***> wrote:

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.

— Reply to this email directly, view it on GitHub https://github.com/NLP4ALL/nlp4all/pull/61#issuecomment-1458387896, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFIHMRY4JVQ5RNLBGHUL3VDW25JJ3ANCNFSM6AAAAAARZGS44Q . You are receiving this because you were mentioned.Message ID: @.***>