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

Display errors on change password (lost) #8331

Merged
merged 1 commit into from Sep 20, 2017

Conversation

Projects
None yet
3 participants
@prestarocket
Contributor

prestarocket commented Sep 14, 2017

Questions Answers
Branch? develop
Description? When user submit a new passwod (reset password), if there is an error (ex: less than 5 characters), no errors are displayed
Type? bug fix
Category? FO
BC breaks? no
Deprecations? no
How to test? ask reset password, clink link in your email=>fill the form new password with an error (ex: less than 5 char.)=>submit

This change is Reviewable

FO: display errors on change password (lost)
When user submit a new passwod (reset password), if there is an error (ex: less than 5 characters), no errors are displayed

@vincentbz vincentbz added this to the 1.7.3.0 milestone Sep 14, 2017

@vincentbz

This comment has been minimized.

Show comment
Hide comment
@vincentbz

vincentbz Sep 14, 2017

Contributor

Hi @prestarocket

Thanks for that PR !
I now can see the error message.

But I have to see if there are several error messages, one for each error type, or a global one. We have to adapt it.

image

Contributor

vincentbz commented Sep 14, 2017

Hi @prestarocket

Thanks for that PR !
I now can see the error message.

But I have to see if there are several error messages, one for each error type, or a global one. We have to adapt it.

image

</svg>
</i>
<p>{$error}</p>
</li>

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Sep 20, 2017

Member

Although we all like the design of the alert panel, it is not supposed to be implemented like this. This would be the only place we can see it.

Can you just please reuse the alert alert-danger classes as done in notifications.tpl? All the integration must be done elsewhere.

{if $notifications.error}
{block name='notifications_error'}
<article class="alert alert-danger" role="alert" data-alert="danger">
<ul>
{foreach $notifications.error as $notif}
<li>{$notif nofilter}</li>
{/foreach}
</ul>
</article>
{/block}
{/if}

@Quetzacoalt91

Quetzacoalt91 Sep 20, 2017

Member

Although we all like the design of the alert panel, it is not supposed to be implemented like this. This would be the only place we can see it.

Can you just please reuse the alert alert-danger classes as done in notifications.tpl? All the integration must be done elsewhere.

{if $notifications.error}
{block name='notifications_error'}
<article class="alert alert-danger" role="alert" data-alert="danger">
<ul>
{foreach $notifications.error as $notif}
<li>{$notif nofilter}</li>
{/foreach}
</ul>
</article>
{/block}
{/if}

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Sep 20, 2017

Member

Okay, I see that you reused the example in password-email.tpl, password-infos.tpl etc.

We made a mistake by not making this simple, and this needs to be changed. Because this is outside the perimeter of this PR, I approve it.

@Quetzacoalt91

Quetzacoalt91 Sep 20, 2017

Member

Okay, I see that you reused the example in password-email.tpl, password-infos.tpl etc.

We made a mistake by not making this simple, and this needs to be changed. Because this is outside the perimeter of this PR, I approve it.

@Quetzacoalt91 Quetzacoalt91 merged commit 4f73184 into PrestaShop:develop Sep 20, 2017

2 checks passed

codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91
Member

Quetzacoalt91 commented Sep 20, 2017

Thank you @prestarocket

@prestarocket

This comment has been minimized.

Show comment
Hide comment
@prestarocket

prestarocket Sep 20, 2017

Contributor

Thank you @Quetzacoalt91 !!

Contributor

prestarocket commented Sep 20, 2017

Thank you @Quetzacoalt91 !!

@eternoendless eternoendless changed the title from FO: display errors on change password (lost) to Display errors on change password (lost) Dec 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment