Mentorship-backend: Update the user with existing name and username.

[faznan3nazer]

When you are updating the user with existing name and username, you are getting an error message "A user with that username already exists."
To Reproduce

Steps to reproduce the behavior:

Current username: zxcvbnm
Updating username to: zxcvbnm,
Current name: zxcvbnm
Updating name to: zxcvbnm,

1. Go to 'User-update user profile'.
2. Provide the access token.
3. .{
   "name": "zxcvbnm",
   "username": "zxcvbnm",
   "bio": "string",
   "location": "string",
   "occupation": "string",
   "organization": "string",
   "slack_username": "string",
   "social_media_links": "string",
   "skills": "string",
   "interests": "string",
   "resume_url": "string",
   "photo_url": "string",
   "need_mentoring": true,
   "available_to_mentor": true
   }
4. Click on 'Execute'.
5. Scroll down to 'Responses'.
6. 
   

Expected behavior
It should either be successfully updated message or a message like "no fields have been updated". Also the error message being shown now is not listed under responses.

Desktop (please complete the following information):

• OS: Windows 10
• Browser chrome

[gaurivn]

I added the Status: On Hold since I thought of including this issue for the upcoming Open Source Hack.

[alapan-sau]

I would like to work on this issue for OSH.

[gaurivn]

Hi @alapan-sau, this issue will be made available once Open Source Hack starts on September 1st, 2020. Till then you can set up the development environment for and familiarise yourself with mentorship-backend. If you've any doubts feel free to ask them on Zulip.

[alapan-sau]

As OSH is open today, May I work on this issue?

[gaurivn]

As OSH is open today, May I work on this issue?

Hi @alapan-sau, you've already been assigned issue #746, once you're done with sending a PR for the same, you can work on available issues.

[faznan3nazer]

I would like to work on this issue... Can you please assign this to me for Open-Source Hack?

[gaurivn]

I would like to work on this issue... Can you please assign this to me for Open-Source Hack?

Sure @faznan3nazer, here you go

[vj-codes]

@faznan3nazer making it available as you are already assigned #606

[pallavisavant]

can i work on this issue ...Thanks.

[vj-codes]

Assigning you @pallavisavant
Happy coding!

[pallavisavant]

@vj-codes I am facing one issue while reproducing this....we get the access token by giving username and password in login section rgt,and after that we are supposed to use this token for updating ...it shows invalid token wherever i am using it.Can anyone help ..Thanks.

[vj-codes]

@pallavisavant this my be mainly because you might have pasted the token directly while the syntax is Bearer <access_token>
Just add the word Bearer before ur token no brackets and it works good OR you might have taken the token from Mentorship Relation /login instead of Users/login
I believe the 1st is the reason for error ,was able to produce it from my side
Do update if it still doesn't work

[pallavisavant]

@vj-codes yes u r absolutely right...the first one was the cause....it got resolved...Thank you so much.😊

[pallavisavant]

This isn't a issue logically....the username should be unique because we use username/email while login ,so while updating user profile if the username is something that already exists...then it should prompt that "username exists already use a different one".
@vj-codes correct me if i am wrong....Thanks.

[vj-codes]

@pallavisavant this issue isn't about the updated username being different from current one , this is about both are same
If I have a username asIam_tester and I update it to Iam_tester on swagger , what is expected is a message like
no fields have been updated but what I get is something like username already exists (which is true but our aim was to tell user that current and and what is to be updated is same which cannot be possible)

[vj-codes]

@pallavisavant does it solve your query?If not do lemme know

[pallavisavant]

@vj-codes ok i am assuming that on detecting same username...it will give no error but run successfully and prompt message as "no fields have been updated".....one doubt i had was what if user uses same username but changes other details like eg :location then it should update the location rgt though username is same....Thanks.

[mtreacy002]

@pallavisavant and @vj-codes . If I'm not mistaken, this issue was raised since atm user cannot update their details without changing their username. This should not be the case because of 2 reasons:

1. it's normal for users to update details every now and then, and they shouldn't be asked to change username just for this purpose.
2. If they have to change their username every time they are updating their details, this means there'll be more than one record of this user in the database, one for each details update that they would've done with all the new usernames (since an update with the same username is currently not possible). This should not be the accepted logic.

So, to answer your question above, @pallavisavant, the code change should allow the change of details with the same username.

Note: "no fields have been updated" would've been the expected return if the same username is detected but no changes in that user details are detected in the payload.

Does this make sense? cc @anitab-org/mentorship-backend-maintainers, please confirm whether or not this is the case.

[vj-codes]

@mtreacy002 hey firstly I attended your demo live it was great:)
And your point above does make sense and just noticed that updating username has been removed android apk too
But what I think is username is public to everyone so I would say the option should be available , example
when I had first created the acc in dec last year didn't think of a proper username and I was studying chemistry so wrote it as carbon_dioxide :( Now , when I think of it I feel the username makes least sense

