user settings: directly send password reset email #11517

ss62171 · 2019-02-10T19:56:05Z

This sends a password reset link to the user's current email
instead of redirecting them to another page.
An option to resend the link is also added.

Fixes: #6342

GIFs or Screenshots:

zulipbot · 2019-02-10T19:56:16Z

Hello @zulip/server-settings members, this pull request was labeled with the "area: settings (user)" label, so you may want to check it out!

ss62171 · 2019-02-10T20:53:32Z

This is open for review :)

rishig · 2019-02-11T03:29:36Z

Thanks! One more place the password thing appears is under Show/Change your API key.

Also a quick change I would make is to put "Check your email" and "Resend" on the same line, maybe like "Check your email (resend)", or "Check your email | Resend email".

The "Check your email" can be in green; the same color we use for other success messages.

ss62171 · 2019-02-11T20:04:49Z

@rishig is this fine now?

rishig · 2019-02-11T20:39:34Z

could you send another screencast of the interaction in settings? The first one you sent was very helpful, thanks :).

ss62171 · 2019-02-11T21:22:08Z

@rishig here it is :)

ss62171 · 2019-02-11T21:23:03Z

Actually quality of this gif is not good due to its size.

rishig · 2019-02-11T21:34:11Z

Great, thanks! I do think it should be "Check your email (Resend)" rather than the other way around, but I'll let Tim decide that when merging.

There's a minor issue of there being a flash of something when you click "Reset password" in the API section; do you know what that is?

ss62171 · 2019-02-11T22:08:00Z

It's just text saying "sending..."

timabbott · 2019-02-11T22:29:11Z

@shubhamdhama @pragatiagrawal31 can you help review this?

rishig · 2019-02-11T22:38:40Z

Got it. Let's just use the same inline loading indicator here as well.

ss62171 · 2019-02-11T22:48:23Z

any other changes needed??

pragatiagrawal31 · 2019-02-12T16:37:52Z

static/templates/settings/account-settings.handlebars

+                                <a class="sea-green" reset_password_container id="reset_password" style="display: none">{{t "Reset password" }}</a>
+                                <div class="loading_indicator_spinner"></div>
+                                <a class="sea-green reset_password_container" id="resend_link">{{t "Resend" }}</a>
+                                <span id="email_text" style="display: none">{{t "(Check your email)" }}</span>


I was being told that we don't use inline CSS in templates, so here, instead, you can use Bootstrap classes .show & .hide for controlling the display.

pragatiagrawal31 · 2019-02-12T16:39:17Z

static/templates/settings/account-settings.handlebars

+                <a class="sea-green" id="api_forgot_password" target="_blank">{{t "Never had one? Forgotten it?" }}</a>
+                <a class="sea-green reset_password_container" id="api_reset_password" style="display: none">{{t "Reset password" }}</a>
+                <a class="sea-green reset_password_container" id="api_resend_link">{{t "Resend" }}</a>
+                <span id="api_email_text" style="display: none">{{t "(Check your email)" }}</span>


Here too we can use Bootstrap class .hide.

Also, I can see a lot of repetition of code here, so can you please try to de-duplicate some code.

pragatiagrawal31 · 2019-02-12T16:41:18Z

static/js/settings_account.js

