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

#2863864 - Disable promotions via cron if end date or usage limit hit #708

Open
wants to merge 21 commits into
base: 8.x-2.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
527df0b
Disable expired promotions on cron run.
Apr 4, 2017
221cbd1
Merge branch '8.x-2.x' of https://github.com/swickham78/commerce into…
Apr 7, 2017
2b8f266
Issue #2863864: Moved max usage load of promotions to its own method …
Apr 7, 2017
caf23a5
Issue #2863864: Added tests for loading of expired and maxed usaged p…
Apr 7, 2017
863ce8f
Issue #2863864: Fix merge conflict w/ recent work.
Apr 7, 2017
24856a6
Merge branch '8.x-2.x' of https://github.com/swickham78/commerce into…
Apr 10, 2017
e7d8a40
Issue #2863864: Updates to meet coding standards.
Apr 10, 2017
0794a1b
Disable expired promotions on cron run.
Apr 4, 2017
e0f422f
Issue #2863864: Moved max usage load of promotions to its own method …
Apr 7, 2017
73e867a
Issue #2863864: Added tests for loading of expired and maxed usaged p…
Apr 7, 2017
70ce340
Issue #2863864: Updates to meet coding standards.
Apr 10, 2017
029e4c4
Added cron function test.
Jun 28, 2017
4cb8f54
Merge branch 'promotions_disable_expired' of https://github.com/swick…
Jun 28, 2017
a557186
Disable expired promotions on cron run.
Apr 4, 2017
f659ea1
Issue #2863864: Moved max usage load of promotions to its own method …
Apr 7, 2017
6bba454
Issue #2863864: Added tests for loading of expired and maxed usaged p…
Apr 7, 2017
4ac0f14
Issue #2863864: Updates to meet coding standards.
Apr 10, 2017
d3fda01
Issue #2863864: Moved max usage load of promotions to its own method …
Apr 7, 2017
9aef5f3
Issue #2863864: Updates to meet coding standards.
Apr 10, 2017
04ef19f
Added cron function test.
Jun 28, 2017
d5a4e42
Merge branch 'promotions_disable_expired' of https://github.com/swick…
Aug 11, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions modules/promotion/commerce_promotion.module
Original file line number Diff line number Diff line change
Expand Up @@ -79,3 +79,30 @@ function commerce_promotion_entity_base_field_info(EntityTypeInterface $entity_t
return $fields;
}
}

/**
* Implements hook_cron()
*/
function commerce_promotion_cron() {
commerce_promotion_disabled_expired();
}

/**
* Get all expired promotions that are still active and disable them.
*/
function commerce_promotion_disabled_expired() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the storage provides a method to load expired, we can just keep this within the cron, no need for its own function. If people want to run this manually then they can.

/** @var \Drupal\commerce_promotion\PromotionStorageInterface $promotion_storage */
$promotion_storage = \Drupal::service('entity_type.manager')->getStorage('commerce_promotion');

// Get query for all expired promotions.
$promotions = $promotion_storage->loadExpired();

if ($promotions !== FALSE) {
// Disable all expired promotions.
foreach ($promotions as $promotion) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bojanz I think this is fine versus a queue. There shouldn't be that much activity, generally.

$promotion->setEnabled(FALSE);
$promotion->save();
}
}
}

40 changes: 40 additions & 0 deletions modules/promotion/src/PromotionStorage.php
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,46 @@ public function loadByCoupon(OrderTypeInterface $order_type, StoreInterface $sto
return reset($promotions);
}

/**
* Builds a query that will load all promotions that are no longer valid
* determined by the base field limiting values.
*
* @param bool $only_enabled
* Only include currently enabled promotions that have expired.
* @return array|bool|\Drupal\Core\Entity\EntityInterface[]
* The expired promotion entities. Returns FALSE if none found.
*/
public function loadExpired($only_enabled = TRUE) {
// Subquery to determine the amount of times a coupon has been used.
$usage_query = $this->database->select('commerce_promotion_usage', 'cpu');
$usage_query->addExpression('COUNT(cpu.promotion_id)', 'count');
$usage_query->where('cpu.promotion_id = cpd.promotion_id');
$usage_query->groupBy('cpu.promotion_id');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have this query in the usage service :: getUsageMultiple. It feels like we should rework this method a bit so it can be used.

https://github.com/drupalcommerce/commerce/blob/8.x-2.x/modules/promotion/src/PromotionUsage.php#L65

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it work if I add another method to PromotionStorage that will load and check promotion uses by calling getUsageMultiple? That way I can run a quick query to only pass promotions to it which are active and have a usage set.

I think the only way I can mess around with getUsageMultiple to handle return usage on its own would be to be just make it load all promotions if none are passed which could be excessive even if we're not too concerned w/ performance.

Thoughts?


$query = $this->database->select($this->getDataTable(), 'cpd');

// We want to get results of queries that have passed their final date or
// have met their max usage.
$or_condition = $query->orConditionGroup()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is using a regular select, when we can run $this->getQuery().

I'd actually rather do a load of ->condition('end_date', gmdate('Y-m-d'), '<') and ->condition('status', TRUE)

We need < because it's valid if >=. This would cancel promotions on their last day.

->condition('cpd.end_date', gmdate('Y-m-d'), '<=')
->condition('cpd.usage_limit', $usage_query, '<=');

$query->addField('cpd', 'promotion_id');
$query->condition($or_condition);

if ($only_enabled) {
$query->condition('cpd.status', 1);
}

$result = $query->execute()->fetchCol(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We return the invalid and set them as disabled. Then I'd say run the usage checks afterwards for anything not yet expired


if (empty($result)) {
return FALSE;
}

return $this->loadMultiple($result);
}

/**
* Builds the base query for loading valid promotions.
*
Expand Down
11 changes: 11 additions & 0 deletions modules/promotion/src/PromotionStorageInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,15 @@ public function loadValid(OrderTypeInterface $order_type, StoreInterface $store)
*/
public function loadByCoupon(OrderTypeInterface $order_type, StoreInterface $store, CouponInterface $coupon);

/**
* Builds a query that will load all promotions that are no longer valid
* determined by the base field limiting values.
*
* @param bool $only_enabled
* Only include currently enabled promotions that have expired.
* @return array|bool|\Drupal\Core\Entity\EntityInterface[]
* The expired promotion entities. Returns FALSE if none found.
*/
public function loadExpired($only_enabled);

}