-
Notifications
You must be signed in to change notification settings - Fork 257
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 #2901504 by sumanthkumarc: Provide limit by no of orders per user condition for promotions in commerce core #771
base: 8.x-2.x
Are you sure you want to change the base?
Conversation
…er condition for promotions in commerce core
@sumanthkumarc How is this different than limited per user? |
@mglaman Thanks for the review :) When you say limited per user? you mean the option "Usage limits" available in the right hand panes on promotions form?? If so , i saw that no where the mail is passed to check availability per user, also i guess its confusing for user to understand if this is total limit irrespective of users or per user limit. See this available method from promotion class. public function available(OrderInterface $order) {
if (!$this->isEnabled()) {
return FALSE;
}
if (!in_array($order->bundle(), $this->getOrderTypeIds())) {
return FALSE;
}
if (!in_array($order->getStoreId(), $this->getStoreIds())) {
return FALSE;
}
$time = \Drupal::time()->getRequestTime();
if ($this->getStartDate()->format('U') > $time) {
return FALSE;
}
$end_date = $this->getEndDate();
if ($end_date && $end_date->format('U') <= $time) {
return FALSE;
}
if ($usage_limit = $this->getUsageLimit()) {
/** @var \Drupal\commerce_promotion\PromotionUsageInterface $usage */
$usage = \Drupal::service('commerce_promotion.usage');
if ($usage_limit <= $usage->getUsage($this)) {
return FALSE;
}
}
return TRUE;
} let me know if i'm missing anything here :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sumanthkumarc I see. I thought we had rate limiting per user.
|
||
$orders = $this->orderStorage->getQuery() | ||
->condition('cart', FALSE) | ||
->condition('uid', $user_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anonymous orders would always fail, correct? Should we be looking up the user's email (or current order email) and any orders with that email?
@mglaman Rate/usage limiting is not the same thing. That is "a user can only receive this promotion once". This is "the user can only receive this promotion if this is his first order (or one of the first N)". |
@bojanz @mglaman Even then, below is the getUsage method, which takes $mail for rate limiting, and i wasn't able to find any instance where, getUsage() method is passed with mail, everywhere either promotion object or coupon object is being passed. /**
* {@inheritdoc}
*/
public function getUsage(PromotionInterface $promotion, CouponInterface $coupon = NULL, $mail = NULL) {
$coupons = $coupon ? [$coupon] : [];
$usages = $this->getUsageMultiple([$promotion], $coupons, $mail);
return $usages[$promotion->id()];
} UPDATE: According to bojan, this is the intended behaviour and per user rate limiting is new feature. |
No description provided.