@@ -209,16 +209,64 @@ exports.set_up = function () {
    $("#show_api_key_box").hide();
    $("#api_key_button_box").show();

+    function show_reset_email_loader() {
+        loading.make_indicator($(".loading_indicator_spinner"), {text: 'Sending...'});


This should be marked for translation. You should use the internationalization library i18n as i18n.t("Sending...")

Yep strings marked with i18n.t function is marked for translation.

pragatiagrawal31 · 2019-02-12T17:45:23Z

I've tested this locally and it works fine.

shubhamdhama · 2019-02-12T18:10:02Z

static/styles/settings.scss

+    margin-top: 0;
+}
+
+#account-settings #change_password_modal .reset_password_container {


Is this necessary? Use .hide instead!!

shubhamdhama · 2019-02-12T18:11:51Z

static/js/settings_account.js

+        $('#api_forgot_password').show();
+        $('#api_reset_password').hide();
+        $('#api_resend_link').hide();
+        $('#api_email_text').hide();


I'm always not sure about using hide and show on many selectors, it would be great if possible we can use some class and apply hide and show on that.

I tried using class but it didn't work out

Can you please elaborate on how the class didn't work out, I mean what's the problem with that approach, and if doesn't work out it's fine too.

@shubhamdhama can I use seperate class for every selector?

This sends a password reset link to the user's current email instead of redirecting them to another page. An option to resend the link is also added. Fixes: zulip#6342 Fixes error of lint user_settinngs: Directly send password reset mail. user_settings: Fixes lint error. Fix inline CSS Fix for translation Use hide instead of "display: none"

rishig · 2019-02-14T21:27:50Z

static/js/settings_account.js

@@ -209,16 +209,64 @@ exports.set_up = function () {
    $("#show_api_key_box").hide();
    $("#api_key_button_box").show();

+    function show_reset_email_loader() {
+        loading.make_indicator($(".loading_indicator_spinner"), {text: i18n.t("Sending...")});


I think we shouldn't use Sending .., and just use the loading indicator.

shubhamdhama · 2019-03-01T13:37:17Z

static/templates/settings/account-settings.handlebars

+                <a class="sea-green" id="api_forgot_password" target="_blank">{{t "Never had one? Forgotten it?" }}</a>
+                <a class="sea-green reset_password_container" id="api_reset_password" style="display: none">{{t "Reset password" }}</a>
+                <a class="sea-green reset_password_container" id="api_resend_link">{{t "Resend" }}</a>
+                <span id="api_email_text">{{t "(Check your email)" }}</span>


Regarding my remove duplicate comment, you can try using partials otherwise left it, for now, Tim will tell whether it's worth to use partials here or not.

zulipbot · 2019-06-25T05:19:52Z

Heads up @ss62171, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

zulipbot added size: L area: settings (user) enhancement labels Feb 10, 2019

ss62171 force-pushed the password_reset branch 2 times, most recently from 48fe402 to 206bf63 Compare February 10, 2019 20:38

ss62171 force-pushed the password_reset branch from 206bf63 to 2cd2398 Compare February 11, 2019 19:53

pragatiagrawal31 reviewed Feb 12, 2019

View reviewed changes

shubhamdhama reviewed Feb 12, 2019

View reviewed changes

ss62171 force-pushed the password_reset branch from 2cd2398 to b8e80ba Compare February 12, 2019 21:45

rishig reviewed Feb 14, 2019

View reviewed changes

shubhamdhama reviewed Mar 1, 2019

View reviewed changes

zulipbot added the has conflicts label Jun 25, 2019

vaibhavshn mentioned this pull request Mar 31, 2020

[WIP] Changed 'forgotten password' to not ask for an email #14394

Closed

timabbott removed the enhancement label Jul 24, 2020

timabbott force-pushed the main branch 2 times, most recently from 4ec3636 to 88b200c Compare August 18, 2023 23:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

user settings: directly send password reset email #11517

user settings: directly send password reset email #11517

ss62171 commented Feb 10, 2019

zulipbot commented Feb 10, 2019

ss62171 commented Feb 10, 2019

rishig commented Feb 11, 2019 •

edited

ss62171 commented Feb 11, 2019

rishig commented Feb 11, 2019

ss62171 commented Feb 11, 2019

ss62171 commented Feb 11, 2019

rishig commented Feb 11, 2019

ss62171 commented Feb 11, 2019

timabbott commented Feb 11, 2019

rishig commented Feb 11, 2019

ss62171 commented Feb 11, 2019 •

edited

pragatiagrawal31 Feb 12, 2019

pragatiagrawal31 Feb 12, 2019

shubhamdhama Feb 12, 2019

pragatiagrawal31 Feb 12, 2019 •

edited

shubhamdhama Feb 12, 2019

pragatiagrawal31 commented Feb 12, 2019

shubhamdhama Feb 12, 2019

shubhamdhama Feb 12, 2019

ss62171 Feb 12, 2019

shubhamdhama Mar 1, 2019

ss62171 Mar 4, 2019 •

edited

rishig Feb 14, 2019

shubhamdhama Mar 1, 2019

zulipbot commented Jun 25, 2019

user settings: directly send password reset email #11517

Are you sure you want to change the base?

user settings: directly send password reset email #11517

Conversation

ss62171 commented Feb 10, 2019

zulipbot commented Feb 10, 2019

ss62171 commented Feb 10, 2019

rishig commented Feb 11, 2019 • edited

ss62171 commented Feb 11, 2019

rishig commented Feb 11, 2019

ss62171 commented Feb 11, 2019

ss62171 commented Feb 11, 2019

rishig commented Feb 11, 2019

ss62171 commented Feb 11, 2019

timabbott commented Feb 11, 2019

rishig commented Feb 11, 2019

ss62171 commented Feb 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pragatiagrawal31 Feb 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pragatiagrawal31 commented Feb 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ss62171 Mar 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zulipbot commented Jun 25, 2019

rishig commented Feb 11, 2019 •

edited

ss62171 commented Feb 11, 2019 •

edited

pragatiagrawal31 Feb 12, 2019 •

edited

ss62171 Mar 4, 2019 •

edited