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

6539: split selectlists and varselect #507

Closed

Conversation

Projects
None yet
2 participants
@alfredbez
Copy link
Member

commented Nov 10, 2016

This fixed 6539, I described the problem and steps to reproduce there.

I introduced a new delimiter || if a value from a selectlist and a variant are selected.

To make things testable I changed $oProduct->oxarticles__oxvarselect->getRawValue() to $oContent->getVarSelect(), since we're using this already for selectlists ($oContent->getChosenSelList()).

I also added a break to a foreach loop in getOrderArticleSelectList() because we don't need to check all other selectlist values if we already found a matching one.

You can test the wrong behaviour with this failing test:

diff --git a/tests/Unit/Application/Model/OrderarticleTest.php b/tests/Unit/Application/Model/OrderarticleTest.php
index 62d65fa..2303a13 100644
--- a/tests/Unit/Application/Model/OrderarticleTest.php
+++ b/tests/Unit/Application/Model/OrderarticleTest.php
@@ -431,6 +431,11 @@ class OrderarticleTest extends \OxidTestCase
         $sFields = "Color : red, Material : wood ";
         $oOrderArticle = oxNew('oxOrderArticle');
         $this->assertEquals(array(0 => 0), $oOrderArticle->getOrderArticleSelectList('1126', $sFields));
+
+        // articles with selectlists and variants should work
+        $sFields = "Color : red variantvalue1 | variantvalue2";
+        $oOrderArticle = oxNew('oxOrderArticle');
+        $this->assertEquals(array(0 => 0), $oOrderArticle->getOrderArticleSelectList('1126', $sFields));
     }
 
     public function testMakeSelListArrayPriceON()
split selectlists and varselect
use '||' as delimiter if both (selectlist and varselect) is used
break out foreach if selectlist found in OrderArticle@getOrderArticleSelectList()

@alfredbez alfredbez changed the title split selectlists and varselect 6539: split selectlists and varselect Nov 10, 2016

@alfredbez

This comment has been minimized.

Copy link
Member Author

commented Nov 24, 2016

It would be nicer if we could create a new field like oxselectlistvalue in oxorderarticles

@alfredbez

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2017

Example

Article shirt
Variants
color red, blue
size s, m, l
Selectlists
someSelectlist firstValue, secondValue, thirdValue

User buys Article "shirt" with the following selection:

variant red & s
selectlist secondValue

OXSELVARIANT becomes: someSelectlist : secondValue | red | s
see:

$oOrderArticle->oxorderarticles__oxselvariant = new oxField(trim($sSelList . ' ' . $oProduct->oxarticles__oxvarselect->getRawValue()), oxField::T_RAW);

Problem: The delimiter | is used to separate each variant, but also to separate the variant-selection and the selectlist.

My suggested solution: Use | to separate each variant and || to separate the variant-selection and the selectlist. So OXSELVARIANT becomes: someSelectlist : secondValue || red | s

@alfredbez

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2018

I've resolved the merge conflicts.

@Sieg Sieg added the in progress label Oct 23, 2018

@Sieg

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2018

Hello @alfredbez, i have merged the fix to b-6.1.x recently on 5b7de69

@Sieg Sieg closed this Oct 24, 2018

@Sieg Sieg removed the in progress label Oct 24, 2018

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.