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

Feature: Order item unit generation mode #15414

Open
wants to merge 27 commits into
base: 1.13
Choose a base branch
from

Conversation

senghe
Copy link
Contributor

@senghe senghe commented Oct 6, 2023

Q A
Branch? 1.13
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Related tickets resolves #10155
License MIT

Hi all! :) I'm preparing a new functionality, which will resolve an issue with attaching order item (unit) promotions into carts, when having a lot of quantity there. This is currently a VERY EARLY version of the feature, but I appreciate your comments on that.

The main idea is to introduce wholesale variant checkbox, which will result only one order item unit for that variant, independently from the quantity.

I hope you like it as I am! 🖖

@senghe senghe requested a review from a team as a code owner October 6, 2023 12:36
@probot-autolabeler probot-autolabeler bot added the Admin AdminBundle related issues and PRs. label Oct 6, 2023
@senghe senghe marked this pull request as draft October 6, 2023 12:40
@github-actions
Copy link

github-actions bot commented Oct 6, 2023

Bunnyshell Preview Environment deployment failed

Check https://github.com/Sylius/Sylius/actions/runs/6737013256 for details.

Available commands:

  • /bns:deploy to redeploy the environment
  • /bns:delete to remove the environment

@diimpp
Copy link
Member

diimpp commented Oct 6, 2023

Hi,

I've had this case in one of my projects before, company was selling by tons, but price was defined per KG on variants, which lead to massive performance hits with thousands/tens of thousands of OrderItemUnit per order.

I don't have source codes under a hand to refresh my knowledge, but solution was not to use OrderItemUnit at all, we've introduces "Unit of Measure" on variants with values like (KG, Metric ton, Metre, Kilometre, Litre, etc) and switchable mode of work via native OrderItemUnit or via OrderItem only. As far as I remember it was quite limited amount of work, thought probably not feature complete, like ShipmentUnit's were probably not updated.

Additionally, I think there were times in sylius history, when OrderItemUnit didn't exists at all, but Order/OrderItem were (Pre 1.0?)

So this can be considered as alternative solution.


For the current PR,
calculation mode for Order\Model and Core\Model shouldn't be derived from wholesale/b2c/b2b/business terms, but rather should be true to universal Order/OrderItem/OrderItemUnit/Adjustment terms, for example calculation mode. ($item->getCalculationMode())

Given that sylius is ecommerce framework first and ecommerce app second, IMO we can/should support this in sylius-framework level, but not actually implement in sylius-app as app tends to be more of B2C specialized and leave wholesale solution up to end-users/some b2b plugin.

For example sylius supports multiple partial payments per order, but has never implemented it in sylius-app, so I think this the same case.

@senghe
Copy link
Contributor Author

senghe commented Oct 9, 2023

Hi @diimpp, thanks for your point of view.

I agree with you - the wholesale belongs mostly to B2B cases, but it's the solution to resolve #10155. My intention is not to prepare Sylius to handle B2B cases, but resolve an issue, which happened when some users used the current mechanism, which doesn't scale.

Regarding to non-unit solution, it's not quite easy to do so, as Sylius needs order item units i.e. for shipping cases. There is some cases we can handle on order item level (promotions, taxes). In some Sylius Plus cases like returns management and split shipping it also is based on order item unit.

So, long story short, I just want to fix a performance bug only.

@probot-autolabeler probot-autolabeler bot added the API APIs related issues and PRs. label Oct 12, 2023
@bitbager
Copy link
Contributor

A few cents from my end - the wholesale keyword is not related to B2B alone. This could also refer to an e-grocery industry, where it's pretty common to use it for the B2C customer too. By the way, this is also the industry that suffers most from this issue.

I do understand your point of view, many may in fact misrecognize this keyword. Top label synonyms by GPT:

  • Bulk sale
  • Collective sale
  • Mass sale
  • Extensive sale

@diimpp
Copy link
Member

