Issue #2710107: Availability responses as value objects with quantity range by steveoliver · Pull Request #593 · drupalcommerce/commerce

steveoliver · 2016-12-31T00:00:48Z

steveoliver

The main change is in classed responses, and returning min and max values for available and unavailable responses. Min and Max are the ~~widest~~ narrowest range of all indicated availability.

An explicit "unavailable" return from any checker will automatically return unavailable for the entire check.

No opinion (Neutral) responses are still ignored / passed on from AvailabilityManager::check() as available.

Added the availability manager service to add to cart form, originally thinking we'd use it within the core add to cart form, but instead not doing anything, but providing it and the currentUser(), selectStore() methods, plus the base form id as conveniences for hook_form_alter implementations.

steveoliver · 2017-01-22T21:52:05Z

+   */
+  public function getCurrentUser() {
+    return $this->currentUser;
+  }


providing this public method for the convenience of form alters.

steveoliver · 2017-01-22T21:53:30Z

+   */
+  public function getAvailabilityManager() {
+    return $this->availabilityManager;
+  }


this also for the convenience of add to cart form alters.

steveoliver · 2017-01-22T21:54:07Z

   *   The selected store.
   */
-  protected function selectStore(PurchasableEntityInterface $entity) {
+  public function selectStore(PurchasableEntityInterface $entity) {


also made AddToCartForm::selectStore() public for the convenience of form alters.

@bojanz This works, but I wonder if we want to provide a custom form alter hook for add to cart forms, passing store and user explicitly. What do you think?

I don't like this. People should be providing their own add to cart form if they're altering this much.

I'm sad that this function is protected now. Current cart form behavior is to show "add to cart" no matter what the availability is. So it can say out of stock, you can click the button and it doesn't add the item to the cart. I Am using hook_form_alter and the enhanced $form_object->getAvailabilityManager()->check of this PR to change the button to "Unavailable". To do that, I need to construct a $context, which requires the $store. This seems like a reasonable use of form_alter to me, if the stock AddToCart form isn't going to be availability-aware.

steveoliver · 2017-01-23T09:20:39Z

@bojanz, Check out the latest fails ... why should ->refresh() change qty from 2 to 1?

olafkarsten

Some nitpicks. :/

olafkarsten · 2017-01-24T10:16:25Z

-   * @return bool|null
-   *   TRUE if the entity is available, FALSE if it's unavailable,
-   *   or NULL if it has no opinion.
+   * @return \Drupal\commerce\AvailabilityResponseInterface


should be: \Drupal\commerce\AvailabilityResponse\AvailabilityResponseInterface

(I ended up just moving all the AvailabilityResponse* classes into the root commerce namespace.)

This is changing the interface of tagged services, so breaks API.

olafkarsten · 2017-01-24T10:16:42Z

   *
-   * @return bool
-   *   TRUE if the purchasable entity is available, FALSE otherwise.
+   * @return \Drupal\commerce\AvailabilityResponseInterface


should be: \Drupal\commerce\AvailabilityResponse\AvailabilityResponseInterface

I ended up moving these AvailabilityResponse* classes into the root Drupal\commerce namespace.

olafkarsten · 2017-01-24T10:17:17Z

-   * Checks the availability of the given purchasable entity.
+   * Gets an availability response from the aggregate of all checkers.
+   *
+   * Possible \Drupal\commerce\AvailabilityResponseInterface classes:


should be: \Drupal\commerce\AvailabilityResponse\AvailabilityResponseInterface

BBGuy · 2017-01-24T17:44:08Z

+        if ($response instanceof AvailabilityResponseNeutral) {
+          continue;
+        }
+        $has_opinion = TRUE;


I think this should be reversed
$min = max($min, $response->getMin());
$max = min($max, $response->getMax());
As its I think it in consistent with:
if ($response instanceof AvailabilityResponseUnavailable) {
return $response;
i.e. it is the interaction of the availability checkers.

Real life example:
stock module says: max: 500
Max per order module says: max: 10 (per order)
In such case I think the maximum should be the min of the two: 10

steveoliver · 2017-01-24T17:55:32Z

Thanks @olafkarsten. Working on getting tests passing now, and after discussion with @BBGuy about his min/max comment, I agree that makes sense, and will make that change as well.

ransomweaver · 2017-01-25T03:09:04Z

+            ]), 'warning');
+            $order_item->setQuantity($availability->getMax());
+            $order_item->save();
+          }


