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

Introduce SolidusPromotions #5805

Open
wants to merge 642 commits into
base: main
Choose a base branch
from

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Jun 26, 2024

Summary

This imports the solidus_friendly_promotions into the main solidusio/solidus repository, including all history.
This has been done using git filter-branch.

This is a big pull request, because it keeps the entire git history of the solidus_friendly_promotions repository. I encourage reviewers to review the last few commits, but better yet: Try it out on your stores.

See the README file as well as the MIGRATING.md file for instructions how to move from the legacy promotion system to solidus_promotions.

This gem supports

  • stackable promotions
  • much better code readability
  • the old and the new admin
  • creating discounted line items

And many more things. It should be well-linted, and has an extensive test suite.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

tvdeyen and others added 30 commits November 10, 2023 12:19
…ie-quantity

Allow changing quantity for automatic line items
Without the description, the calculator name might not tell you enough
about how the calculator works. This is especially true for the tiered
calculcators.
So you get a percentage based on how many items that match the criteria
the customer has in their cart!
The default promotion adjuster has been renamed, and that has to be
reflected in the README and in the migration guide.
We want user's to take advantage of semantic versioning.
This adds a documentation section so people know they have to change
explicit references to the legacy system's promotion handler.
…ry-condition

Fix admin sub menu entry visibility condition
New standard version, new rules, new fixes.
This gem allows recalculating complete orders until they are shipped,
because Solidus allows changing an order's contents until the order is
shipped.

This change adds a configuration option that disables the
recalculation of orders after they are completed. Some shops make
changes to existing promotions, and expect that this does not change
complete, but unshipped orders.
…ion-after-complete

Add configuration option: `recalculate_complete_orders`
In Rails 7.1.3, the `table_name_prefix` method is redefined if it is not
already defined. Since we have multiple module definitions, we need to
make sure that the class method is defined before we load `engine.rb`.
…ix-rails-7-1-3

Define SolidusFriendlyPromotions.table_name_prefix early
Revert "Temporarily disable running specs for Solidus main"
This was somehow missed in the past.

Fixes solidusio#93
Also fix the spec name so it actually runs.
This reflects better what it does.
This looked a bit garbled without these extra w-100s.
Solidus Core now ships with a Null promotion handler.
We'll just be using Spree.solidus_version instead, like all the other
gems.
This is to indicate clearly that this JS is used for the backend UI.
This allows developers to more clearly see what they need to do in order
to implement custom promotion benefits.
This should give developers better hints as to how to use this gem.
As pointed out in
https://github.com/solidusio/solidus/pull/5383/files#r1391519251, the
new promotion code batch builder had worse performance characteristics
than the one in the legacy promotion system. This gets the
characteristics back to those of the legacy promotion system.

Time spent and memory usage still go up significantly and linearly with
the number of batches. I ran some specs with the following code:

```rb
  context "with a very large number of promotion codes" do
    let(:number_of_codes) { 10000 }

    it "creates the correct number of codes" do
      puts Benchmark.measure { subject.build_promotion_codes }
      expect(promotion.codes.size).to eq(number_of_codes)
    end
  end
```

With a 1000 codes, I measured:

```
Randomized with seed 49097
  1.036322   0.028977   1.065299 (  1.074396)
```

With 10000 codes, I measured:

```
Randomized with seed 33250
  9.606364   0.278920   9.885284 (  9.978242)
```

Memory usage went up linearly as well.
@mamhoff mamhoff force-pushed the import-solidus-friendly-promotions branch from 64f18c1 to a2afec0 Compare September 2, 2024 20:38
@jarednorman
Copy link
Member

I keep coming back and looking at this PR... and I'm still both excited and intimidated by it.

@mamhoff
Copy link
Contributor Author

mamhoff commented Sep 3, 2024

I keep coming back and looking at this PR... and I'm still both excited and intimidated by it.

Is there anything I can do to augment the excitement and reduce the anxiety?

@jarednorman
Copy link
Member

I probably should just take it for a spin on a store.

@kennyadsl
Copy link
Member

@jarednorman my anxiety level dropped after a very useful 1-hour session live with @mamhoff. 😄 BTW, testing this on a real/demo store could be a good way for getting the approve button pressed, which I'm going to do now.

@tvdeyen
Copy link
Member

tvdeyen commented Sep 4, 2024

@kennyadsl @jarednorman @mamhoff would it be helpful to record a session? So that the community also has a chance to get some insights on the design decision made here?

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use this in a real live store with lots of volume and personally have been attending countless pairings and reviews with @mamhoff during the last couple of months.

I am hitting the precious approve button now as well.

Amazing work Martin. I cannot add enough emojis to show my gratitude.

@jarednorman
Copy link
Member

would it be helpful to record a session?

That would be amazing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:repository Changes to the repository not within any gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants