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

Fix change password error #3536

Merged

Conversation

hitenvidhani
Copy link
Contributor

@hitenvidhani hitenvidhani commented Apr 12, 2024

Fixes #3528

Screenshots

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the develop branch of the repository
  • My commit messages follow 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.

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
Copy link
Contributor Author

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?

@hitenvidhani hitenvidhani marked this pull request as ready for review April 12, 2024 21:35
@Anish9901
Copy link
Member

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

@hitenvidhani
Copy link
Contributor Author

hitenvidhani commented Apr 15, 2024

#3536 (comment)
Was gonna add after finalizing few things here, should we close it? @Anish9901

@Anish9901
Copy link
Member

Anish9901 commented Apr 15, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

hitenvidhani commented Apr 17, 2024

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

validation works when user tries to change the password.

@hitenvidhani
Copy link
Contributor Author

@Anish9901 we can merge this.

Copy link
Member

@Anish9901 Anish9901 left a comment

Choose a reason for hiding this comment

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

Hey @hitenvidhani, the PR looks good overall, I've requested a couple of changes that need to be addressed before we can merge this in.

mathesar/tests/api/test_user_api.py Outdated Show resolved Hide resolved
mathesar/api/ui/serializers/users.py Outdated Show resolved Hide resolved
mathesar/tests/api/test_user_api.py Show resolved Hide resolved
Copy link
Member

@Anish9901 Anish9901 left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this @hitenvidhani! The PR looks good after the changes, I made a small change(98a3315) that raises the ValidationError directly instead of typecasting the exception to str and then raising it, this way the exception string isn't converted to a list of string of errors.

@Anish9901 Anish9901 added this pull request to the merge queue Apr 22, 2024
Merged via the queue into mathesar-foundation:develop with commit 7944c44 Apr 22, 2024
38 checks passed
@hitenvidhani hitenvidhani deleted the fix_pass_change_error branch April 22, 2024 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when trying to reset password of other user
3 participants