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.

Sign up for GitHub

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

Change: display user's password reset link to the admin when mail server disabled #1371

Merged
merged 4 commits into from
Nov 20, 2016
Merged

Change: display user's password reset link to the admin when mail server disabled #1371

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
72389b0
Display user's password reset link to the admin
vitorbaptista Nov 1, 2016
69c1f15
Wrap "Password Reset Button" in form group to visually separate it
vitorbaptista Nov 2, 2016
438ebb9
Fix condition to show forgotten password link and alter its message
vitorbaptista Nov 7, 2016
3ca78be
Fix conditions to show the user reset link
vitorbaptista Nov 16, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions rd_ui/app/scripts/controllers/users.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,9 +302,9 @@

$scope.sendPasswordReset = function() {
$scope.disablePasswordResetButton = true;
$http.post('api/users/' + $scope.user.id + '/reset_password').success(function(user) {
$http.post('api/users/' + $scope.user.id + '/reset_password').success(function(data) {
$scope.disablePasswordResetButton = false;
growl.addSuccessMessage("The user should receive a link to reset his password by email soon.")
$scope.passwordResetLink = data.reset_link;
});
};
};
Expand Down
18 changes: 15 additions & 3 deletions rd_ui/app/views/users/show.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,21 @@ <h3 class="p-l-5">{{user.name}}</h3>
</p>
<div ng-if="currentUser.isAdmin">
<hr/>
<button class="btn btn-default" ng-click="sendPasswordReset()" ng-disabled="disablePasswordResetButton">Send
Password Reset Email
</button>
<div class="form-group">
<button class="btn btn-default" ng-click="sendPasswordReset()" ng-disabled="disablePasswordResetButton">Send
Password Reset Email
</button>
</div>

<div ng-if="passwordResetLink && !disablePasswordResetButton" class="alert alert-success">
<p>
Copy link
Member

Choose a reason for hiding this comment

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

Let's add here ng-if="clientConfig.mailSettingsMissing".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So then I should remove the ng-if from the "You don't have a mail server configured" and always show that message?

Copy link
Member

Choose a reason for hiding this comment

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

No. We show one message if there is no mail server configured and the other otherwise. But I just noticed that you have the ng-if backwards: it should be clientConfig.mailSettingsMissing for your message and ! clientConfig.mailSettingsMissing for the original one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see. Just changed it. To be honest, as a user, I'd prefer to have the link shown regardless if I have an e-mail server configured or not, similar to the user invites. I find them useful even when the user will receive an e-mail, and it also gives admins the ability to change a user's password herself. Happy to go with whatever you think is best, though.

Just to let you know, I'm adding new commits and not changing the original ones just because I think it's easier to review this way, but I expect that you (or me) will squash them before merging.

<strong>The user should receive a link to reset his password by email soon.</strong>
</p>
<p ng-if="clientConfig.mailSettingsMissing">
You can use the following link to reset the password yourself:<br/>
{{passwordResetLink}}
</p>
Copy link
Member

Choose a reason for hiding this comment

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

  1. Let's show the original message ("should receive by email...") only if email is enabled (ng-if="!clientConfig.mailSettingsMissing").
  2. Change the text here to: You don't have mail server configured, please send the following link to {{user.name}} to reset their password:.
  3. To have the reset link as an <a> tag: <a ng-href="passwordResetLink">{{paswordResetLink}}</a>. IIRC, it's a full link so the base href doesn't matter.

</div>
</div>
</div>
<div ng-show="selectedTab == 'apiKey'" class="p-10">
Expand Down
4 changes: 4 additions & 0 deletions redash/handlers/users.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ def post(self, user_id):
user = models.User.get_by_id_and_org(user_id, self.current_org)
reset_link = send_password_reset_email(user)

return {
'reset_link': reset_link,
}


class UserResource(BaseResource):
def get(self, user_id):
Expand Down