#2863864 - Disable promotions via cron if end date or usage limit hit #708
Conversation
There was a problem hiding this comment.
There are a few changes and no tests. We definitely need to test this.
I don't think we need to use a queue worker. But I'd like to use more of the API for querying the promotions. Even if this means running an entity query to load invalid via end date. Then load valid and check their usages and add to list of promotions to exired/disable.
I'm not too worried on the performance since this should only be invoked during cron.
| /** | ||
| * Get all expired promotions that are still active and disable them. | ||
| */ | ||
| function commerce_promotion_disabled_expired() { |
There was a problem hiding this comment.
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.
|
|
||
| if ($promotions !== FALSE) { | ||
| // Disable all expired promotions. | ||
| foreach ($promotions as $promotion) { |
There was a problem hiding this comment.
@bojanz I think this is fine versus a queue. There shouldn't be that much activity, generally.
| $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'); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
|
|
||
| // We want to get results of queries that have passed their final date or | ||
| // have met their max usage. | ||
| $or_condition = $query->orConditionGroup() |
There was a problem hiding this comment.
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.
| $query->condition('cpd.status', 1); | ||
| } | ||
|
|
||
| $result = $query->execute()->fetchCol(0); |
There was a problem hiding this comment.
We return the invalid and set them as disabled. Then I'd say run the usage checks afterwards for anything not yet expired
… promotions_disable_expired
…making use of existing service.
|
@swickham78 at quick glance this looks good, better change. Bojan made some changes which are causing conflict. |
|
@mglaman Conflicts have been addressed. |
|
@swickham78 okay, cool. Errors are just PHPCS. I'll get around to a proper review |
… promotions_disable_expired
| @@ -104,4 +104,61 @@ public function loadAvailable(OrderTypeInterface $order_type, StoreInterface $st | |||
| return $promotions; | |||
| } | |||
|
|
|||
| /** | |||
| * Return active promotions that have passed their end date. | |||
There was a problem hiding this comment.
Document blocks should only be on the interface, implementers just do {@inheritdoc}
| } | ||
|
|
||
| /** | ||
| * Returns active promotions which have a met their maximum usage. |
There was a problem hiding this comment.
Document blocks should only be on the interface, implementers just do {@inheritdoc}
| * @return \Drupal\commerce_promotion\Entity\PromotionInterface[] | ||
| * Promotions with maxed usage. | ||
| */ | ||
| public function loadMaxedUsage(); |
There was a problem hiding this comment.
@bojanz name bikeshedding.
| $this->assertEquals(SAVED_NEW, $expired_promotion->save()); | ||
|
|
||
| $promotions = $this->promotionStorage->loadExpired(); | ||
| $this->assertEquals(count($promotions), 1); |
bojanz
force-pushed
the
8.x-2.x
branch
7 times, most recently
from
6be1d5c to
b8a7444
Compare
…making use of existing service.
…ham78/commerce into promotions_disable_expired
|
@mglaman Completely forgot this was left unresolved. Just pushed a new commit with adjustments based on your last notes. |
…making use of existing service.
…making use of existing service.
…ham78/commerce into promotions_disable_expired
|
@mglaman Looks like this got lost in the shuffle. Nothing changed since the last push, just did a rebase and fixed some conflicts that came form it. |
No description provided.