mathesar-foundation / mathesar

Web application providing an intuitive user experience to databases.
https://mathesar.org/
GNU General Public License v3.0
2.36k stars 327 forks source link

Fix change password error #3536

Closed hitenvidhani closed 5 months ago

hitenvidhani commented 5 months ago

Fixes #3528

Screenshots

Checklist

- [x] My pull request has a descriptive title (not a vague title like `Update index.md`). - [x] My pull request targets the `develop` branch of the repository - [x] My commit messages follow [best practices][best_practices]. - [ ] My code follows the established code style of the repository. - [ ] I added tests for the changes I made (if applicable). - [ ] I added or updated documentation (if applicable). - [ ] I tried running the project locally and verified that there are no visible errors. [best_practices]:https://gist.github.com/robertpainsi/b632364184e70900af4ab688decf6f53 ## Developer Certificate of Origin
Developer Certificate of Origin ``` Developer Certificate of Origin Version 1.1 Copyright (C) 2004, 2006 The Linux Foundation and its contributors. 1 Letterman Drive Suite D4700 San Francisco, CA, 94129 Everyone is permitted to copy and distribute verbatim copies of this license document, but changing it is not allowed. Developer's Certificate of Origin 1.1 By making a contribution to this project, I certify that: (a) The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or (b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or (c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it. (d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved. ```
hitenvidhani commented 5 months ago

Do we want to display these messages on front-end? image

This may not be compliant with out multilingual-ui, and we can display a generalized message every time. Let me know what you think.

image

This is on the current experience where we are displaying validation errors. But we do not validate password currently while creating the user, do we want to add that?

Anish9901 commented 5 months ago

@hitenvidhani there is a backend test related to password reset that is failing, can you fix that?

hitenvidhani commented 5 months ago

https://github.com/mathesar-foundation/mathesar/pull/3536#issuecomment-2052572747 Was gonna add after finalizing few things here, should we close it? @Anish9901

Anish9901 commented 5 months ago

We do not validate password currently while creating the user, do we want to add that?

This is by design @hitenvidhani, an admin sets up a generic password for a user which is then changed by the user on their first login and that's when we perform validation.

@seancolsen can you weigh in on Hiten's query about the UI? I also see that the alignment of the info icon is broken in the second image we might want to fix that as it looks like a regression. I noticed that there is validation for when an admin resets the password for an already created users, which I think should be removed to be consistent with the flow of user creation.

seancolsen commented 5 months ago

There seem to be potentially a few questions in this thread regarding UI... i18n of the error message, display of error message, formatting of icon, etc... I consider all these to be out of scope for this PR and also not important enough to bother with tracking or fixing at this point. At the very least, definitely ignore all those UI issues within this PR. Probably ignore them entirely.

But @hitenvidhani if you feel motivated to chase any of those UI issues down, then you could open a new issue where we could discuss it. For example you could try putting Mathesar into Japanese mode and demonstrate an English error message about a user's password. That would be a compelling thing to work on. You could take that up if you like. Just not in this PR.

Good eye, by the way, thinking about that i18n issue!

hitenvidhani commented 5 months ago

as per design, not implementing validation while resetting password(admin).

validation works when user tries to change the password.

hitenvidhani commented 5 months ago

@Anish9901 we can merge this.