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

Issue with one use limited coupon #1408

Open
diogoceribelli opened this issue Jan 21, 2021 · 3 comments
Open

Issue with one use limited coupon #1408

diogoceribelli opened this issue Jan 21, 2021 · 3 comments

Comments

@diogoceribelli
Copy link

Magento 1.9

Steps to reproduce (*)

  1. Administrator creates a new discount coupon, limited to one use;
  2. Two or more users insert products into their shopping cart and apply the discount using the coupon;
  3. Two or more users click on checkout at the same time.

Expected result (*)

  1. Even if pressing at the same time, some order must have been placed first;
  2. This order will receive the coupon discount, subsequent orders will receive some type of error saying that the coupon has already been used;
  3. Now the coupon that had only one use, can no longer be used.

Actual result (*)

  1. All users who pressed the checkout button at the same time will receive the coupon discount even with the one use limit;
  2. Only after simultaneous orders will the coupon be deactivated.

The problem then is that purchases made simultaneously will disregard the limit defined in the discount coupon rule. This should not happen, only one of those purchases should receive the discount.

@colinmollenhour
Copy link
Member

Magento is actually rife with possible race conditions.. The only place it uses real locking (SELECT ... FOR UPDATE) is around the inventory. Everything else is anything goes. It may be possible to reorganize the code so that coupon loading and updates happen inside the transaction after the inventory is locked which would probably fix the issue. I personally am not in a position to tackle this myself right now, though.

@kiatng
Copy link
Collaborator

kiatng commented Jan 22, 2021

I can confirm @colinmollenhour that real locking is SELECT ... FOR UPDATE, an example:

/**
* Retreive new incrementId
*
* @param int $storeId
* @return string
* @throws Exception
*/
public function fetchNewIncrementId($storeId = null)
{
if (!$this->getIncrementModel()) {
return false;
}
if (!$this->getIncrementPerStore() || ($storeId === null)) {
/**
* store_id null we can have for entity from removed store
*/
$storeId = 0;
}
// Start transaction to run SELECT ... FOR UPDATE
$this->_getResource()->beginTransaction();
try {
$entityStoreConfig = Mage::getModel('eav/entity_store')
->loadByEntityStore($this->getId(), $storeId);
if (!$entityStoreConfig->getId()) {
$entityStoreConfig
->setEntityTypeId($this->getId())
->setStoreId($storeId)
->setIncrementPrefix($storeId)
->save();
}
/** @var Mage_Eav_Model_Entity_Increment_Abstract $incrementInstance */
$incrementInstance = Mage::getModel($this->getIncrementModel())
->setPrefix($entityStoreConfig->getIncrementPrefix())
->setPadLength($this->getIncrementPadLength())
->setPadChar($this->getIncrementPadChar())
->setLastId($entityStoreConfig->getIncrementLastId())
->setEntityTypeId($entityStoreConfig->getEntityTypeId())
->setStoreId($entityStoreConfig->getStoreId());
/**
* do read lock on eav/entity_store to solve potential timing issues
* (most probably already done by beginTransaction of entity save)
*/
$incrementId = $incrementInstance->getNextId();
$entityStoreConfig->setIncrementLastId($incrementId);
$entityStoreConfig->save();
// Commit increment_last_id changes
$this->_getResource()->commit();
} catch (Exception $e) {
$this->_getResource()->rollBack();
throw $e;
}
return $incrementId;
}

I had used similar method above in a custom module to resolve a race condition.

I hope this comment is useful for tackling this issue.

[edit]
I think this is the code @colinmollenhour is referring:

/**
* Correct particular stock products qty based on operator
*
* @param Mage_CatalogInventory_Model_Stock $stock
* @param array $productQtys
* @param string $operator +/-
* @return $this
*/
public function correctItemsQty($stock, $productQtys, $operator = '-')
{
if (empty($productQtys)) {
return $this;
}
$adapter = $this->_getWriteAdapter();
$conditions = array();
foreach ($productQtys as $productId => $qty) {
$case = $adapter->quoteInto('?', $productId);
$result = $adapter->quoteInto("qty{$operator}?", $qty);
$conditions[$case] = $result;
}
$value = $adapter->getCaseSql('product_id', $conditions, 'qty');
$where = array(
'product_id IN (?)' => array_keys($productQtys),
'stock_id = ?' => $stock->getId()
);
$adapter->beginTransaction();
try {
$adapter->update($this->getTable('cataloginventory/stock_item'), array('qty' => $value), $where);
$adapter->commit();
} catch (Exception $e) {
$adapter->rollBack();
throw $e;
}
return $this;

@lucasfpiovesan12
Copy link

@kiatng But how to implement this in the shopping cart rule?

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

No branches or pull requests

5 participants