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

forms: Fix accounts listed in password_reset email to active accounts. #10186

Closed
wants to merge 1 commit into from

Conversation

shubhamdhama
Copy link
Member

Previously we were listing both accounts, active as well as non-active.
Fixes: #10130.
screenshot from 2018-08-04 18-00-21
A better alternative for "Kindly, contact the organization administrators." required.

Previously we were listing both accounts, active as well as non-active.
Fixes: zulip#10130.
@zulipbot
Copy link
Member

zulipbot commented Aug 4, 2018

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

@timabbott
Copy link
Sponsor Member

Merged as 2dec30e, after:
(1) Removing the changes to the text; I wasn't sure what should go there. @rishig thoughts? The question is basically whether to mention something about deactivated accounts. I feel like it's kinda clear from how consistently we use the term "active account" in the email.
(2) Bumping PROVISION_VERSION, which one generally needs to do after changing email source templates.

Thinking about this more, though, I'd really like to have a test for this corner case. @shubhamdhama would you be up for extending the existing password reset test class to cover this case?

@timabbott timabbott closed this Aug 4, 2018
@shubhamdhama
Copy link
Member Author

shubhamdhama commented Aug 4, 2018

Bumping PROVISION_VERSION, which one generally needs to do after changing email source templates.

ok... I'll take care of this next time.

@shubhamdhama would you be up for extending the existing password reset test class to cover this case?

Sure, I'll do it.

@rishig
Copy link
Member

rishig commented Aug 4, 2018

I think something like the following?
"Someone (possibly you) requested a password reset for prospero@zulip.com on http://localhost:9991, but your account has been deactivated. You can contact an organization administrator to reactivate your account."

[reactivate your account] can be a link to https://zulipchat.com/help/deactivate-or-reactivate-a-user#reactivate-a-user? (Or possibly we should have a separate reactivate-your-account article if we're going to link to it like this; I can take care of that edit if we decide to do it.)

Also as a note remember to change both the .html.source and the .txt email when changing the email templates.

@timabbott
Copy link
Sponsor Member

Seems reasonable to me. @rishig do you want to do the PR for this?

@rishig
Copy link
Member

rishig commented Aug 13, 2018

This will take some refactoring, but I think it's worth it. @shubhamdhama, would you be up for making the changes, and I can review?

The final state of password_reset.source.html should be the following (note that I renamed some variables for clarity). This will probably be a string of ~10 small commits to get here.

Not sure if this is the best place to store this, or if I should open a new issue with the updated text below.

{% extends "zerver/emails/compiled/email_base_default.html" %}

{% block content %}
{% if active_account_in_realm %}
    <p>
        Psst. Word on the street is that you need a new password, {{ email }}.<br />
        It's all good. Click here and we'll take care of the rest:<br />
        <a class="button" href="{{ reset_url }}">Reset password</a>
    </p>
{% else %}
    <p>
        {% if user_deactivated %}
            Someone (possibly you) requested a password reset email for {{ email }}
            on {{ realm_uri }}, but your account has been deactivated. You can contact
            an organization administrator to reactivate your account.
        {% else %}
            Someone (possibly you) requested a password reset email for {{ email }}
            on {{ realm_uri }}, but you do not have an account in that organization.
        {% endif %}
    </p>
    {% if active_accounts_in_other_realms %}
        <p>
            You do have active accounts in the following organization(s).
            <ul>
                {% for active_account in active_accounts_in_other_realms %}
                <li>{{ active_accounts.realm.uri }}</li>
                {% endfor %}
            </ul>
            You can try logging in or resetting your password in the organization(s)
            above.
        </p>
    {% endif %}
{% endif %}
<p>
    Thanks,<br />
    Your friends at Zulip HQ
</p>
{% endblock %}

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.

None yet

4 participants