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

Fix of the carrier choice in payment preferences, product editor and cart rules #12379

Merged
merged 12 commits into from Oct 31, 2019

Conversation

@Rolige
Copy link
Contributor

Rolige commented Jan 30, 2019

Questions Answers
Branch? develop
Description? Fix the lack of payment methods when choosing a specific carrier in the FO.
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #11379 #16094
How to test? You only have to create two carriers with the same name, and in the "BO > Payment > Preferences > Carrier restrictions" only ONE will appear, automatically leaving one outside, which will make that when trying to pay in the FO, there will be no payment options available.

This change is Reviewable

@mickaelandrieu

This comment has been minimized.

Copy link
Member

mickaelandrieu commented Jan 31, 2019

Hi @Rolige,

is the delay always an integer?

I understand why you want to fix it this way but I'm worried about the side effects.

Regards

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Jan 31, 2019

As far as I understand, we would only change the label to make sure all carriers would appear here.

capture d ecran du 2019-01-31 12-04-16

Well, if we have to make sure each carrier is properly differentiated from the others, we could add its identifier.
The result would be:

$choices['#' . $carrier['id_reference'] . ' - ' . $carrier['name']] = $carrier['id_reference'];

capture d ecran du 2019-01-31 12-06-04

We need a review from the product team.

@Rolige

This comment has been minimized.

Copy link
Contributor Author

Rolige commented Jan 31, 2019

Hi,

@mickaelandrieu
No, it's usually a text.

@Quetzacoalt91
Include the id_reference, as you comment, would be much more accurate, but it's important include also the "delay" because it's the differentiated with which the administrator user is familiar, as is now done in different sections of PrestaShop, as shown in the following screenshot in the product edition.

image

This is a very critical issue because the selection of payment methods in the FO is completely nulled due to the overwriting generated in the array responsible for generating the list of carriers in the payment preferences.
Continuing with the previous example, I share with you a screenshot of how the transporters are currently shown, as you can see, all those that have the same name now only one appears.

image

Regards!

@Rolige

This comment has been minimized.

Copy link
Contributor Author

Rolige commented Jan 31, 2019

A video can always show the details better, check here please: https://streamable.com/svqgi

Regards!

@antho-girard

This comment has been minimized.

Copy link
Contributor

antho-girard commented Feb 5, 2019

Hello,

Adding the delay is not enough as we may have carriers with the same name and the same delay.
We should at least add the ID in the label as suggested by @Quetzacoalt91. And like @Rolige, I think it would be helpful for merchants to have the delay too.

I also notice we have the same behaviour in the Product form if two carriers or more have the same name and the same delay. Adding the carrier ID there too would be great.

@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Feb 5, 2019

Hello @Rolige

It seems to work fine, just if you can add also the ID of the carrier it will be more clear

Thank you :)

@Rolige Rolige changed the title Fix of payment restriction by carrier Fix of the carrier choice in payment preferences, product editor and cart rules Feb 5, 2019
@Rolige

This comment has been minimized.

Copy link
Contributor Author

Rolige commented Feb 5, 2019

Hi team,

I have updated my patch including the same fix in the payment preferences, product editor and cart rules (Am I forgetting something else?).

Some screenshots of how it would look now...

image
image
image

Regards!

