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.

Sign up for GitHub

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

Jump to bottom

Issue #2763705 by mglaman: Implement a usage API #695

Closed
wants to merge 1 commit into from
Closed

Issue #2763705 by mglaman: Implement a usage API #695

wants to merge 1 commit into from

Conversation

mglaman
Copy link
Contributor

@mglaman mglaman commented Mar 24, 2017

Here's a TDD approach to it. Uncovered a lot of flaws with our processing.

mglaman
mglaman commented Mar 24, 2017
View reviewed changes
modules/promotion/tests/src/Kernel/UsageTest.php Outdated
* @covers ::getCouponUsage
* @covers ::addCouponUsage
*/
public function testMockUsage() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bojanz this method at leasts tests the logic and storage. Order refresh and legitimate implementation will take some time. If you'd like to review the table name, columns, and basic storage logic.

@mglaman mglaman force-pushed the 2763705-promotion-usage branch 2 times, most recently from b4009d1 to a85cbd3 Compare March 25, 2017 02:35
bojanz
bojanz reviewed Mar 25, 2017
View reviewed changes
modules/promotion/src/PromotionUsageInterface.php Outdated
* @param \Drupal\commerce_order\Entity\OrderInterface $order
* The order.
*/
public function addPromotionUsage(PromotionInterface $promotion, OrderInterface $order);
Copy link
Contributor

Choose a reason for hiding this comment

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

why not a single addUsage($order, $promotion, $coupon = NULL)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess. Just felt weird since we can get the promotion from the coupon, but a single method for adding works for me

bojanz
bojanz reviewed Mar 25, 2017
View reviewed changes
modules/promotion/src/PromotionUsageInterface.php Outdated
* @return int
* The usage.
*/
public function getCouponUsage(CouponInterface $coupon);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, getUsage($promotion, $coupon = NULL), and some documentation to clarify?

bojanz
bojanz reviewed Mar 25, 2017
View reviewed changes
modules/promotion/src/PromotionStorage.php
@@ -24,8 +24,27 @@ public function loadValid(OrderTypeInterface $order_type, StoreInterface $store)
if (empty($result)) {
return [];
}

$usages_query = $this->database->select('commerce_promotion_usage', 'cpu');
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels wrong to query the DB table outside of the service. We have an API, let's use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The API doesn't allow for a bulk look up, and core does this as well within storage to provide a more performant query. Otherwise we'd have to do a SELECT for each promotion, and that's wasteful

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it's wasteful, but that means we need a new method on the service then. If we keep it this way people won't be able to easily swap the service to use a different storage, which is something that core encourages with its own examples.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which reminds me, we should look into the backend_overridable tag for the service.

@bojanz
Copy link
Contributor

bojanz commented Mar 25, 2017

We need a post_update to drop the current_usage field.

@mglaman mglaman force-pushed the 2763705-promotion-usage branch 3 times, most recently from 5f48c6d to 42af6d7 Compare March 25, 2017 20:11
bojanz
bojanz requested changes Mar 27, 2017
View reviewed changes
modules/promotion/commerce_promotion.install
*/
function commerce_promotion_schema() {
$schema['commerce_promotion_usage'] = [
'description' => 'Stores module data as key/value pairs per user.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Untrue.

modules/promotion/commerce_promotion.install
'default' => 0,
],
'coupon_id' => [
'description' => 'Primary key: {commerce_promotion_coupon}.id for coupon.',
Copy link
Contributor

Choose a reason for hiding this comment

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

None of these are primary keys, right? We might as well drop the phrase.

modules/promotion/commerce_promotion.post_update.php
@@ -84,3 +84,17 @@ function commerce_promotion_post_update_4() {
$promotion->save();
}
}

/**
* Remove promotion current_usage field.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Remove the promotion current_usage field."
People will see the lack of an article and think a Slavic person wrote this :P

modules/promotion/src/PromotionUsageInterface.php
public function getUsage(PromotionInterface $promotion, CouponInterface $coupon = NULL);

/**
* Add a promotion usage record.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Adds a usage record for the given promotion." ?

modules/promotion/src/PromotionUsageInterface.php
interface PromotionUsageInterface {

/**
* Gets the usage for a promotion.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Gets the usage for the given promotion."?

modules/promotion/src/PromotionUsageInterface.php
* @param \Drupal\commerce_promotion\Entity\PromotionInterface $promotion
* The promotion.
* @param \Drupal\commerce_promotion\Entity\CouponInterface|null $coupon
* The promotion's coupon, optional.
Copy link
Contributor

Choose a reason for hiding this comment

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

(Optional) The used coupon.

modules/promotion/src/PromotionStorage.php
$usages_query = $this->database->select('commerce_promotion_usage', 'cpu');
$usages_query->condition('promotion_id', array_keys($result), 'IN');
$usages_query->addField('cpu', 'promotion_id');
$usages_query->addExpression("COUNT(promotion_id)", 'count');
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call the PromotionUsage method: getUsageMultiple(array $promotions, array $coupons)

@bojanz
Copy link
Contributor

bojanz commented Mar 28, 2017

Final version committed in 5ab9e14.
I'll explain what changed in the issue.

@bojanz bojanz closed this Mar 28, 2017
@mglaman mglaman deleted the 2763705-promotion-usage branch March 29, 2017 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bojanz bojanz bojanz requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@mglaman @bojanz