I think I get why there is no check for ->isAvailable here, but I will walk through how this would work for the custom backorder availability checker I think I need to implement.

my checker should run first, and in the case that the item has 0 stock but can be backordered, we return an Availability response with isAvailable == true, and we might think that is all we need to do, but..

that isAvailable TRUE doesn't tell us anything about what amount of stock it means to be available, so even though our checker knows that it means "any amount" we have to explicitly answer with a large getMax in this case.

the edge condition here, for me, is a order item that has positive stock (deliverable immediately) AND can be backordered (N-time delivery). If, for instance, stock was 2, then 2 items were put in cart A, but not checked out, then someone else added 1 item to cart B and checked out. Cart A refresh will still (correctly) get an isAvailable response here, with a very high max. However by default there is no way to tell them that now since stock is 1 not 2, one of the items won't deliver immediately anymore. That is probably the task of a custom OrderProcessor just for updating e.g. a custom OrderItem delivery_time field and maybe dsm the customer.

I think you are got a few separate issues (although I can c the link).
Yesterday me and @steveoliver covered some of this. I explained the idea behind stock that separated the "is in stock" & the "get total stock level" methods. That was to allow cases like yours. However as the availability checker is passed in the requested quantity, it can simply return that as the max to get around the issue. Steve has shown me its a technique he already used. I will try and address your other points in the drupal issue q as it does not directly relate to the code.

I don't think with the current code in this PR here we're providing a way to support backorder-ability in Commerce core--the AvailabilityManager::check returns neutral, available, or unavailable. Backorderable is arguably a fourth possible return value between available and unavailable. This PR needs work to support back-orderability (based on commerce_stock or any other availability-determining contrib module), if we are to support it in Commerce core -- and I think we should. @bojanz What do you think about Commerce core supporting the logic of backorderability?

With the use of the commerce_stock_backorderable module I posted in a sandbox, I seem to have a properly functioning backorder stock system while using this PR. Seems to me the question should be submodule vs standard support in core stock, But maybe a review of my submodule would indicate if there are some inherent flaws to that approach as a general-purpose submodule vs a project-level custom module.

Ah, you're right - this can work! I reviewed the sandbox module and it does look like it'll need some changes in commerce_stock such as the StockCheckInterface::getOrderStockTransactionSum addition, but I think we're headed in a good direction. I'd open a PR against commerce_stock for backorderable as a submodule, that way you can start writing tests against the commerce_stock for the changes to stock and the addition/integration of backorderable. I think this PR is ready for a real review from @bojanz.

@ransomweaver Are you still working on/using backorderable? I'd like to know how this PR affects your use cases.

@steveoliver Yes, I'll check this out and see what it does for backorderable.

@steveoliver commerce_stock_backorderable definitely depends on this PR. Its checker returns Drupal\commerce\AvailabilityResponse rather than a boolean. After some tweaks, everything about backorderable seems to work properly with the latest version of this PR.
In another comment here I've noted that I have a separate issue depending on this PR (https://www.drupal.org/node/2847529) having nothing to do with backorderable. This add-to-cart form availability issue I have chosen to solve with a hook_form_alter in commerce_stock_local, as that seems the best place for it. I will turn that code into a PR for commerce_stock.

ransomweaver · 2017-05-03T03:14:53Z

This will no longer apply as a patch due to the change in the AddToCartForm constructor. Rebase please.

olafkarsten · 2017-05-03T04:37:35Z

   */
  public function check(PurchasableEntityInterface $entity, $quantity, Context $context) {
+    $min = .0000001;
+    $max = 9999999;


I remember how someone discribed a use case where several millions were a common quantity in cart. I don't remember the details, but the learning: never assume 9999999 .. is absurdly large ;). Since than I use PHP_INT_MAX for such cases.

…ean.

…bclasses.

…rrides.

… and max.

steveoliver · 2017-05-03T16:26:41Z

Rebased and PHP_INT_MIN and PHP_INT_MAX for min and max.

ransomweaver · 2017-08-15T17:39:26Z

This PR no longer applies as a patch. Can it be rebased?

mglaman · 2017-09-15T20:12:32Z

This breaks on < PHP 7

    1)
    Drupal\Tests\commerce_cart\Functional\AddToCartFormTest::testProductAddToCartForm
    Exception: Notice: Use of undefined constant PHP_INT_MIN - assumed
    &#039;PHP_INT_MIN&#039;
    Drupal\commerce\AvailabilityManager-&gt;check()() (Line: 35)

