dotmailer / dotmailer-magento2-extension

The official Dotdigital for Magento2 extension
https://dotdigital.com/integrations/magento
MIT License
48 stars 63 forks source link

Plugin `afterUpdateSpecificCoupons` isn't working as it should be. Wrong coupons are updated. #526

Closed jasperzeinstra closed 5 years ago

jasperzeinstra commented 5 years ago

There's a big bug in de the plugin \Dotdigitalgroup\Email\Plugin\CouponPlugin::afterUpdateSpecificCoupons. It saves the $subject by using the values of the $rule. $subject is the coupon record, $rule is the sales rule record.

    /**
     * @param \Magento\SalesRule\Model\ResourceModel\Coupon $subject
     * @param \Magento\SalesRule\Model\ResourceModel\Coupon $result
     * @param \Magento\SalesRule\Model\Rule $rule
     *
     * @return \Magento\SalesRule\Model\Rule
     */
    public function afterUpdateSpecificCoupons(
        \Magento\SalesRule\Model\ResourceModel\Coupon $subject,
        $result,
        \Magento\SalesRule\Model\Rule $rule = null
    ) {
        if ($rule) {
            //update the generated and the expiration date
            $rule->setData('expiration_date', null);
            $rule->setData('generated_by_dotmailer', '1');
            $subject->save($rule);
        }

        return $rule;
    }

How to reproduce

dotmailer-db-1

dotmailer-rules

dotmailer-rule-save

dotmailer-db-2

From my point of view you should check if DotMailer is enabled. There's a helper being defined in the constructer method, but it's not being used in the class. And you should not change the rule, but only the coupon.

If you take a look at how they save date for the coupon in \Magento\SalesRule\Model\ResourceModel\Coupon::updateSpecificCoupons you'll notice that they use the update method. So I think it's also better to use this code in the plugin of DotMailer

            $this->getConnection()->update(
                $this->getTable('salesrule_coupon'),
                $updateArray,
                ['rule_id = ?' => $rule->getId()]
            );
simon-letch commented 5 years ago

Hey @jasperzeinstra,

We're taking a look at this during our current sprint. I'll update again once we've investigated. From a quick initial glance, I can't see why this plugin is required at all.

Simon

simon-letch commented 5 years ago

This has now been released in v3.0.3.