diimpp commented Oct 15, 2023

I do understand your point of view, many may in fact misrecognize this keyword. Top label synonyms by GPT:

Yep.problem with wholesale it's a domain term, while PR makes changes to application code level.


image

Sylius already exposes Order Items/Order item units as admin user known concepts, so how about this:

  • At channel (I can imagine some people don't want to set this value on each variant)/product variant we can add Adjustments application strategy along side with alert-box with explanation or/and link to the docs.
  • At Order/Model / Core/Model rename it to calculationStrategy.

There is a bit of mismatch as Model calculationStrategy is mostly about units creation and price calculus, while adjustment application strategy can be implemented without units change from this PR, but still it sounds better to me.

P.S. As for mentioned alert-box, it can mentioned, that Order item units based mode allows for individual adjustments differ per unit (Which sylius doesn't do to my knowledge, but still a possibility), but will load memory/cpu on 1k+ sets and Order item based mode as opposition will allow for less flexibility, but can handle unlimited quantities.

@senghe
Copy link
Contributor Author

senghe commented Oct 16, 2023

Okay, so I think I can go with changing the "Wholesale" keyword everywhere to "Order Item Unit generation mode".
The option will have possible values: "Single Order Item Unit", "Multiple Order Item units". and still would be settable where the wholesale currently is - in inventory tab of product variant form. So we don't keep the B2B naming convention, but the function will work as I expected.

I don't want to resolve the issue on order item level (allow to have no order item units in item), as there is next issue to be resolved: shipments, which bases on order item unit level. So there should be at least one order item unit to be consistent with that.

But still I've got one issue with promotions and adjustments, which is not covered yet.

@diimpp please review my comment 🙏

@probot-autolabeler probot-autolabeler bot added the Shop ShopBundle related issues and PRs. label Oct 17, 2023
@diimpp
Copy link
Member

diimpp commented Oct 17, 2023

Okay, so I think I can go with changing the "Wholesale" keyword everywhere to "Order Item Unit generation mode". The option will have possible values: "Single Order Item Unit", "Multiple Order Item units". and still would be settable where the wholesale currently is - in inventory tab of product variant form. So we don't keep the B2B naming convention, but the function will work as I expected.

I don't want to resolve the issue on order item level (allow to have no order item units in item), as there is next issue to be resolved: shipments, which bases on order item unit level. So there should be at least one order item unit to be consistent with that.

But still I've got one issue with promotions and adjustments, which is not covered yet.

@diimpp please review my comment 🙏

Hi, let's split our concerns into points:

Best architecture for high quantity item optimization/approach: "Single OrderItemUnit vs No OrderItemUnits at all"

  • Both approaches are drastic change, which means this PR should target 2.0. Refund plugin (By extension any community plugin) is tightly coupled with OrderItemUnit and only 2.0 will save us from endless stream of issues, when end-user will enable optimized calc, but plugin still treats it as regular multi OrderItemUnit order.
  • For the same reason, @lchrusciel needs to be involved before further work will be done.
  • Single OrderItemUnit
    • pros:
      • Partial drop in support for existing functionality
      • ???
    • cons:
      • For the plugins tightly coupled with OrderItemUnits collection, will require fixes in each individual plugin.
      • IMO feels like a workaround as design is adjusted for existing functionality.
      • OrderItemUnit quantity feature is concerning1
      • Units based adjustment application becomes pointless, since there are no units to distribute values like 10 / 3 = 3, 3, 4, which means SingleOrderItemUnit approach not solving the problem, but still introduces workaround for the problem.
  • No OrderItemUnit at all
    • pros:
      • IMO no compromise in design for the partial support of existing functionality.
      • IMO implementation is true to Order/Order Item/Order Item Unit/Adjustment order model skeleton
    • cons:
      • Every unit based functionality (promotion,shipping rules) will need to be updated
      • For the plugins tightly coupled with OrderItemUnits collection, will require fixes in each individual plugin.

Naming

We have three points of interest over here,

  1. Name of the feature visible to end-users (non tech personal as well)
  2. Mode of work with adjustments application (Item, Multi unit, Single unit, No unit)
  3. Mode of work with units calculation (Multi/Single/Non)

It's important to note, that feature 2. and 3. can be done independently.

1.

Our biggest change with this feature is the way adjustments are applied, end-users need to be aware, that the way taxes are applied is no longer true and they won't get individual tax per unit. Single unit/no unit is just a side effect optimization business wise.

So I believe this should be named as "Adjustment calculation (application) strategy" with alert box explaining what's this about similar to already existing
image

P.S. This channel based feature will need to be integrated with Single/No units system

Other option is to ground naming in existing features, like tracked and base strategy selection on tracked: true/false.

2.

For example, promotion doesn't particularly advertise which level it will applied and uses Item name, while internally decides on unit.
image

With this feature it should be applied to item instead, no change in end-user naming.

3.

It's only about in-code naming, a mode work or $calculationMode as changes to Order Item/Order Item Unit are mostly within calculus methods.

Questions

I don't want to resolve the issue on order item level (allow to have no order item units in item), as there is next issue to be resolved: shipments, which bases on order item unit level. So there should be at least one order item unit to be consistent with that.

There is no conceptual issue with mismatch between ShipmentUnit and OrderItemUnit as ShipmentUnit is more grounded in real world, which means single shipment unit can be 10 ton of cold rolled steel or pair of socks or single box with 100 pieces of air freshener. :)

Our options are

  1. Skip generation of shipment units at all
  2. Generate single shipment unit per order item.

Resume

This feature needs to be designed from adjustment application POV, where Single/No units is a desired, but a side effect.

It needs to be tested with Invoice/Refund plugin, it needs to explicitly specify what we're loosing with item based tax application and specification should come from government rules on commerce calculus, which to my knowledge can be different per region.

I hope it helps and adds some perspective 🙂

Footnotes

  1. image

@senghe
Copy link
Contributor Author

senghe commented Oct 18, 2023

Hi @diimpp,

I agree with you. It should be done in another way, including redesigning the architecture etc. But there is a performance issue. which is unresolved from 4 years and blocks people from using the application in some cases. Changing it on they own in the right way you mentioned is an expensive cost, which would be "paid" by every client who have the case.

And you're right. The PR is just a workaround to resolve one of the weakest link: performance issue from #10155 where people use bigger quantities in cart/checkout.

Some pros from my side:

  • I've implemented it with order item unit existence, so most of the plugins won't be broken. They'll treat the "Single Order Item Unit" as one unit. This is the consequence, which should be considered to be taken by people, when decide to use this option.
  • This is just an option. I've implemented this to not to change the basic Sylius mechanisms: the "multiple" item units will have the quantity of 1, so it doesn't change anything in calculation. There are some changes in adjustments calculation etc. but still, as the feature is an optional thing - people can decide whether they're OK with this.
  • I've implemented the function keeping all your behats, so the logic won't produce any BC or calculation bug.

@senghe
Copy link
Contributor Author

senghe commented Oct 19, 2023

I forgot to add translations in admin panel 🤦‍♂️ I've added this with last commit. I think the PR is ready from my side to be merged, if you agree with the concept.

@senghe senghe changed the title [WIP] Feature: wholesale products Feature: Order item unit generation mode Oct 19, 2023
@senghe senghe marked this pull request as ready for review October 19, 2023 08:13
@Geolim4
Copy link

Geolim4 commented May 7, 2024

Hello, what's the status of this PR ?
I'm curious too since I'm struggling a lot too for a project with large amount of cart quantities :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin AdminBundle related issues and PRs. API APIs related issues and PRs. Shop ShopBundle related issues and PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

500 when applying promotion on 9999 item units
4 participants