Solidus: A promotion without actions should log errors

[jacobherrington]

Steps to reproduce

1. create a sandbox app
2. start the server and navigate to localhost:3000/admin/promotions
3. create any promotion with a coupon code, but don't add any actions
4. checkout using the promotion code

Expected behavior

The server should log a warning that the coupon does nothing. I feel this way because the promotion is "active." If the admin doesn't want the promotion to attempt to apply, it should be set to inactive. As an admin, I would expect that any "active" promotion will at least attempt to apply itself.

Actual behavior

The coupon gives an error: "The coupon code you entered doesn't exist. Please try again."

That error message is totally fine for the frontend as someone shopping on a Solidus store shouldn't see that they tried to use an empty promotion, but the server should log something more useful to the admin. e.g., "Someone tried to use an empty coupon code..."

This way the admin isn't confused as to why an active coupon isn't applied.

System configuration

Solidus Version:
Using master

[jacobherrington]

There is also an argument to be made that it shouldn't be possible to create a promotion with no actions.

[eyewritecode]

Is anyone working on this? @jacobherrington @kennyadsl? i would like to give it a try

[jacobherrington]

@eyewritecode go for it!

[eyewritecode]

@jacobherrington I reproduced the "error" and just wanted to make sure i understand the issue here! When you say the server should log a warning... do you mean something like

logger.info("some text")

As for this

There is also an argument to be made that it shouldn't be possible to create a promotion with no actions.

I think it makes more sense too

[jacobherrington]

Yes, something like that.

I think the second suggestion is a larger change and would probably need a little more discussion, but do what you think is best! 😄

[EduardoGHdez]

Does this was already solved? @jacobherrington

There isn't another PR referring to this issue.

[jacobherrington]

@EduardoGHdez I don't think it has been solved! Would be great if you could open a PR! :)

[EduardoGHdez]

Sweet, I'll work on it!
Thanks

[kennyadsl]

I know we already discussed this but thinking twice I'm not sure how this proposal would solve the issue since generally, admin users cannot access server logs. Or am I missing something?

[johnny243]

I know we already discussed this but thinking twice I'm not sure how this proposal would solve the issue since generally, admin users cannot access server logs. Or am I missing something?

Agreed. I think we should somehow force the admin user to specify an action OR keep the promotion in a "pending" status, so they'll know the promotion is not ready.

[wildbillcat]

Hi there, I was interested in giving this a try in my free time. However as a solution instead of logging, I'd like to force validation on the promotion model that the promotion either be disabled, or have an associated action. So the situation is avoided to begin with, admin user is warned that they can't activate without an action, but can save their work and come back to it.
If that meets with general approval, I'll see if I can whip something up.

[kennyadsl]

Thanks @wildbillcat, I think it's a good solution to explore but we should keep backward compatibility. At the moment existing stores may be relying on not having a validation error saving existing promotions without actions and that should be kept some way, at least for existing stores/promotions.

[wildbillcat]

Is there a use case for an active promotion that doesn't have an action?

[wildbillcat]

We could put in the application configuration whether or not to have the validation enabled for backward compatibility. The question the is should the default be enabled or disabled? I'd personally vote for enabling.

[kennyadsl]

I think that having an app configuration is a good way of changing that preserving backward compatibility. We can keep it enabled in new apps and disabled for old ones so that existing stores are not affected when upgrading but who cares can change the preference. We've done something similar here: https://github.com/solidusio/solidus/pull/3456.

[wildbillcat]

Thanks for the example. So for the definition of work:
Force validation on the promotion model that the promotion either be disabled, or have an associated action. Amend the application configuration to allow this validation to be disabled for backwards compatibility.
Does that look ok?

[kennyadsl]

Works for me but I'd wait to try changing some code before giving for granted that this can be done. Maybe it's easier to just exclude the promotions from the eligible ones if it doesn't have an action associated.

I'm a bit worried about adding a model validation since promotion action can be added in the second step of promotion creation. So the validation can't be on: :create. This means that we should create new promotions disabled by default, or create a new disabled state that changes when at least one action is present.

Just saying that I think it's better to spike a little bit before agreeing on a path upfront.

[wildbillcat]

No worries, just wanted to confirm my inital plan sounded sane, should it work haha.

I'll try making up a draft PR for your review.

[wildbillcat]

Hey there, I have a first draft whipped up over at https://github.com/solidusio/solidus/pull/3596

You were right on implementation varying. Originally I made another brand adding a disable function that resulted in a small migration. But after spending some time thinking about a less disruptive means of getting change, I opted to embrace the inferred state and extended the active scope and active? methods, while adding the legacy support through the config file. Still need to loop back on documentation and tests, but let me know what you think of the general implementation idea and if it make sense to you.

[DanielePalombo]

I have forked https://github.com/solidusio/solidus/pull/3596 and I have made some improvements on https://github.com/solidusio/solidus/pull/3749 .

[seand7565]

With #3749 merged, can we close this issue? @kennyadsl