-
Notifications
You must be signed in to change notification settings - Fork 44
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
212 password change #254
212 password change #254
Conversation
@dethos How do you think the new password form should behave for users that registered using GithubOauth? (there is no current password for these cases). |
{{ form.current_password.errors }} {{ form.current_password.label_tag }} {{ form.current_password }} | ||
</div> | ||
<div class="form__block"> | ||
{{ form.new_password2.errors }} {{ form.new_password1.label_tag }} {{ form.new_password1 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a typo, should be {{ form.new_password1.errors }}
I did it on purpose to always display the same message to both of them!
…On Oct 3, 2017 16:00, "Nuno Silva" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In humans/templates/humans/update_user_form.html
<#254 (comment)>:
> @@ -65,6 +66,19 @@
</div>
</li>
<li class="section" id="section3">
+ <div class="col-xxs-12 col-xs-12 col-md-12 start-xs">
+ <div class="form__block">
+ {{ form.current_password.errors }} {{ form.current_password.label_tag }} {{ form.current_password }}
+ </div>
+ <div class="form__block">
+ {{ form.new_password2.errors }} {{ form.new_password1.label_tag }} {{ form.new_password1 }}
I think this is a typo, should be {{ form.new_password1.errors }}
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#254 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA7IXlcWpOnpyF9sYQfv-dilnDERQgXkks5sooRXgaJpZM4PsRfq>
.
|
@Lobosque for users who signed up using Github, we should not display the change password tab or to make it clear, instead of the form a message could be displayed saying that since they used Oauth there is not need to update passwords. By the way, thanks for your contribution. As soon as you hide the tab or display that message, I will make a full review and merge the PR (on a first look everything appears to be fine 👍 ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After testing a bit I found a small issue.
Also #tab4
is missing here: https://github.com/whitesmith/hawkpost/pull/254/files#diff-634d4c9620895e0cdf312c07c3ad90b6R49
$("#tab3").addClass("active"); | ||
$("#tab1").removeClass("active"); | ||
$("#tab2").removeClass("active"); | ||
$(".sett__nav__item").click(function(evt) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason this event is not being added to #tab4
(at least on Firefox)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested here on Firefox and it triggers the event just fine.
Maybe a version issue? Can you try again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested again, with different browsers and it is working fine. Not sure what was the problem the other time.
53c3e51
to
88df73f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dethos I added a commit to hide the change password section from github signups.
However I couldn't reproduce the tab problem. All of them worked just fine in both Chrome and Firefox.
$("#tab3").addClass("active"); | ||
$("#tab1").removeClass("active"); | ||
$("#tab2").removeClass("active"); | ||
$(".sett__nav__item").click(function(evt) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested here on Firefox and it triggers the event just fine.
Maybe a version issue? Can you try again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small observation
humans/forms.py
Outdated
self.pub_key = None | ||
return super().__init__(*args, **kwargs) | ||
return super(UpdateUserInfoForm, self).__init__(*args, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? This is a Python 3 project, there is no need to call super using the old "style".
humans/forms.py
Outdated
new_password = self.cleaned_data.get('new_password2') | ||
if self.change_password: | ||
self.instance.set_password(new_password) | ||
return super(UpdateUserInfoForm, self).save(commit=commit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here super can also be changed to the new style
Fixes #212
UpdateUserInfoForm
to accept password change;