[4.x][5.x]: Tidy up UserException thrown during pending user purge?

[samhibberd]

What happened?

Could the UserException thrown during the pending user purge be reworked, we use sentry to monitor exceptions and this has added ~100k issues to our usage:

commerce/src/services/Orders.php

Lines 188 to 200 in 8f6a112

	public function beforeDeleteUserHandler(ModelEvent $event): void
	{
	/** @var User $user */
	$user = $event->sender;
	// If there are any orders, make sure that this is not allowed.
	if (Order::find()->customerId($user->id)->status(null)->exists()) {
	// TODO revise this stop-gap measure when Craft CMS gets a way to hook into the user delete process.
	throw new UserException(Craft::t('commerce', 'Unable to delete user {user}: the user has a Craft Commerce order.', [
	'user' => $user->id,
	]));
	}
	}

As the user is correctly not being deleted as the user (customer) has an order, could this not be handled - the activation code / state be removed, or event added to the user purge logic so commerce could hook into that and exclude users that have an order?

Also is there a reason you don't return theisValid = false and intead throw the excpetion?

You may set [[\yii\base\ModelEvent::$isValid]] to false to prevent the element from getting deleted.

Impacts both 4.x and 5.x

Craft CMS version

4.10.8

Craft Commerce version

4.6.7

PHP version

No response

Operating system and version

No response

Database type and version

No response

Image driver and version

No response

Installed plugins and versions

[linear]

PT-2161 [4.x][5.x]:

[samhibberd]

Thanks @nfourtythree @lukeholder

[nfourtythree]

Hi @samhibberd

Commerce versions 4.6.13 and 5.1.3 have now been released with this fix included.

Thanks!

[samhibberd]

Actually @lukeholder @nfourtythree thinking about it, this doesn't change much for us (as in we still push craft errors to Sentry).

Would it be fair to argue in this case that this is not actually an error, or a warning, it's doing exactly what it should, prevent the deleting of an user that has an order. Should the logging level not be set to info?

[peteeveleigh]

Agree with @samhibberd. This shouldn't be logged as an Error. It's Info or, at worst, a Warning.

[lukeholder]

@peteeveleigh @samhibberd is is logged as info now: https://github.com/craftcms/cms/pull/13494/files#diff-b306bfe80cb67a6f20bb99a1019385d006e706dc7033449406686bf8a83f3026R1246

Let me know if you have any other questions.

Thanks

[lukeholder]

Sorry just realised you were referring to our logging in Commerce.

I have gone ahead and fixed that for the next release in 5.3.x

To get the fix early, change your craftcms/commerce requirement in composer.json to:

"require": {
  "craftcms/commerce": "5.x-dev#5a31da5714f7442f5fd864274ebaccbf51a95db3 as 5.3.0.2",
  "...": "..."
}

Then run composer update.

We will update this ticket once the release is out.

[nfourtythree]

Commerce version 5.3.1 has now been released with this fix included.

Thanks!

[samhibberd]

Will this make it into the 4.x branch @nfourtythree ?

[nfourtythree]

HI @samhibberd

Yes this is in the 4.x branch with this commit: fee301f

It will be included in the next release of Commerce 4.

To get this early, change your craftcms/commerce requirement in your project's composer.json to:

"require": {
  "craftcms/commerce": "4.x-dev as 4.8.0.1",
  "...": "..."
}

Then run composer update.

Thanks!

[nfourtythree]

Hi

This update has now been back ported to Commerce 4 in version 4.8.1 which has just been released.

Thanks!