Modification to order-total "management"

[proseLA]

cindy, is it possible to leave branches of previous releases of this code? everything gets committed to a master branch, and it makes it difficult for me searching through this repo.

in addition, if you are refactoring so much, i think it would be better to do comparisons prior to doing updates. i will give you an example. i create an order on the storefront, it has 1 item, it's marked for pickup in store, it gets taxed. the order comes into the admin. i click edit order, and then update with 0 changes to the order. this simple task creates a debug log of ~23k when nothing was requested. in addition, EO deletes my ot_tax records. i figure out where the ot_tax record gets deleted; address that; it then gets deleted in another place. i address that, now EO is updating my tax description to include the addition of the shipping tax description. what if i do not want that there? figuring out where this was updated seems to be another gargantuan task.

now perhaps this debug log is helpful to you... i find it has too much data to be of value.

perhaps we can set up a different level of logging. write log messages ONLY when updating/deleting a record in the database?

sorry if it seems like i am complaining. but i do not understand why this module would do the math on an orders total correctly (or at least consistently) but at the same time delete an order total record that makes the math seem incorrect.

best.

[lat9]

Since each release is tagged, e.g. the v4.5.7 release file-set can be found here, I see no reason to keep release-specific branches.

I'll grant that I'm refactoring, but what you're requesting in the "check to see if any changes were made" is significantly more than refactoring. That feature will be part of the re-design that removes the unwanted product removal/re-add processing that EO currently uses.

The debug-log generation is also on my list. As you indicate, there's a surplus of information that tends to hide the 'golden nuggets'. My preference would be to refine the information being logged to reduce repetitions, otherwise the log layout will be different for different actions and make it much more difficult to mentally parse.

sorry if it seems like i am complaining. but i do not understand why this module would do the math on an orders total correctly (or at least consistently) but at the same time delete an order total record that makes the math seem incorrect.

Please elaborate.

[proseLA]

Since each release is tagged, e.g. the v4.5.7 release file-set can be found here, I see no reason to keep release-specific branches.

concur. i should have looked at tags.

sorry if it seems like i am complaining. but i do not understand why this module would do the math on an orders total correctly (or at least consistently) but at the same time delete an order total record that makes the math seem incorrect.

Please elaborate.

i will provide an example. with the log. 9% tax. 1 product -> $399.99, walk in. $36 tax. when i click update my ot_tax record gets removed, but the order totals are still the same, so math now does not add up. but i know that the tax is $36.

