I have used a few deprecated functions that will be removed in D9 those should all be replaced.
Those include: db_insert & db_update

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

guy_schneerson created an issue. See original summary.

Nikhilesh Gupta’s picture

Assigned: Unassigned » Nikhilesh Gupta
Nikhilesh Gupta’s picture

Assigned: Nikhilesh Gupta » Unassigned
steveoliver’s picture

Assigned: Unassigned » steveoliver

Taking this on.

steveoliver’s picture

Assigned: steveoliver » Unassigned
Status: Active » Needs review
FileSize
18.41 KB

This cleans up the API and .install database calls. Also cleaned up the API with tighter functions, re-using one once and making the names of the methods more intuitive.

See PR: https://github.com/steveoliver/commerce_stock/pull/1/files

Patch attached.

kscheirer’s picture

Status: Needs review » Reviewed & tested by the community

Patch applies cleanly and looks like a great improvement - the refactoring of method names is much clearer. I had a few comments on the pull request but they're all minor.

guy_schneerson’s picture

Thanks steveoliver and sorry its taken me so long to get around to this.
Patch looks great and will go over it in detail. I agree the renaming is a good idea, some of the function names got a bit out of control, however we should have probably separated to another issue as not all instances got renamed this includes the interface the class is implementing, but no big deal, I should be able to figure it out. Great work and thanks

guy_schneerson’s picture

May have jumped the gun, I think I was mixing the getStockLevel in the StockManager and the renamed getTotalStockLevel in the StockStorageAPI.

guy_schneerson’s picture

Status: Reviewed & tested by the community » Needs work

Hi A couple of issues I have found:

1) Getting stock level stopped working
The renamed getStockLocationLevel into getLocationsStockLevels had code removed, correlates to your github comment "The previous code was calculating the real-time current quantity across all stores by combining the computed levels with the as yet un-processed transactions' quantities. Is this something we really want to be doing?"
A: Yes it is essential to total the "un-processed" transactions as the real stock is the cs_inventory_location_level (processed) + total of unprocessed transactions from cs_inventory_transaction. I can also confirm that the stock widget is no longer showing the correct number.

2) Calculating the location level will not work correctly first time for a product.
Haven't tested this but looks like the logic changed a little, but easy to fix and breaking it down to smaller functions is great.
The original function started of by checking if the cs_inventory_location_level exist and if not insert it with zeroes and later on did an update.
The new function uses the same condition at the end and if found updates otherwise inserts the record with zeroes, at this point it should insert the values used in the update ($qty & $last_txn).
so replace:

<?php
      $db->insert('cs_inventory_location_level')
        ->fields(['location_id', 'variation_id', 'qty', 'last_transaction_id'])
        ->values([$location_id, $variation_id, 0, 0])
        ->execute();

?>

with

<?php
      $db->insert('cs_inventory_location_level')
        ->fields(['location_id', 'variation_id', 'qty', 'last_transaction_id'])
        ->values([$location_id, $variation_id, $qty, $last_txn])
        ->execute();
?>
guy_schneerson’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
18.68 KB

I have made the two changes. updated patch attached.
and tested both the calculation of available stock and the cron job that sets cs_inventory_location_level. Both work correctly and the user always gets the corect available stock.

guy_schneerson’s picture

Status: Reviewed & tested by the community » Fixed

committed the code. Thanks again @steveoliver for your help with this :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.