Add Fullname Support to User Profiles #2608
Conversation
mslib/mscolab/migrations/versions/922e4d9c94e2_to_version_10_0_0.py
Outdated
Show resolved
Hide resolved
| @@ -56,18 +70,20 @@ def setupUi(self, addUserDialog): | |||
| self.gridLayout.addLayout(self.verticalLayout, 0, 0, 1, 1) | |||
|
|
|||
| self.retranslateUi(addUserDialog) | |||
| self.buttonBox.accepted.connect(addUserDialog.accept) | |||
| self.buttonBox.rejected.connect(addUserDialog.reject) | |||
| self.buttonBox.accepted.connect(addUserDialog.accept) # type: ignore | |||
There was a problem hiding this comment.
why is there a # type: ignore now?
usually this is not in a pyuic5 autogenerated file
There was a problem hiding this comment.
i will check this and revert it.
There was a problem hiding this comment.
Should this have been reverted now or was this created by the conversion process?
There was a problem hiding this comment.
i think this is created by the conversion process, because i don't remember doing it manually, should i remove this?
There was a problem hiding this comment.
it is maybe OS dependent, I can reproduce it.
pyuic5 --version
Python User Interface Compiler 5.15.9 for Qt version 5.15.8
pyuic5 ui_add_user_dialog.ui > ../qt5/ui_add_user_dialog.py
reimarbauer@MBP-von-Reimar ui % grep -i type ../qt5/ui_add_user_dialog.py
self.buttonBox.accepted.connect(addUserDialog.accept) # type: ignore
self.buttonBox.rejected.connect(addUserDialog.reject) # type: ignore
|
try to alter a test that now also the fullname is checked if one was added |
i am sorry, i didn't understand...can you please explain |
|
Where we currenly have Change Avatar, there should be an "Edit" |
|
Is it possible to create a new issue for this change? I would be happy to work on it. |
|
We have now changed the API and the model. The CI runs should cover the changes. The tests are all in your fork too We should have at least a test which covers adding a fullname on user creation |
|
@annapurna-gupta Eric has a good documentation about pytest https://pytest-with-eric.com/ |
Thanks for the pytest resources! I couldn’t work on the issue as my college practicals were going on. |
|
Hi @ReimarBauer , I’ve pushed PR, please review it...meanwhile, I’ll fix the tests. |
There was a problem hiding this comment.
have a look on the signature of add_user and improve it and add tests which use fullname
export QT_QPA_PLATFORM=offscreen
pytest -k test_url_combo
ERROR tests/_test_msui/test_mscolab.py::Test_Mscolab_connect_window::test_url_combo - TypeError: add_user() missing 1 required positional argument: 'fullname'
|
@ReimarBauer can you please review the changes. |
mslib/mscolab/migrations/versions/89cbaa26ae78_to_version_10_0_0.py
Outdated
Show resolved
Hide resolved
Could you please guide me on how to fix the migration script issue? Once that is resolved, I will fix the CI results. |
|
Look into the content of the added file. We we will have only 1 version 10.0.0 release. Your new file is not needed. Move the changes to the existing mig script for version 10.0.0. |
okk |
|
@ReimarBauer could you please look at the changes and the workflow error. |
|
@ReimarBauer could you please take a look in this pr. |
|
Hi @ReimarBauer, i have made the requested changes. |
|
Thx I look later today. |
btw what is your time zone. |
|
@matrss please do a final review :) |
MEZ |
|
@annapurna-gupta please look at the merge conflict and solve it |
done |
mslib/mscolab/server.py
Outdated
| fullname = " ".join(fullname.split()) # Removes extra spaces from fullname | ||
| result_with_regex = slugify(fullname, regex_pattern=r"[^a-zA-Z\s\-]") |
There was a problem hiding this comment.
This seems rather restrictive, my own name would not be allowed according to these rules. Why is the name validated against these rules at all?
There are a lot of falsehoods that people believe about the structure of names: https://www.kalzumeus.com/2010/06/17/falsehoods-programmers-believe-about-names/. In the end those falsehoods boil down to "a software must allow an arbitrary UTF-8 string of arbitrary (or at least sufficient) length as a name to not inadvertently declare a real name as invalid.".
There was a problem hiding this comment.
You're right, I think I shouldn't restrict the full name, I should pass it as it is.
| @@ -56,18 +70,20 @@ def setupUi(self, addUserDialog): | |||
| self.gridLayout.addLayout(self.verticalLayout, 0, 0, 1, 1) | |||
|
|
|||
| self.retranslateUi(addUserDialog) | |||
| self.buttonBox.accepted.connect(addUserDialog.accept) | |||
| self.buttonBox.rejected.connect(addUserDialog.reject) | |||
| self.buttonBox.accepted.connect(addUserDialog.accept) # type: ignore | |||
There was a problem hiding this comment.
Should this have been reverted now or was this created by the conversion process?
…napurna-gupta/MSS into feature/add-fullname-nickname
|
This is maybe a difference on the OS where it is created. At least I can reproduce it on my MBP too. |
|
@matrss could you please review the changes. |
|
Likly you want to ask @matrss or? |
mslib/mscolab/server.py
Outdated
| if fullname == "": | ||
| return {"success": True, "processed_name": fullname} | ||
| if any(char.isalpha() for char in fullname): | ||
| return {"success": True, "processed_name": fullname} | ||
| else: | ||
| return {"success": False, "message": "Full name must contain at least one letter"} |
There was a problem hiding this comment.
What is the rationale for the remaining restrictions?
There was a problem hiding this comment.
What is the rationale for the remaining restrictions?
I thought a full name shouldn't contain only numbers or special characters..i will remove it.
yes..sorry for the confusion.. |
I got you, and @ReimarBauer has already suggested changing the avatar button to an edit button that will allow editing the full name and changing the avatar. I think it would be better to create a separate issue for this, and I will definitely work on it. Since updating the avatar button might take some time, and I need to start working on my proposal, I’ll work on this in the meantime or maybe after the proposal period. |
Fixes #1282