now if i add a coupon for $100 off, that coupon gets removed off the total. not the subtotal. to me (and i'm sure most accountants can make a similar argument) discount coupons should be applied pre-tax; not post-tax (gift certificates the opposite). i have not found the sort order to be of any relevance as the ot_coupon has a lower sort order than than the ot_tax.

so now, i have an order where the ot_tax record is removed and the value of the tax is based on the pre discount total.

in addition, i have also tested with the ot_coupon having a lower sort order than ot_subtotal, and that also does not address the tax issue. in my experience the sort order is only used for display. (this part of the test is not in the attached log).

finally, the change in commit a0cdd747b was applied prior to this test.

let me know if you need further elaboration. debug_edit_orders_17562.log

[proseLA]

cindy, again,i am not trying to be a pain here. i'm trying to think about what sort of code makes the most sense for the most number of users.

i have always viewed this plugin as a necessary evil. i warn my clients not to use it. its not that i do not think you have made it better, clearly you have. but the more time i spend with ZC, the more inherent problems i see with the structure of the order totals modules. ot_total does nothing other than output some vars that are calculated elsewhere. it each order total module calculated its own thing, i think it would be much better.

in the over 10 years i have played around with ZC, i am finally trying to get my clients to be on the base ZC and work all customizations through the notifier system. easier said than done. but it seems that the issue i have brought up here with regards to the coupon being pre tax or post tax is something that has plagued me for years. i'm now trying to get the ZC team to add the coupon amount to the order->info array:

https://github.com/zencart/zencart/pull/4324

i want that amount so that i can properly recalculate when using EO and incorporate it into whatever tax module they are using.

it just seems too big a step to get an order total module to actually calculate what it is doing.

finally, i am not sure why EO needs to validate coupons that are already part of the order.... perhaps i can reconfigure my code some... i will look into that... but my customized ot_coupon when using EO allows me to enter a code without it being in the DB (my own customization), but EO removes it when updating. whether the current structure of ZC can have a validation method in the ot_coupon class, i'm not sure (currently EO only looks to see if the code is the DB, not whether the ot_coupon class allows it). but perhaps i can play with records in the DB to avoid EO removing it on update. my clients have needs to enter a discounts on orders for customer service issues for whatever reason. i'll look into having it in the DB to see if it avoids EO removing it.

[proseLA]

https://github.com/zencart/zencart/issues/4339#issuecomment-843536619

[lat9]

@proseLA, circling back on this (since I'd really like to get EO v4.6.0 released). Other than confirmation that there's a bunch of information missing in the coupon-related factors for an order, is there an EO problem to be solved?

[proseLA]

@lat9 i'm looking right now... give me a little bit. thanks.

[proseLA]

@lat9,in looking at my code, it seems the only changes that i have are in the extra functions script. i will go over them here:

this one is not related to this problem, but i thought i would share it none-the-less:

https://github.com/lat9/edit_orders/blob/924b7aa315dd654cc8cd678a5503f4f2de9f7714/YOUR_ADMIN/includes/functions/extra_functions/edit_orders_functions.php#L530-L531

right after the start of this method i have:

        if (is_array($country)) {
        return $country;
    }

another unrelated issue, but i have changed the following line:
https://github.com/lat9/edit_orders/blob/924b7aa315dd654cc8cd678a5503f4f2de9f7714/YOUR_ADMIN/includes/functions/extra_functions/edit_orders_functions.php#L625

"SELECT p.*, pd.*

(i use some extra fields).

now to the meat of what i found. i commented out this line:

https://github.com/lat9/edit_orders/blob/924b7aa315dd654cc8cd678a5503f4f2de9f7714/YOUR_ADMIN/includes/functions/extra_functions/edit_orders_functions.php#L1466

i have also commented out this section of code:

https://github.com/lat9/edit_orders/blob/924b7aa315dd654cc8cd678a5503f4f2de9f7714/YOUR_ADMIN/includes/functions/extra_functions/edit_orders_functions.php#L1482-L1489

what i found is that what you are now calling a 'corner-case' is the only time an ot_tax record should be deleted. and that would happen here (NO CODE CHANGE HERE):

https://github.com/lat9/edit_orders/blob/924b7aa315dd654cc8cd678a5503f4f2de9f7714/YOUR_ADMIN/includes/functions/extra_functions/edit_orders_functions.php#L1501

in looking at my comments from this thread, the eo problem was that ot_tax records were getting removed from the orders_total table, and not getting added back in. despite the math being correct.

it seems that leaving those records in the table, eo will properly update the ot_tax records with the correct amount.

i hope that helps. from what i remember much time was spent debugging.

best.

[lat9]

@lat9,in looking at my code, it seems the only changes that i have are in the extra functions script. i will go over them here:

this one is not related to this problem, but i thought i would share it none-the-less:

https://github.com/lat9/edit_orders/blob/924b7aa315dd654cc8cd678a5503f4f2de9f7714/YOUR_ADMIN/includes/functions/extra_functions/edit_orders_functions.php#L530-L531

right after the start of this method i have:

        if (is_array($country)) {
      return $country;
  }

Under what condition is eo_get_country called with an array? The only place that I see that function called is from the editOrders class, based on the (presumed) country-id that's present in the reconstructed order.

another unrelated issue, but i have changed the following line:

https://github.com/lat9/edit_orders/blob/924b7aa315dd654cc8cd678a5503f4f2de9f7714/YOUR_ADMIN/includes/functions/extra_functions/edit_orders_functions.php#L625

"SELECT p.*, pd.*

(i use some extra fields).

Sounds like a good change, but the remainder of the eo_get_new_product function grabs only the fields currently selected and stashes them into the returned array. I've opened #200 to address these changes.

now to the meat of what i found. i commented out this line:

https://github.com/lat9/edit_orders/blob/924b7aa315dd654cc8cd678a5503f4f2de9f7714/YOUR_ADMIN/includes/functions/extra_functions/edit_orders_functions.php#L1466

i have also commented out this section of code:

https://github.com/lat9/edit_orders/blob/924b7aa315dd654cc8cd678a5503f4f2de9f7714/YOUR_ADMIN/includes/functions/extra_functions/edit_orders_functions.php#L1482-L1489

what i found is that what you are now calling a 'corner-case' is the only time an ot_tax record should be deleted. and that would happen here (NO CODE CHANGE HERE):

https://github.com/lat9/edit_orders/blob/924b7aa315dd654cc8cd678a5503f4f2de9f7714/YOUR_ADMIN/includes/functions/extra_functions/edit_orders_functions.php#L1501

in looking at my comments from this thread, the eo problem was that ot_tax records were getting removed from the orders_total table, and not getting added back in. despite the math being correct.

it seems that leaving those records in the table, eo will properly update the ot_tax records with the correct amount.

i hope that helps. from what i remember much time was spent debugging.

best.

For these remaining changed, I'll update the subject of this and hold it until a follow-on release ... I don't have the bandwidth right now to fully review the impact of these order-total related changes.

[proseLA]

Under what condition is eo_get_country called with an array? The only place that I see that function called is from the editOrders class, based on the (presumed) country-id that's present in the reconstructed order.

i implemented that change in 11/2020. not that long ago.

country in ZC has always been confusing. just look at this code in EO:

https://github.com/lat9/edit_orders/blob/924b7aa315dd654cc8cd678a5503f4f2de9f7714/YOUR_ADMIN/includes/classes/editOrders.php#L111-L113

if we presume what you are saying is correct, we start off with country representing a country-id. we then call the function, which returns an array. we then assign that array back to the country var. so now, what started as a string or an integer is now an array. how that might get called later i can not say.

for my clients, country ends up being quite important as i have integrated shipping (and label printing) with a few common carriers into the back end (no ship station for us). and every time i look at it, at one point it's an array; then it's not.

looking at the order class, in the cart it is an array, when you query the order, it is not. but what happens (and this is the most likely scenario) if someone is trying to do what edit orders is doing BEFORE EO does it? via an observer?

that is most likely what i am doing, and why i implemented that section of code.

in fact, i found it. i'm doing the same thing EO is doing via a notifier from the order class.

case 'NOTIFY_ORDER_AFTER_QUERY':
        if (strstr($class->info['shipping_module_code'], 'storepickup')) {
            $class->delivery['firstname'] = MODULE_SHIPPING_STOREPICKUP_COMPANY;
            $class->delivery['lastname'] = '';
            $class->delivery['street_address'] = MODULE_SHIPPING_STOREPICKUP_ADDR1;
            $class->delivery['city'] = MODULE_SHIPPING_STOREPICKUP_CITY;
            $class->delivery['postcode'] = MODULE_SHIPPING_STOREPICKUP_ZIP;
            $class->delivery['zone_code'] = MODULE_SHIPPING_STOREPICKUP_STATE;
            $class->delivery['state'] = 'California';
            $class->delivery['suburb'] = '';
            $class->delivery['name'] = $class->delivery['firstname'];
            // now making use of duplicate function.  not loaded on admin side.
            $countries = $this->zen_get_countries(SHIPPING_ORIGIN_COUNTRY, true);
            $class->delivery['country'] = array(
                'id' => SHIPPING_ORIGIN_COUNTRY,
                'title' => $countries['countries_name'],
                'iso_code_2' => $countries['countries_iso_code_2'],
                'iso_code_3' => $countries['countries_iso_code_3'],
                'countries_name' => $countries['countries_name'],
            );

obviously it would be far better if ZC maintained consistency in their vars on the storefront as well as in the admin.

i currently started a PR to refactor the order class. it needs a lot of work. too much legacy stuff in there IMO. whether any of my work makes on the class makes it into the repo is another story.

best.

[lat9]

Separate issue (see above) created to track the change associated with the previous posting.