@@ -1483,6 +1483,7 @@ public function getAssociatedRestrictions(
' . (in_array($type, array('carrier', 'shop')) ? ' AND t.deleted = 0' : '') . '
' . ($type == 'cart_rule' ? 'AND t.id_cart_rule != ' . (int) $this->id : '') .
$shop_list .
' GROUP BY t.id_' . $type .

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 Apr 1, 2019

Member

Why is this change needed? I wonder if all the carriers will still be displayed.

This comment has been minimized.

Copy link
@Rolige

Rolige Apr 1, 2019

Author Contributor

Without this, the query return duplicate rows.

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 Apr 18, 2019

Member

As I can't reproduce it, I'd like to keep the previous version.

$shop_list
. (in_array($type, array('carrier', 'shop')) ? ' AND t.deleted = 0' : '') .
$shop_list .
' GROUP BY t.id_' . $type .

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 Apr 1, 2019

Member

Same comment as the previous GROUP BY

This comment has been minimized.

Copy link
@Rolige

Rolige Apr 1, 2019

Author Contributor

Same case, without this, the query return duplicate rows.

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 Apr 18, 2019

Member

Same answer, as I can't reproduce it I'd like to keep the previous version.
We can handle this case in another PR if we finally find the issue.

This comment has been minimized.

Copy link
@Rolige

Rolige Apr 18, 2019

Author Contributor

The problem with this is that the carriers appears duplicated in the “cart rule > carrier restriction”, this does not occurs in a traditional shop, but in my localhost with multi-lang and multi-store if it happens. Anyway I have changed the fix to the carriers loop taking advantage of the fact that a cycle already exists and this way does not affect the original query, but please check this video about what's happen without this patch in the real life: https://streamable.com/qbdv3

@Rolige Rolige requested a review from PrestaShop/prestashop-core-developers as a code owner Apr 18, 2019
Copy link
Member

Quetzacoalt91 left a comment

We still need to apply the the ID and the delay, I remove the surrounding else

@marionf marionf requested a review from Quetzacoalt91 May 23, 2019
@matks matks force-pushed the Rolige:patch-2 branch from a72691d to 5e7e682 Sep 26, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Sep 26, 2019

Hi @Rolige sorry for the late feedback

I rebased the PR to fix git conflicts and restart CI. I also added 5c475f2 to handle usecases where no delay has been provided for carriers.

I'll need one approval from another maintainer to get this merged, but to me it looks good 😄
@PrestaShop/prestashop-core-developers ?

@matks matks dismissed Quetzacoalt91’s stale review Sep 26, 2019

Requested changes has been applied

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Sep 26, 2019

I'm still puzzled by this crazy behavior in https://github.com/PrestaShop/PrestaShop/pull/12379/files#diff-bf34b8ed5c52c8da93b3424488bd5047L644 🤔 why is it needed

Also this page is being migrated to Symfony so once it's merged we'll need to re-apply to the Symfony part (I'll do it)

@matks matks mentioned this pull request Sep 26, 2019
0 of 32 tasks complete
@Rolige

This comment has been minimized.

Copy link
Contributor Author

Rolige commented Sep 26, 2019

I'm still puzzled by this crazy behavior in https://github.com/PrestaShop/PrestaShop/pull/12379/files#diff-bf34b8ed5c52c8da93b3424488bd5047L644 🤔 why is it needed

Also this page is being migrated to Symfony so once it's merged we'll need to re-apply to the Symfony part (I'll do it)

Hi @matks This is needed for the CartRules

@Rolige

This comment has been minimized.

Copy link
Contributor Author

Rolige commented Sep 26, 2019

@matks In fact, in the recent days I've noticed that a patch is also needed in the carrier preferences, where you can select by best price, best grade or a specific carrier.

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Sep 26, 2019

I'm still puzzled by this crazy behavior in https://github.com/PrestaShop/PrestaShop/pull/12379/files#diff-bf34b8ed5c52c8da93b3424488bd5047L644 🤔 why is it needed
Also this page is being migrated to Symfony so once it's merged we'll need to re-apply to the Symfony part (I'll do it)

Hi @matks This is needed for the CartRules

What I mean is this strange double foreach, it seems needed but I dont get the full intent 😅 but the code works

I see we got a from Travis but a 🔴from PrettyCI, I fix that

@Rolige

This comment has been minimized.

Copy link
Contributor Author

Rolige commented Sep 26, 2019

I'm still puzzled by this crazy behavior in https://github.com/PrestaShop/PrestaShop/pull/12379/files#diff-bf34b8ed5c52c8da93b3424488bd5047L644 🤔 why is it needed
Also this page is being migrated to Symfony so once it's merged we'll need to re-apply to the Symfony part (I'll do it)

Hi @matks This is needed for the CartRules

What I mean is this strange double foreach, it seems needed but I dont get the full intent 😅 but the code works

I see we got a from Travis but a 🔴from PrettyCI, I fix that

Without it you will have a duplicated results, of course, you need to have more than one language in your installed languages.

Please consider to take a look at my previous comment, this is a related issue also, just I don't have time to review it now.

@matks
matks approved these changes Oct 30, 2019
@sarahdib sarahdib added QA ✔️ and removed waiting for QA labels Oct 31, 2019
@sarahdib sarahdib added this to the 1.7.7.0 milestone Oct 31, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 31, 2019

Thank you @Rolige

@matks matks merged commit 05a5147 into PrestaShop:develop Oct 31, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@jf-viguier

This comment has been minimized.

Copy link
Contributor

jf-viguier commented Oct 31, 2019

Thanks @Rolige

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.