View Issue Details

IDProjectCategoryView StatusLast Update
0006252OXID eShop (all versions)1.03. Basket, checkout processpublic2015-11-07 13:32
Reportermario_lorenz 
PrioritynormalSeverityminorReproducibilityalways
Status acknowledgedResolutionopen 
Product Version4.9.6 / 5.2.6 
Target VersionFixed in Version 
Summary0006252: bug in method isForBasket (class oxdelivery.php)
DescriptionI have three deliverys D.A, D.B, D.C

Each delivery has an own deliveryset
DS.A -> D.A
DS.B -> D.B
DS.C -> D.C

I have two Articles X and Y

Article X is connect with delivery D.A and D.B
Article Y is connect with delivery D.A and D.C

First i put article X in my basket. In the checkout the Delivery-Sets DS.A and DS.B are shown.
Now i delete article X from the basket and i put article Y in the basket.
In the checkout the Delivery-Sets DS.A and DS.C are shown.

Both situations are just as I expected it.

Now i put both articles in my Basket. Normally i would expect, that only Delivery-Set DS.A are shown, because only DS.A is valid for both, but all three Delivery-Sets are shown. Thats a mistake.
Steps To ReproduceCreate three deliverys
Create three deliverysets

Connect each of the deliverys with exact one deliveryset

Connect two different Articles like this:
Article X is connect with delivery A and B
Article Y is connect with delivery A and C

Put both articles in the basket. Go to the checkout.
Additional InformationI helped me in the moment with an overloading of the method.
So I get my expected behavior. But it could be that I've forgotten something.

(look at variable $bIsForAllArticlesInBasket)

public function isForBasket($oBasket)
{
  // amount for conditional check
  $blHasArticles = $this->hasArticles();
  $blHasCategories = $this->hasCategories();
  $blUse = true;
  $iAmount = 0;
  $blForBasket = false;
  $bIsForAllArticlesInBasket = true;

  // category & article check
  if ($blHasCategories || $blHasArticles)
  {
    $blUse = false;

    $aDeliveryArticles = $this->getArticles();
    $aDeliveryCategories = $this->getCategories();

    foreach ($oBasket->getContents() as $oContent)
    {
      //V FS#1954 - load delivery for variants from parent article
      $oArticle = $oContent->getArticle(false);
      $sProductId = $oArticle->getProductId();
      $sParentId = $oArticle->getParentId();

      if ($blHasArticles && (in_array($sProductId, $aDeliveryArticles) || ($sParentId && in_array($sParentId, $aDeliveryArticles)))) {
        $blUse = true;
        $iArtAmount = $this->getDeliveryAmount($oContent);
        if ($this->getCalculationRule() != self::CALCULATION_RULE_ONCE_PER_CART)
        {
          if ($this->_isForArticle($oContent, $iArtAmount))
          {
            $blForBasket = true;
          }
        }
        if (!$blForBasket)
        {
          $iAmount += $iArtAmount;
        }
      }
      elseif ($blHasCategories)
      {
        if (isset(self::$_aProductList[$sProductId]))
        {
          $oProduct = self::$_aProductList[$sProductId];
        }
        else
        {
          $oProduct = oxNew('oxArticle');
          $oProduct->setSkipAssign(true);

          if (!$oProduct->load($sProductId))
          {
            continue;
          }
          $oProduct->setId($sProductId);
          self::$_aProductList[$sProductId] = $oProduct;
        }

        foreach ($aDeliveryCategories as $sCatId)
        {
          if ($oProduct->inCategory($sCatId))
          {
            $blUse = true;
            $iArtAmount = $this->getDeliveryAmount($oContent);
            if ($this->getCalculationRule() != self::CALCULATION_RULE_ONCE_PER_CART)
            {
              if ($this->_isForArticle($oContent, $iArtAmount))
              {
                $blForBasket = true;
              }
            }
            if (!$blForBasket)
            {
              $iAmount += $iArtAmount;
            }
            //HR#5650 product might be in multiple rule categories, counting it once is enough
            break;
          }
        }
      }
      else
      {
        $bIsForAllArticlesInBasket = false;
      }
    }
  }
  else
  {
    // regular amounts check
    foreach ($oBasket->getContents() as $oContent)
    {
      $iArtAmount = $this->getDeliveryAmount($oContent);
      if ($this->getCalculationRule() != self::CALCULATION_RULE_ONCE_PER_CART)
      {
        if ($this->_isForArticle($oContent, $iArtAmount))
        {
          $blForBasket = true;
        }
      }
      if (!$blForBasket)
      {
        $iAmount += $iArtAmount;
      }
    }
  }

  //#M1130: Single article in Basket, checked as free shipping, is not buyable (step 3 no payments found)
  if (!$blForBasket && $blUse && $bIsForAllArticlesInBasket && ($this->_checkDeliveryAmount($iAmount) || $this->_blFreeShipping))
  {
    $blForBasket = true;
  }

  return $blForBasket;
}
TagsNo tags attached.
ThemeNot defined
BrowserNot defined
PHP VersionNot defined
MySQL VersionNot defined

Activities

leofonic

2015-11-03 17:09

reporter   ~0011301

This is a duplicate of 0001659, which has eventually been marked resolved and moved to uservoice. Method isForBasket serves two purposes:

1. Shipping cost calculation (needs match if one article is valid)
2. Selection of valid deliverysets (needs match if all articles are valid)

This will not work at the same time. If you change isForBasket to satisfy number 2, number 1 will go wrong (0002149)

This method has to be refactored so both cases can be handled.