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

Fixed bug where invalid address message is displayed only for billing address even if both are invalid #8435

Merged
merged 5 commits into from Nov 2, 2017

Conversation

Projects
None yet
6 participants
@LittleBigDev
Contributor

LittleBigDev commented Oct 24, 2017

Questions Answers
Branch? develop
Description? At first loading of checkout page, if different addresses are selected for billing and shipping, and both addresses are invalid, a warning message should be displayed for both addresses. But only billing has its warning message displayed.
Type? bug fix
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? BOOM-4085
How to test? See screen record in ticket

This change is Reviewable

@PrestaShop PrestaShop deleted a comment from prestonBot Oct 24, 2017

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 24, 2017

Member

Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


classes/checkout/CheckoutAddressesStep.php, line 277 at r1 (raw file):

        foreach ($errors as $errorKey => $errorMessage) {
            $params[$errorKey] = $errorMessage;
        }

You can use array_replace($params, $errors) instead


Comments from Reviewable

Member

eternoendless commented Oct 24, 2017

Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


classes/checkout/CheckoutAddressesStep.php, line 277 at r1 (raw file):

        foreach ($errors as $errorKey => $errorMessage) {
            $params[$errorKey] = $errorMessage;
        }

You can use array_replace($params, $errors) instead


Comments from Reviewable

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Oct 24, 2017

Contributor

Review status: 4 of 5 files reviewed at latest revision, 1 unresolved discussion.


classes/checkout/CheckoutAddressesStep.php, line 277 at r1 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

You can use array_replace($params, $errors) instead

Done.


Comments from Reviewable

Contributor

LittleBigDev commented Oct 24, 2017

Review status: 4 of 5 files reviewed at latest revision, 1 unresolved discussion.


classes/checkout/CheckoutAddressesStep.php, line 277 at r1 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

You can use array_replace($params, $errors) instead

Done.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 24, 2017

Member

:lgtm:


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Member

eternoendless commented Oct 24, 2017

:lgtm:


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@eternoendless eternoendless added this to the 1.7.3.0 milestone Oct 24, 2017

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Oct 25, 2017

Contributor

Hello @LittleBigDev
Please see screenrecord24 on the BOOM-4085 ticket.

Contributor

marionf commented Oct 25, 2017

Hello @LittleBigDev
Please see screenrecord24 on the BOOM-4085 ticket.

LittleBigDev added some commits Oct 25, 2017

@LittleBigDev LittleBigDev removed the WIP label Oct 25, 2017

@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Oct 25, 2017

Codacy Here is an overview of what got changed by this pull request:

Complexity decreasing per file
==============================
+ themes/core.js  -2
+ themes/_core/js/checkout-address.js  -2
         

See the complete overview on Codacy

codacy-bot commented Oct 25, 2017

Codacy Here is an overview of what got changed by this pull request:

Complexity decreasing per file
==============================
+ themes/core.js  -2
+ themes/_core/js/checkout-address.js  -2
         

See the complete overview on Codacy

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Oct 26, 2017

Contributor

Hello @LittleBigDev
I am sorry but I can't reproduce anymore on develop branch the issue described in the ticket and @vincentbz neither.
So, is this PR is really necessary ?

Contributor

marionf commented Oct 26, 2017

Hello @LittleBigDev
I am sorry but I can't reproduce anymore on develop branch the issue described in the ticket and @vincentbz neither.
So, is this PR is really necessary ?

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Nov 2, 2017

Contributor

Yes it is. It's a very specific scenario, few chances to happen in real life. But code definitely needed to be strengthened. It's done.

Contributor

LittleBigDev commented Nov 2, 2017

Yes it is. It's a very specific scenario, few chances to happen in real life. But code definitely needed to be strengthened. It's done.

@marionf marionf added QA ✔️ and removed waiting for author labels Nov 2, 2017

@Quetzacoalt91 Quetzacoalt91 merged commit aeba453 into PrestaShop:develop Nov 2, 2017

2 of 3 checks passed

code-review/reviewable 4 files left (eternoendless)
Details
codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@LittleBigDev LittleBigDev deleted the LittleBigDev:BOOM-2989 branch Nov 7, 2017

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