Similarly , what if there's a typo mistake while registering username?When there's a mistake in email I get a toast message but whatever I have written in username will be final
Updating username will indeed generate more database but how about we keep a limit to updates like the feature will be blocked after 3 updates or something . Even updating password generates more records
All I want to say is username update will be a user friendly feature

[vj-codes]

@mtreacy002 do you also mean to say that updating username should be removed from swagger

[mtreacy002]

Thanks, @vj-codes for your for attending my demo 😁😉.

I agree with you that, changing username can be also allowed along with updating other details, however it should not be made compulsory (so, user can keep it as it is if they want to).
More importantly, if we are to allow the change of username as part of details update, there should be a logic to prevent multiple user data get recorded in the database, since we should allow only one source of truth for user details.
The existing code doesn't have this logic in place. Instead, it treats the change of username in the same manner as adding a new user to the database (so we'll end up having multiple records for the same user, which shouldn't be allowed). What do you think? I'm interested on the outcome of this issue as this is part of cross-project issues we have with Bridge-In-Tech project as mentioned in issue #74 and #54

[vj-codes]

@mtreacy002 now got it , thanks for explaining again .
So should I close this issue which is marked for OSH? or wait until Isabel or other maintainers reply
And if there are more regarding the same can I tag you there for your opinion?

[mtreacy002]

For OSH purposes, I reckon we should keep it simple. For now we can say that the goal/task is to allow user to update details without having to change username. Later outside OSH, we can open a new issue to allow user to change username with additional logic put in place so that no multiple record of that user is allowed. This task (putting logic to prevent multiple records of same user) is a bit complex since it might require delete logic which is another discussion on its own whether hard vs soft delete would be involved in the process. cc @anitab-org/mentorship-backend-maintainers / @anitab-org/coding-team, can we have your opinion on this?

[pallavisavant]

@mtreacy002 if this is finalized..then we can do it this way like even if the username entered by user is already existing it will update the other details if i got this correct...Thanks.

[mtreacy002]

@mtreacy002 if this is finalized..then we can do it this way like even if the username entered by user is already existing it will update the other details if i got this correct...Thanks.

that is exactly what I have in mind, @pallavisavant. 😉. @vj-codes , what do you think?

[vj-codes]

Yes sure @mtreacy002 @pallavisavant

[mtreacy002]

@pallavisavant and @vj-codes, I have to say, I've just realised that I was wrong about one thing. After double checking the code I realised that the current update doesn't create a new record. So we don't have to worry that current logic as it is would create multiple records if user update their details using different username, it won't. The bug in the current logic is only because it makes it compulsory for user to change their username with each update that is a bit out of the norm.
So the task is not as complicated as I thought it might be. Now I believe that even if you want to allow username to be changed upon update within this PR, it shouldn't extort too much of extra effort compared to just allowing update using the same username. @vj-codes, it's your call to which goal you want to set for now 😉. Sorry again for my misconception.

[pallavisavant]

@vj-codes pls do review and let me know if it requires any changes to be made.....Thanks.

[epicadk]

Can I work on this ? @vj-codes

[vj-codes]

Assigning you @epicadk
Happy coding!

[epicadk]

Should I also handle race conditions? @isabelcosta @faznan3nazer

[isabelcosta]

@epicadk which race conditions?

[epicadk]

@epicadk which race conditions?

Currently , we first check if the username is available or not and then proceed to updating it . However this isn't how it should be implemented because what happens when two users try to register with the same username and almost the same time? It would lead to an unexpected error. So we shouldn't query the database to check if the username if available or not but rather just update the username and handle the exception if a user with that username already exists.

[epicadk]

@isabelcosta I can't seem to find the link to the resource where I read about this but I can try to explain here. So consider 2 users user1 and user2. Suppose both of them want to ( in this case) change their existing username to the new same username. So what'll happen is first user1's request will check if the username is already taken and it will find that the username does not exist in the database, at the same time user2's request can also query the database to check if the username exists and since user1's data hasn't been updated yet user2 will also find that the username is available. Consider User1's data is now updated however when user2 attempts to update their data an exception will be thrown and the app will crash if this is not handled.

[epicadk]

I'm sorry that probably doesn't explain it well enough maybe I can try in the mentorship open session.

[isabelcosta]

No problem @epicadk we'll discuss in the meeting :)

[epicadk]

Actually this is working just fine. The documentation is not clear enough maybe you only have to send the data you want to change. So say you only want to change location from Perth to Sydney then you would only send
{"location" : "Sydney"} and nothing else. Do we want to change this behavior and expect the user to send all fields or should I keep it like this?

[epicadk]

Un-assigning myself as the required behavior needs to be discussed before changes are made. The endpoint does what is claims to do. If we do expect all the user data to be present then the documentation needs to be changed as well. Adding this to the mentorship system agenda. cc @isabelcosta