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

Bugfix: Change password for current user #2244

Merged
merged 13 commits into from
Aug 30, 2024
Merged

Conversation

NguyenThuyLan
Copy link

@NguyenThuyLan NguyenThuyLan commented Aug 28, 2024

Description

This PR fix bug can not change the current user password from this issue #16943

  1. Can not change password when click "Change password" by clicking my user-icon in the top right corner
  2. Got error when click change password on User section

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Motivation and context

How to test?

Screenshots (if appropriate)

Checklist

  • If my change requires a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@NguyenThuyLan NguyenThuyLan requested review from nielslyngsoe and iOvergaard and removed request for iOvergaard August 28, 2024 07:07
Copy link
Contributor

@madsrasmussen madsrasmussen left a comment

Choose a reason for hiding this comment

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

Hi @NguyenThuyLan

In the client code we have separate modules for "a user" and "the current user". I think these changes would fit better into the "current user module" and could possibly simplify the code a bit.

You can find the data source here:
https://github.com/umbraco/Umbraco.CMS.Backoffice/blob/main/src/packages/user/current-user/repository/current-user.server.data-source.ts

Then you can add a method to the "current user"-repository to change the password here:
https://github.com/umbraco/Umbraco.CMS.Backoffice/blob/main/src/packages/user/current-user/repository/current-user.repository.ts

@NguyenThuyLan
Copy link
Author

Hi @madsrasmussen
yes, but I couldn't avoid to change file change-user-password.repository.ts with adding OldPassword and IsCurrentUser params here, is that ok? Because when a user is a current user click to button "Change the password" at User Section, it will hit change-user-password.action.ts

@madsrasmussen
Copy link
Contributor

madsrasmussen commented Aug 28, 2024

I did a bit of digging, and I can see you are right that we use the same modal for both a user and the current user which results in the "wrong" endpoint being called.

I would still like us to move your changes in the data source and repository as mentioned in the previous comment. Then we also have to change the repository depending on the type of user.

The repository is set up in the entity action code here:
https://github.com/umbraco/Umbraco.CMS.Backoffice/blob/main/src/packages/user/user/entity-actions/change-password/change-user-password.action.ts#L26

You can make a check to see if the user unique matches the current user and then spin up the UmbCurrentUserRepository instead. You can see how we do something similar here:
https://github.com/umbraco/Umbraco.CMS.Backoffice/blob/main/src/packages/user/modals/change-password/change-password-modal.element.ts#L67

Feel free to reach out on Slack if it doesn't make any sense :D

Copy link

sonarcloud bot commented Aug 29, 2024

@madsrasmussen madsrasmussen changed the title Fix bug change password for current user Bugfix: Change password for current user Aug 29, 2024
@iOvergaard iOvergaard merged commit fc1f80e into main Aug 30, 2024
8 checks passed
@iOvergaard iOvergaard deleted the 16943_Fix-change-password branch August 30, 2024 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants