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 product features #10956

Merged
merged 6 commits into from Nov 9, 2018

Conversation

Projects
None yet
8 participants
@PierreRambaud
Contributor

PierreRambaud commented Oct 10, 2018

Questions Answers
Branch? 1.7.5.x
Description? Can't get feature data if you don't have token in url & Can't save duplicate feature data.
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #10757
How to test? Follow ticket instructions, but can you test all the Product page ?

This change is Reviewable

@PierreRambaud PierreRambaud added this to the 1.7.5.0 milestone Oct 10, 2018

@@ -296,6 +297,20 @@ public function buildForm(FormBuilderInterface $builder, array $options)
$event->setData($data);
}
if (isset($data['features'])) {

This comment has been minimized.

@matks

matks Oct 12, 2018

Contributor

I understand that you are making sure there is no duplicates in $data['features'] however I fail to understand the goal behind it. Maybe an helpful comment would help here ? 😄

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 15, 2018

Contributor

Done

@PierreRambaud PierreRambaud force-pushed the PierreRambaud:fix-product-features branch from 4688178 to cd301e7 Oct 15, 2018

@@ -466,6 +466,7 @@ public function formAction($id, Request $request)
if ($form->isValid()) {
//define POST values for keeping legacy adminController skills
$_POST = $modelMapper->getModelData($formData, $isMultiShopContext) + $_POST;
$_POST['form'] = $formData;

This comment has been minimized.

@kpodemski

kpodemski Oct 16, 2018

Contributor

This is very interesting line, is it related to all the problems with not working product saving in some scenarios? How did you catch it?

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 16, 2018

Contributor

It's maybe related to some scenarios, but not all of them 😄
By editing the ProductInformation Form, I found that my changes were not catch in the legacy controller (don't understand why they not fully migrate this page :/) and step by step I realized that the $_POST needs to be filled.

@marionf

This comment has been minimized.

Contributor

marionf commented Oct 18, 2018

@PierreRambaud

  1. Whathever the feature I choose, I have always the same predefined values in the drop down

capture d ecran_465

capture d ecran_466

  1. If I try to save a feature with a predefined value that is not corresponding to this feature, I have this error

capture d ecran_467

I add 3 values to the same feature:
Frame Size: Coton (predefined)
Frame size: test (custom)
Frame size: Elasthanne (predefined)
The 3 values are displayed in FO: OK

I delete Frame size: Elasthanne and save
The 2 values (Frame Size: coton & Frame size: test) are displayed in FO: OK

I add again Frame size: Elasthanne and save
Only 2 values are displayed in FO: Frame size coton & frame size Elasthanne), the custom value "test" has disappeared

I add some Feature & values:
Frame size: coton (predefined)
Frame size: elasthanne (predefined)
Color: test (custom)
Size: test2 (custom)

The 4 features & values are displayed in FO: OK

I change the tax rule of the product & save

In FO I have well the 4 features but only 2 values:
Frame size: coton
Frame size: elasthanne
Color:
Size:
The 2 custom values have disappeared

I think scenario 3 and 4 are the same issue: if I change something on the product page and save, custom values are removed

PierreRambaud added some commits Oct 10, 2018

Fix product features
- Can't get feature data if you don't have token in url
- Can't save due tu duplicate feature data
@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Oct 18, 2018

@marionf Should be ok now :)

@PierreRambaud PierreRambaud force-pushed the PierreRambaud:fix-product-features branch from cd301e7 to 229b023 Oct 18, 2018

@marionf marionf self-assigned this Oct 19, 2018

@marionf

This comment has been minimized.

Contributor

marionf commented Oct 19, 2018

@PierreRambaud
I still have custom values that disappeared
I add:
Frame size: Coton
Frame size: test
Paper type: Quadrillé
Save -> OK
Change Paper type for "Ligné" and custom value "test" disappear

@marionf

This comment has been minimized.

Contributor

marionf commented Oct 22, 2018

@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Oct 31, 2018

This seems to be caused by the count of existing features when adding a new line. If you previously removed a feature (not the last one), adding another feature will get exactly the same name as an existing line.

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Nov 7, 2018

@marionf @Quetzacoalt91 Yeah, you found a new bug 👍 (on custom fields too)
Everything should be ok now (I hope), don't forget to clear your cache.

@marionf marionf removed their assignment Nov 7, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Nov 7, 2018

$id = sprintf('%d-%d', $feature['feature'], $feature['value']);
if (in_array($id, $ids)) {
unset($data['features'][$idx]);

This comment has been minimized.

@jolelievre

jolelievre Nov 8, 2018

Contributor

isn't it dangerous to unset an element of the array while going through it?
you should either copy the array and go though the copy, or instantiate a new array and populate it while going through the initial one

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 8, 2018

Contributor

Nope it's not dangerous and because it's a data we really want to remove in this array :)
If I copy the array into another, the $_POST and other will not be changed and the legacy code will still using the wrong data.

This comment has been minimized.

@jolelievre

jolelievre Nov 8, 2018

Contributor

alright, I understand that you need to unset this array specificly
but you are sur there's not a chance you miss an index if you unset one during the loop? does php stores/saves each index? or the Iterator interface is able not to get lost?

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 8, 2018

Contributor

It always clean the current index and not the one after.
Nothing can be lost :)

This comment has been minimized.

@jolelievre

jolelievre Nov 8, 2018

Contributor

alright, good to know!! :)
all good then

@matks

This comment has been minimized.

Contributor

matks commented Nov 9, 2018

Thank you @PierreRambaud

@matks matks merged commit 2fe87c0 into PrestaShop:1.7.5.x Nov 9, 2018

1 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@PierreRambaud PierreRambaud deleted the PierreRambaud:fix-product-features branch Nov 9, 2018

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