Promotions remember their base code

[jrochkind]

When multi-promotions were added to Solidus, they seem to have done so in a way that the base code is not stored on the promotion, it is purely a transitory property of the promotion creation process.

This makes it hard to later create more promotion codes using the same base code.

It also means when later looking at the Promotion in an admin edit screen, the first code from the db is shown on screen labelled as 'base code', rather than the base code itself. Which is kinda messy.

This PR:

1. Adds a Promotion#base_code, and changes the controller to record the base code on #create. This was done in the least changes way possible. So various solidus internal API is a little bit odd with this change. base_code should arguably be sent in params as an attribute of the promotion instead of a separate builder argument, and at several other parts of the API, etc.
2. Changes the CodeBuilder so it can be used to add one more code and still get a suffix -- now it applies a suffix even if you're only asking for one code if there's existing codes. (Still no suffix if 1 code and no existing codes).
3. Does not change the backend UI at all (other than fixing base_code to display properly), including does NOT actually provide a backend UI to provide an 'add more codes' . this could be a future step. I think the promotion creation backend UI is kind of confusing for newcomers in general, and possibly needs more work beyond adding an 'add more codes' button.

[bbuchalter]

I personally think adding additional codes after a promotion is created is a great feature and this PR moves us in that direction. ❤️

[bbuchalter]

If these two items are interchangeable, perhaps a migration to set that data on base_code would be helpful?

[jrochkind]

They aren't exactly interchangeable, but there's no good way to write a migration to set that data, because that data simply isn't there -- that's what this PR fixes.

I suppose you could write a migration that looked through all existing promotions, if they have promotion codes and if those codes all begin with the same prefix (assuming that prefix is always followed by an underscore, which the existing CodeBuilder does, but you could hypothetically have a custom CodeBuilder which does something else), then extract that prefix and set it as base_code?

That seemed far too error-prone and weird to try, to me.

[jrochkind]

to be more clear: Prior to this PR, if you look at an existing Promotion, the first promotion code (if any) is shown on screen labelled (labelled incorrectly) "base code".

That's just a mess, but the actual base code was not stored anywhere to display instead. After this change, the actual base code is stored somewhere and can be displayed properly. But for backwards compatibility and with existing promotions that do not have a stored base code, it still falls through to the old behavior.

As I warn in the PR, this change leaves a lot of internal API (class/method) API being kind of confusing. A lot of existing design is... not the choices I would have made. But I intentionally made the smallest possible change to get a properly stored base_code.

If people would rather I change more to make the internal API more reasonable, I certainly can -- but it's very challenging to figure out how to do so in a way that is both backwards compatible and makes the code less rather than more confusing.

[jordan-brough]

@jrochkind thanks for raising this issue!

I've got a slightly different suggestion I'd like to offer on tackling this: What if we introduce a PromotionCodeBatch model?

I very much agree with all of these points:

Prior to this PR, if you look at an existing Promotion, the first promotion code (if any) is shown on screen labelled (labelled incorrectly) "base code".

That's just a mess, but the actual base code was not stored anywhere to display instead.

...

hard to later create more promotion codes using the same base code.

...

The CodeBuilder was I guess written with the assumption that it would only be used on new promotion creation.

...

I wouldn't have the CodeBuilder deciding whether to use a suffix based on how many codes there are at all, I'd have another option.

Along with those, here are a few additional points of pain I've had with the current system:

• Multiple promotion codes on a single promotion should not be required to have the same base code, or even any base code at all. (Which actually argues against having a spree_promotions.base_code DB field)
• The admin can easily time out when trying to generate large batches of promotion codes (e.g. 100,000 codes)

I think we might be able to help with all these issues if we added a PromotionCodeBatch model and update the admin API a bit.

Specifics:

• PromotionCodeBatch Schema:
  • promotion_id
  • base_code
  • number_of_codes
  • status (processing/completed/errored)
• PromotionCodeBatch would not be not required. You can add a code to a promotion without it being in a batch (makese sense, imo, and helps with backward-compatibility)
• PromotionCodeBatches can be processed asynchronously. And since they're in the DB we can provide an admin UI on their status.
• Provide a different admin UI for generating single codes vs batches of codes and send them in separate controller parameters
• Don't use CodeBuilder for single codes
• Deprecate calling CodeBuilder with num_codes == 1 instead of changing the behavior (and in the future make it do the same thing for == 1 as it does for > 1)
• Deprecate :base_code as a parameter to PromotionBuilder instead of changing the behavior

Thoughts? I know that's a much larger request, but personally I'd rather not cement the idea of spree_promotions.base_code as a data point, and this would be helpful to our store so I'd be happy to collaborate on it or try taking it on ourselves if people think it's an OK idea.

[jhawthorn]

Other than a small nitpick I am strongly 👍 on merging this.

I like Jordan's proposal for a PromotionCodeBatch model, but I think it is out of scope for what this proposes to fix. I think PromotionCodeBatch would work well with a major rework of the code generation, as it implies some background processing, and begs for some other features too (pluggable code generation?).

I think this is a quick fix which should have always been included. If we ever get to creating PromotionCodeBatch, it will be easy to move the stored base_code over to the new model.

[jhawthorn]

This should avoid modifying the hash passed in.

[jrochkind]

good call. I guess the alternative is promotion_attributes = promotion_attributes.merge(base_code: base_code). Is that what makes sense?

[jrochkind]

done

[jrochkind]

Yep, I agree with @jhawthorn, this is basically meant to fix what I think was an omission in the implementation of the multi-code feature, it should have included this to begin with. Just fixing it in the simplest way possible, entirely backwards compatible.

I'll fix the mutation of the input parameter that @jhawthorn pointed out.

If committers don't want to make this fix to the current architecture, insisting instead on a more major refactoring of the feature (perhaps backwards compat perhaps not), you can reject/close this PR. I can't commit to doing that major refactoring myself though, especially cause I've found that major refactorings are unlikely to be approved by committers who will disagree with each other about how it should look. :)

If this does get accepted, I think the next step is some slight fixes to the admin UI, to deal with confusion several people have reported on the slack channel about how to create a promotion with just one code. With a base code stored, the UI can actually tell whether it's a multi-code promotion or a single-code promotion, and adjust labels accordingly to only say 'base code' if it really is a base code.

Really, I think the original multi-code implementation was merged not really finished, just trying to finish it in a backwards compat way.

[houndci-bot]

Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

[jordan-brough]

@vladstoick actually just implemented PromotionCodeBatch for us in our own store and we have been using it for the past week or so. The aim was to upstream it. Can you give us a week or so to see if we can get a PR up? It could avoid the extra back-and-forth of changing this multiple times? If it doesn't look good or if we don't manage to get it up then maybe we go forward with this PR?

[jrochkind]

Do what you gotta do. My use case that inspired this was I want to be able to say "Add N more codes" (where N can == 1!), and get codes added, without the code making this request having to know through some out of band source what the correct 'base code' is.

I'd put handling that use case in as a request for your alternate proposed refactoring, and additionally a clear path for making the admin UI less confusing than it is now, although I don't think that has to be in the PR improving the behind-the-scenes. (Multiple reports of people not understanding how to add a single-code promotion from current admin UI).

[jordan-brough]

@vladstoick is working on upstreaming our work but I think we should be fine to merge this in the meantime. It's a good step forward and is a nice small set of changes.

[jhawthorn]

Closing now that #1524 has been merged