mglaman

Quick review. I feel like the test coverage should be fine. But we might want more.

mglaman · 2017-09-15T20:17:12Z

   * {@inheritdoc}
   */
  public function buildForm(array $form, FormStateInterface $form_state) {
    $form = parent::buildForm($form, $form_state);


Don't form alters get the form ID passed? And users could use hook_form_BASE_FORM_ID_alter

mglaman · 2017-09-15T20:22:16Z

   *   The selected store.
   */
-  protected function selectStore(PurchasableEntityInterface $entity) {
+  public function selectStore(PurchasableEntityInterface $entity) {


I don't like this. People should be providing their own add to cart form if they're altering this much.

mglaman · 2017-09-15T20:24:19Z


  /**
-   * Checks the availability of the given purchasable entity.
+   * Gets the availability of a given entity with the given context.


We usually have the document block reflect the method name (this doesn't start with getXXX).

Maybe:

Checks the availability of a given purchasable entity with given context.

…ring this much.

BBGuy · 2017-10-24T09:53:02Z

+            ]), 'warning');
+          }
+          elseif ($availability->getMax() < $order_item->getQuantity()) {
+            drupal_set_message(new PluralTranslatableMarkup(abs($availability->getMax() - $order_item->getQuantity()), '1 %item is currently unavailable. The quantity in your cart has been updated.', '@count %item are currently unavailable. The quantity in your cart has been updated.', [


This should use the "availability Reason" something like:

drupal_set_message(new PluralTranslatableMarkup(abs($availability->getMax() - $order_item->getQuantity()), '1 %item %reason. The quantity in your cart has been updated.', '@count %item %reason. The quantity in your cart has been updated.', [ '%item' => $order_item->getTitle(), '%reason' => $availability->getReason(), ]), 'warning');

Also the part: "The quantity in your cart has been updated" is not always true as the quantity may have stayed the same but that's less of an issue.

BBGuy · 2017-10-24T09:58:59Z

Made changes to stock and works as expected :) A few small ux issues like not using the $availability->getReason() but I think this type of changes can be made later as they will have no impact on the API/function definition. We should get this in ASAP so we can make the stock changes needed.

bgronek · 2018-02-01T21:13:02Z

@BBGuy @steveoliver @mglaman

It looks like this issue and review process is the current bottleneck for getting stock / inventory support into Commerce 2. As the last activity was some time ago, I thought I'd ask to see if there was anything I could do to help here.

joachim-n · 2018-02-01T21:41:45Z

One major problem with the PR as it stands is that it's breaking API.

Rename amount to quantity, like the terms used in all other places in this file.

BBGuy · 2018-03-09T12:33:09Z

After seeking to Bojanz and finding it difficult to justify the minimum values of the response. I have spent some time rethinking this aprouch.
The reason is that while products may have a minimum and maximum quantities that they can support, that range is not continues. take for example this Electronic Capacitor that can only be ordered in multiples of 5.
I now believe that the solution is to allow the availability checker to provide a number that's valid for purchase alongside an optional message that will give context. This will allow for a module to react on the above product with a request of 17 by returning 15 and a message of "Product CA05668 needs to be ordered in multiples of 5. We have altered your quantity from 17 to 15".

This however leaves one other issue. Commerce needs to know if to disable the add to cart button. If we use the above logic and provide 1 to the checker, then the module may return 0 and the button will be disabled. I think that this is a different type of availability question.
If the first question is: "Can the user purchase quantity X of Product Y?"
This second question is "Is product Y available for purchase?"

This second question is a simple Yes/No question. I now think that both those questions are needed for a flexible implementation of the availability manager and as far as I can see it leaves no use case were a product is available unsupported. This does not include back order and wait list as I think those should be categorized as unavailable and that's a whole new story.

bojanz force-pushed the 8.x-2.x branch 2 times, most recently from 68e6e5f to ac287b7 Compare January 12, 2017 04:31

steveoliver force-pushed the 2710107-availability-response branch 3 times, most recently from cbb9eb4 to 7c3b692 Compare January 19, 2017 18:20

steveoliver mentioned this pull request Jan 20, 2017

Issue #2699611: Follow Drupal Commerce Availability Responses BBGuy/commerce_stock#12

Closed

steveoliver commented Jan 22, 2017

View reviewed changes

bojanz force-pushed the 8.x-2.x branch from ab45e11 to 04e2e17 Compare January 23, 2017 03:18

olafkarsten reviewed Jan 24, 2017

View reviewed changes

BBGuy reviewed Jan 24, 2017

View reviewed changes

steveoliver force-pushed the 2710107-availability-response branch from 6af3bc7 to 6f0a329 Compare January 24, 2017 19:28

steveoliver changed the title ~~Issue #2710107: Availability as a value object (range), not boolean~~ Issue #2710107: Availability responses as value objects with quantity range Jan 24, 2017

ransomweaver reviewed Jan 25, 2017

View reviewed changes

bojanz force-pushed the 8.x-2.x branch 7 times, most recently from 6be1d5c to b8a7444 Compare April 27, 2017 20:46

olafkarsten reviewed May 3, 2017

View reviewed changes

steveoliver added 6 commits May 3, 2017 09:23

Issue #2710107: POC: Availability as a value object (range), not bool…

c740906

…ean.

WIP: Availability responses as sub-clases of availability response base.

2f84f9e

Model availability responses after abstract AccessResult class and su…

1bf4781

…bclasses.

Provide base form id and public methods for alter hook based form ove…

957cbb0

…rrides.

Coding standards.

8baf087

Do not remove items if availability response is neutral.

d1b1962

steveoliver added 3 commits May 3, 2017 09:24

Return conservative range of availability.

f6862b0

Start from an absurdly large range of availability.

dd63f20

Use PHP_INT_MIN and PHP_INT_MAX for default availability response min…

0beb9be

… and max.

steveoliver force-pushed the 2710107-availability-response branch from d23417a to 0beb9be Compare May 3, 2017 16:26

bojanz force-pushed the 8.x-2.x branch from a047f90 to 4366f38 Compare June 12, 2017 14:58

bojanz force-pushed the 8.x-2.x branch from dc443c7 to e290888 Compare July 27, 2017 12:21

bojanz force-pushed the 8.x-2.x branch from 5b3779b to 2941d08 Compare August 10, 2017 17:37

Merge branch '8.x-2.x' into 2710107-availability-response

5b64402

bojanz force-pushed the 8.x-2.x branch from 53ec70a to 3bc1b88 Compare September 12, 2017 23:36

Merge branch '8.x-2.x' into 2710107-availability-response

d7afff5

PHP_INT_MIN is PHP 7.0.0 or greater

e984ec2

mglaman requested changes Sep 15, 2017

View reviewed changes

steveoliver added 4 commits October 6, 2017 08:53

People should be providing their own add to cart form if they're alte…

7d614cd

…ring this much.

Base form IDs are in form_state.

a66f412

Update docblock of AvailabilityCheckerInterface::check.

3d2a498

Describe them as availability 'checks', not 'requests'.

b32f2fb

ransomweaver mentioned this pull request Oct 13, 2017

Issue #2847529: check stock availability in add-to-cart form and alter button BBGuy/commerce_stock#37

Open

BBGuy reviewed Oct 24, 2017

View reviewed changes

steveoliver added 2 commits February 9, 2018 14:31

Update AvailabilityResponse.php

3080978

Rename amount to quantity, like the terms used in all other places in this file.

Update AvailabilityResponse.php

329fabe

bojanz force-pushed the 8.x-2.x branch from 55280a3 to 4e9a025 Compare June 15, 2018 12:43

bojanz force-pushed the 8.x-2.x branch from b1be14f to 59230dd Compare June 28, 2018 16:59

bojanz force-pushed the 8.x-2.x branch from 807c6e8 to 8898142 Compare October 8, 2018 15:18

Conversation

steveoliver commented Dec 31, 2016

Uh oh!

steveoliver left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveoliver commented Jan 23, 2017

Uh oh!

olafkarsten left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveoliver commented Jan 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveoliver Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ransomweaver commented May 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveoliver commented May 3, 2017

Uh oh!

ransomweaver commented Aug 15, 2017

Uh oh!

mglaman commented Sep 15, 2017

Uh oh!

mglaman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BBGuy commented Oct 24, 2017

steveoliver left a comment •

edited

Loading

steveoliver Feb 7, 2017 •

edited

Loading

BBGuy commented Mar 9, 2018 •

edited

Loading