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

Added check for coupon expiration date #3144

Merged
merged 1 commit into from
Apr 5, 2023
Merged

Added check for coupon expiration date #3144

merged 1 commit into from
Apr 5, 2023

Conversation

fballiano
Copy link
Contributor

Mage_SalesRule_Model_Validator::_canProcessRule() checks the validity of a coupon code that is in the process of being added to the cart, but it doesn't check for the single coupon expiration date. This PR adds this check.

Manual testing scenarios (*)

  1. in the backend go to "promotions -> Shopping Cart Price Rules"
  2. create a new rule with "specific coupon" (but also autogenerated coupons have the same issue) and type a coupon code
  3. be sure that the rule also has some "action", ex: 10% discount
  4. save the rule and check that it's working on the frontend

Now, what we're about to check isn't really supported by the core but I think it's still worth fixing.

  1. open your database console and check for the salesrule_coupon table, you'll find a record with the coupon code you just created above
  2. modify the expiration_date column for that specific coupon and set it in the past
  3. go to the frontend, apply the coupon and notice that it gets applied anyway

It is true that the core wouldn't manage the expiration_date column but, since it exists, I know extensions (actually I was developing one today when I found this bug) that set the expiration_data for a single coupon (let's say you want a coupon to be usable only for 7 days after its generation), and it doesn't make sense to me that the core doesn't check for the expiration_date of a coupon.

After applying this PR you'll notice that the expiration_date gets correctly checked, if present.

Related PRs

This PR was already approved in #3029 but I had to recreate it due to rebasing problems.

@github-actions github-actions bot added the Component: SalesRule Relates to Mage_SalesRule label Apr 5, 2023
@fballiano fballiano requested a review from luigifab April 5, 2023 08:35
@fballiano fballiano merged commit 04d4e68 into OpenMage:main Apr 5, 2023
@fballiano fballiano deleted the couponexpire2 branch April 5, 2023 13:58
@luigifab
Copy link
Contributor

This create a huge side effect! If you have multiple coupon code with same code in multiple rules, if one of the coupon is expired, this prevent another rule to work (here 8829).

select * from salesrule_coupon where code = "JULIA30";
+-----------+----------+---------+-------------+--------------------+------------+---------------------+
| coupon_id |  rule_id | code    | usage_limit | usage_per_customer | times_used | expiration_date     |
+-----------+----------+---------+-------------+--------------------+------------+---------------------+
|   1180785 | off 5409 | JULIA30 |        NULL |               NULL |         84 | 2018-11-30 00:00:00 |
|   1203366 | off 5828 | JULIA30 |        NULL |               NULL |          0 | NULL                |
|   1357897 | off 7914 | JULIA30 |        NULL |               NULL |          0 | NULL                |
|   1358070 | off 7915 | JULIA30 |        NULL |               NULL |          0 | NULL                |
|   1358243 | off 7916 | JULIA30 |        NULL |               NULL |          0 | NULL                |
|   1519071 |  on 8829 | JULIA30 |        NULL |               NULL |          0 | NULL                |
+-----------+----------+---------+-------------+--------------------+------------+---------------------+

@colinmollenhour
Copy link
Member

Yikes!

What should the correct behavior be? Check all coupons or just the newest one? For example here is checking the newest one:

                $couponCollection = Mage::getResourceModel('salesrule/coupon_collection');
                $couponCollection->addFieldToFilter('code', $couponCode);
                $couponCollection->setOrder('rule_id', 'desc');
                $couponCollection->setPageSize(1);
                if ($couponCollection->count()) {
                    $coupon = $couponCollection->getFirstItem(); /* @var $coupon Mage_SalesRule_Model_Coupon */

@fballiano
Copy link
Contributor Author

@luigifab checkin the code, the same thing would happen if usage limit is surpassed:

                    // check entire usage limit
                    if ($coupon->getUsageLimit() && $coupon->getTimesUsed() >= $coupon->getUsageLimit()) {
                        $rule->setIsValidForAddress($address, false);
                        return false;
                    }
                    // check coupon expiration
                    if ($coupon->hasExpirationDate() && ($coupon->getExpirationDate() < Mage::getModel('core/date')->date())) {
                        $rule->setIsValidForAddress($address, false);
                        return false;
                    }
                    // check per customer usage limit
                    $customerId = $address->getQuote()->getCustomerId();
                    if ($customerId && $coupon->getUsagePerCustomer()) {
                        $couponUsage = new Varien_Object();
                        Mage::getResourceModel('salesrule/coupon_usage')->loadByCustomerCoupon(
                            $couponUsage,
                            $customerId,
                            $coupon->getId()
                        );
                        if ($couponUsage->getCouponId() &&
                            $couponUsage->getTimesUsed() >= $coupon->getUsagePerCustomer()
                        ) {
                            $rule->setIsValidForAddress($address, false);
                            return false;
                        }
                    }

so I don't know...

@luigifab
Copy link
Contributor

luigifab commented Apr 25, 2023

I'm not sure, I simply reverted this PR, and now my coupon code is working.
Perhaps the truth is out there.

Here (l. 177) we are loading the coupon by code, perhaps we can load it by code and rule id?

$coupon = Mage::getModel('salesrule/coupon');
$coupon->load($couponCode, 'code');

@colinmollenhour
Copy link
Member

Here (l. 177) we are loading the coupon by code, perhaps we can load it by code and rule id?

Ahh yes, by rule id sounds correct to me..

                $couponCollection = Mage::getResourceModel('salesrule/coupon_collection');
                $couponCollection->addFieldToFilter('code', $couponCode);
                $couponCollection->addFieldToFilter('rule_id', $rule->getId());
                $couponCollection->setOrder('coupon_id', 'desc');
                $couponCollection->setPageSize(1);
                if ($couponCollection->count()) {
                    $coupon = $couponCollection->getFirstItem(); /* @var $coupon Mage_SalesRule_Model_Coupon */

However, I never use this feature so am no expert on it. Are there other circumstances where a rule can have multiple coupons with the same code making it necessary to filter out used up ones or loop through all of them? Just trying to throw out some suggestions, please open a PR if you see fit.

@kyrena
Copy link
Contributor

kyrena commented May 16, 2023

A new PR for this new problem is required?

@colinmollenhour
Copy link
Member

@kyrena I think yes

@fballiano
Copy link
Contributor Author

@luigifab @kyrena how can you create multiple rules with the same coupon code (since the records you copied do not seem to be autogenerated coupons)?

because it is forbidden:
Screenshot 2023-05-16 alle 15 35 14

@luigifab
Copy link
Contributor

Oh yes, true, we are using a module to allow multi coupon code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: SalesRule Relates to Mage_SalesRule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants