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

Open

swickham78 wants to merge 21 commits intodrupalcommerce:8.x-2.x from

swickham78:promotions_disable_expired

Open

#2863864 - Disable promotions via cron if end date or usage limit hit #708
swickham78 wants to merge 21 commits intodrupalcommerce:8.x-2.x from
swickham78:promotions_disable_expired

Conversation

@swickham78

Copy link

@swickham78 swickham78 commented Apr 4, 2017

No description provided.


 Disable expired promotions on cron run.

527df0b

mglaman

mglaman requested changes

Apr 4, 2017

View reviewed changes

Copy link

Contributor

@mglaman mglaman left a comment

Choose a reason for hiding this comment

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

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.

modules/promotion/commerce_promotion.module Outdated

/**

* Get all expired promotions that are still active and disable them.

function commerce_promotion_disabled_expired() {

Copy link

Contributor

@mglaman mglaman Apr 4, 2017

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.

modules/promotion/commerce_promotion.module

if ($promotions !== FALSE) {

// Disable all expired promotions.

foreach ($promotions as $promotion) {

Copy link

Contributor

@mglaman mglaman Apr 4, 2017

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.

modules/promotion/src/PromotionStorage.php Outdated

// 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');

Copy link

Contributor

@mglaman mglaman Apr 4, 2017

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

@swickham78 swickham78 Apr 5, 2017

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?

modules/promotion/src/PromotionStorage.php Outdated

$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.

Copy link

Contributor

@mglaman mglaman Apr 4, 2017

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.

modules/promotion/src/PromotionStorage.php

if ($only_enabled) {

$query->condition('cpd.status', 1);

}

Copy link

Contributor

@mglaman mglaman Apr 4, 2017

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

Sean Wickham added 3 commits

April 7, 2017 01:10


 Merge branch '8.x-2.x' of https://github.com/swickham78/commerce into...

221cbd1

... promotions_disable_expired


 Issue #2863864: Moved max usage load of promotions to its own method ...

2b8f266

...making use of existing service.


 Issue #2863864: Added tests for loading of expired and maxed usaged p...

caf23a5

...romotions.

@mglaman

Copy link

Contributor

mglaman commented Apr 7, 2017

@swickham78 at quick glance this looks good, better change. Bojan made some changes which are causing conflict.


 Issue #2863864: Fix merge conflict w/ recent work.

863ce8f

@swickham78

Copy link

Author

swickham78 commented Apr 7, 2017

@mglaman Conflicts have been addressed.

@mglaman

Copy link

Contributor

mglaman commented Apr 8, 2017

@swickham78 okay, cool. Errors are just PHPCS. I'll get around to a proper review

Sean Wickham added 2 commits

April 10, 2017 16:15


 Merge branch '8.x-2.x' of https://github.com/swickham78/commerce into...

24856a6

... promotions_disable_expired


 Issue #2863864: Updates to meet coding standards.

e7d8a40

mglaman

mglaman requested changes

Apr 11, 2017

View reviewed changes

Copy link

Contributor

@mglaman mglaman left a comment

Choose a reason for hiding this comment

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

The storage methods are tested, but the cron op is still not tested.

modules/promotion/src/PromotionStorage.php Outdated

}

/**

* Return active promotions that have passed their end date.

Copy link

Contributor

@mglaman mglaman Apr 11, 2017

Choose a reason for hiding this comment

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

Document blocks should only be on the interface, implementers just do {@inheritdoc}

modules/promotion/src/PromotionStorage.php Outdated

}

/**

* Returns active promotions which have a met their maximum usage.

Copy link

Contributor

@mglaman mglaman Apr 11, 2017

Choose a reason for hiding this comment

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

Document blocks should only be on the interface, implementers just do {@inheritdoc}

modules/promotion/src/PromotionStorageInterface.php

* @return \Drupal\commerce_promotion\Entity\PromotionInterface[]

* Promotions with maxed usage.

public function loadMaxedUsage();

Copy link

Contributor

@mglaman mglaman Apr 11, 2017

Choose a reason for hiding this comment

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

@bojanz name bikeshedding.

modules/promotion/tests/src/Kernel/PromotionStorageTest.php Outdated

$this->assertEquals(SAVED_NEW, $expired_promotion->save());

$promotions = $this->promotionStorage->loadExpired();

$this->assertEquals(count($promotions), 1);

Copy link

Contributor

@mglaman mglaman Apr 11, 2017

Choose a reason for hiding this comment

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

Can do assertCount

@bojanz bojanz force-pushed the 8.x-2.x branch 7 times, most recently from 6be1d5c to b8a7444 Compare

April 27, 2017 20:46

@bojanz bojanz force-pushed the 8.x-2.x branch from a047f90 to 4366f38 Compare

June 12, 2017 14:59

Sean Wickham added 6 commits

June 26, 2017 20:15


 Disable expired promotions on cron run.

0794a1b


 Issue #2863864: Moved max usage load of promotions to its own method ...

e0f422f

...making use of existing service.


 Issue #2863864: Added tests for loading of expired and maxed usaged p...

73e867a

...romotions.


 Issue #2863864: Updates to meet coding standards.

70ce340


 Added cron function test.

029e4c4


 Merge branch 'promotions_disable_expired' of https://github.com/swick...

4cb8f54

...ham78/commerce into promotions_disable_expired

@swickham78

Copy link

Author

swickham78 commented Jun 28, 2017

@mglaman Completely forgot this was left unresolved. Just pushed a new commit with adjustments based on your last notes.

@bojanz bojanz force-pushed the 8.x-2.x branch from dc443c7 to e290888 Compare

July 27, 2017 12:21

@bojanz bojanz force-pushed the 8.x-2.x branch from 5b3779b to 2941d08 Compare

August 10, 2017 17:37


 Disable expired promotions on cron run.

a557186

Sean Wickham added 7 commits

August 10, 2017 22:05


 Issue #2863864: Moved max usage load of promotions to its own method ...

f659ea1

...making use of existing service.


 Issue #2863864: Added tests for loading of expired and maxed usaged p...

6bba454

...romotions.


 Issue #2863864: Updates to meet coding standards.

4ac0f14


 Issue #2863864: Moved max usage load of promotions to its own method ...

d3fda01

...making use of existing service.


 Issue #2863864: Updates to meet coding standards.

9aef5f3


 Added cron function test.

04ef19f


 Merge branch 'promotions_disable_expired' of https://github.com/swick...

d5a4e42

...ham78/commerce into promotions_disable_expired

@swickham78

Copy link

Author

swickham78 commented Aug 11, 2017

@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.

mglaman

mglaman approved these changes

Aug 11, 2017

View reviewed changes

@bojanz bojanz force-pushed the 8.x-2.x branch from 53ec70a to 3bc1b88 Compare

September 12, 2017 23:36

@bojanz bojanz force-pushed the 8.x-2.x branch from 55280a3 to 4e9a025 Compare

June 15, 2018 12:43

@bojanz bojanz force-pushed the 8.x-2.x branch from b1be14f to 59230dd Compare

June 28, 2018 16:59

@bojanz bojanz force-pushed the 8.x-2.x branch from 807c6e8 to 8898142 Compare

October 8, 2018 15:18

Labels

None yet

2 participants

@swickham78 @mglaman

Conversation

@swickham78 swickham78 commented Apr 4, 2017

Uh oh!

@mglaman mglaman left a comment

Choose a reason for hiding this comment

Uh oh!

@mglaman mglaman Apr 4, 2017

Choose a reason for hiding this comment

Uh oh!

@mglaman mglaman Apr 4, 2017

Choose a reason for hiding this comment

Uh oh!

@mglaman mglaman Apr 4, 2017

Choose a reason for hiding this comment

Uh oh!

@swickham78 swickham78 Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

@mglaman mglaman Apr 4, 2017

Choose a reason for hiding this comment

Uh oh!

@mglaman mglaman Apr 4, 2017

Choose a reason for hiding this comment

Uh oh!

mglaman commented Apr 7, 2017

Uh oh!

swickham78 commented Apr 7, 2017

Uh oh!

mglaman commented Apr 8, 2017

Uh oh!

@mglaman mglaman left a comment

Choose a reason for hiding this comment

Uh oh!

@mglaman mglaman Apr 11, 2017

Choose a reason for hiding this comment

Uh oh!

@mglaman mglaman Apr 11, 2017

Choose a reason for hiding this comment

Uh oh!

@mglaman mglaman Apr 11, 2017

Choose a reason for hiding this comment

Uh oh!

@mglaman mglaman Apr 11, 2017

Choose a reason for hiding this comment

Uh oh!

swickham78 commented Jun 28, 2017

Uh oh!

swickham78 commented Aug 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments