Orders class

[piernas]

Are there any plans to change the orders class in the near future?

I was updating an add-on I made some time ago that needs to check order status. I found it does not contains $order->info['orders_status'] but instead there is $order->info['orders_status_name'], and that property is language dependant. To properly check the current order status we should need another field.

 

Where can I suggest this addition? Is this the correct place?

[tgely]

Hi Juanma,

 

pointed places are not critical but confused a bit. Its a terminal logic obstruction.

 

catalog\account_history_info.php

      <div class="panel-heading"><strong><?php echo sprintf(HEADING_ORDER_NUMBER, $_GET['order_id']) . ' <span class="badge pull-right">' . $order->info['orders_status'] . '</span>'; ?></strong></div>

catalog\admin\orders.php

                <td class="main"><strong><?php echo ENTRY_STATUS; ?></strong> <?php echo tep_draw_pull_down_menu('status', $orders_statuses, $order->info['orders_status']); ?></td>

All these $order->info['orders_status'] cases can handle with simple alternative way. I dont see any barrier to change it, but I have no imagination when it will be corrected.

In orders would be more variable to handle some other conflict but now there is no milestone yet.

[piernas]

Thank you @@Gergely for your answer.

[piernas]

Something similar happens with shipping_metod (the class retrieves method name in shop side and doesn't even exist in admin side). And also, and even more grave, payment_method is stored directly in the database as a localized name.

 

I want to help with these, but I'm not sure on how to. There are several approaches to fix the problem. Any tip on how to help with this? Should I do the modifications and propose them here?

[piernas]

Sorry I don't understand what do you mean?

[piernas]

This is the current code from orders class in the shop side about order status:

      $order_status_query = tep_db_query("select orders_status_name from " . TABLE_ORDERS_STATUS . " where orders_status_id = '" . $order['orders_status'] . "' and language_id = '" . (int)$languages_id . "'");
      $order_status = tep_db_fetch_array($order_status_query);

      $this->info = array('currency' => $order['currency'],
                          'currency_value' => $order['currency_value'],
                          'payment_method' => $order['payment_method'],
                          'date_purchased' => $order['date_purchased'],
                          'orders_status' => $order_status['orders_status_name'],
                          'last_modified' => $order['last_modified'],
                          'total' => strip_tags($order_total['text']),
                          'shipping_method' => ((substr($shipping_method['title'], -1) == ':') ? substr(strip_tags($shipping_method['title']), 0, -1) : strip_tags($shipping_method['title'])));

I think the better way to do this would be this one:

      $order_status_query = tep_db_query("select orders_status_name from " . TABLE_ORDERS_STATUS . " where orders_status_id = '" . $order['orders_status'] . "' and language_id = '" . (int)$languages_id . "'");
      $order_status = tep_db_fetch_array($order_status_query);

      $this->info = array('currency' => $order['currency'],
                          'currency_value' => $order['currency_value'],
                          'payment_method' => $order['payment_method'],
                          'date_purchased' => $order['date_purchased'],
                          'orders_status_id' => $order['orders_status'],
                          'orders_status_name' => $order_status['orders_status_name'],
                          'last_modified' => $order['last_modified'],
                          'total' => strip_tags($order_total['text']),
                          'shipping_method' => ((substr($shipping_method['title'], -1) == ':') ? substr(strip_tags($shipping_method['title']), 0, -1) : strip_tags($shipping_method['title'])));

Another solution would be leaving 'orders_status' as is and just adding 'orders_status_id'. It doesn't break any code but it's unconsistent with andmin class:

      $order_status_query = tep_db_query("select orders_status_name from " . TABLE_ORDERS_STATUS . " where orders_status_id = '" . $order['orders_status'] . "' and language_id = '" . (int)$languages_id . "'");
      $order_status = tep_db_fetch_array($order_status_query);

      $this->info = array('currency' => $order['currency'],
                          'currency_value' => $order['currency_value'],
                          'payment_method' => $order['payment_method'],
                          'date_purchased' => $order['date_purchased'],
                          'orders_status_id' => $order['orders_status'],
                          'orders_status' => $order_status['orders_status_name'],
                          'last_modified' => $order['last_modified'],
                          'total' => strip_tags($order_total['text']),
                          'shipping_method' => ((substr($shipping_method['title'], -1) == ':') ? substr(strip_tags($shipping_method['title']), 0, -1) : strip_tags($shipping_method['title'])));

Or even removing $order_status_query and maknig a function that returns the order status name

      $this->info = array('currency' => $order['currency'],
                          'currency_value' => $order['currency_value'],
                          'payment_method' => $order['payment_method'],
                          'date_purchased' => $order['date_purchased'],
                          'orders_status' => $order['orders_status'],
                          'last_modified' => $order['last_modified'],
                          'total' => strip_tags($order_total['text']),
                          'shipping_method' => ((substr($shipping_method['title'], -1) == ':') ? substr(strip_tags($shipping_method['title']), 0, -1) : strip_tags($shipping_method['title'])));

Any thougts?

[piernas]

I've been digging on the new order class included with paypal app. Seems like Harald Ponce already fixed thison admin side by adding a new status property that contains order status name:

                          'status' => $order['orders_status_name'],
                          'orders_status' => $order['orders_status'],

It will contan order status name in the admin language. I don't know where it's used but seems that the paypal app must need this.

 

But now there's stil an inconsistence with the current shop side where order_status will return name instead of id:

'orders_status' => $order_status['orders_status_name'],

The property is AFAIK used just on a couple of files on the shop side and in one on the admin side:

account_history_info.php

checkout_process.php

administrador\orders.php

 

Also in several payment modules included in the distro, but in this case I don't know if the modules use the store side class or the admin one.

 

Does somebody know what class is used in this case?

 

[piernas]

Yes of course it can be done. In fact that's what I've used before, but I think it is incongruent having the same class in both sides of the shop that uses the same property and returns different results. It would save code and make it conscious. And also that's the reason of having a class, to get all useful information about orders without using external functions no?

 

I just posted a question before reading your answer asking why should be two versions of most classes. In my point of view it would save code too to use the same clas in all the shop and would make it much easier to mantain. I think this matches the purpose of tighten the codebase and cleanup code Harald has started. But I don't know if there's a reason I don't see for mixing all the classes in one directory.

 

And apart of this, also shipping_method and payment_method has similar problems. In the case of payment_method the problem is bigger as if you request the information you get the payment method name IN THE LANGUAGE THE ORDER WAS CREATED, and you don't have any easy way to get that language id after the order has been placed; so you have to figure out this by checking all names in all languages of the payment method. This one must be fixed in any way because it's a pain to get the payment method.

[piernas]

@@wHiTeHaT I mean storing payment_method as a string that is language dependant makes almost impossible to check the payment method used on an order. I've suffered this.

 

Imagine a spanish customer places an order. He wishes to pay by bank transfer, so in the database it will be stored "transferencia bancaria". Another customer whose language is english places another order. In this case the database will hold "Wire bank transfer". A german customer will prefer his language and put "banküberweisung". And so on.

 

Then you want ie. to program an addon that shows a message on the order history that tells the customer something if the payment method is "Wire bank transfer". There's not a reliable, easy way I've found to check what module is this one. If you store the module class name i'ts relatively easy, but if you get the localized name... Well the function to get the class you must use will be a pain. One way I imagine is getting all the installed payment modules and iterate each language file searching for his title... Because orders table does not even contain a reference of the language used by the customer.

 

So, I'm pretty sure "This must be fixed" Because it shows the wrong data. Don't you think so?

 

The other things (orders_status and shipping_method) are in fact improvements. Really easy to do and I think logical improvements in fact, because the sql query in the class already retrieves the order status id. Then it converts the value to a string giving his name. So it's a duplicate task to make another call to a function that reverts back this and gets again the order status id from the order status name.

 

Maybe I'm a bit stubborn with making codebase easy and useful. I see classes like some sort of an api, and I see much more clear and logical to work with ids rather than with localized strings. Of course it can be done but makes things clearer, easier. I love easy code as you may see :)

[tgely]

If we would have only one cool order class than we could use cool ajax order editor easy on admin. This is the goal of us but there are some incosistent different class data processors now.

[piernas]

Now let's go on to "your idea" .................and we go use an ID for the payment_methode.....

Now imagine had 100 clients using payment_methode_id 3 (refers to let's say paypal)............

Now.......... you got sick or tired of paypal..... or it stopped to exist......... or you removed it by accident.

 

@@wHiTeHaT Yes there could be some cases where it could be inconsistences. order_status_id can change (in fact new paypal app uses some id's that I curently use for other purposes. But order_status_name can be changed, too. But it's easy to fix with autonumeric fields in the database as.

 

And seems that the current direction is very close to what I've purposed, as in the current paypal app the orders class has been updated with a new property instead of adding a custom function as you suggest.

 

In case of payment_method you won't store the id on the database but the class 'code' (moneyorder, flat, paypal_express...) so the only way to get the problem you mention is that someone create a payment method that uses a 'code' that's currently in use by another. And same applies for shipping.

 

I have at least three bits of code I can't share because all of them refer to the payment_methot. For example, something as simple as a report that gives you how many customers have chosen each payment method this month... can't be done without hardcoding the methods descriptions in each language the shop has. If you have three payment methods and say four languages you end up with 12 types of payments instead of just three. And if you later install a new language you have to manually add it to the addon. Not a good practice for me. It can't be installed seamlessly at all even with the best hook system.

 

But if you use the payment code (as does shopping cart) you will have just three as it should be.

 

For all shops who sells locally that's not a problem but if you sell internationally (I do and the vast majority nowadays) it is a problem.

 

And I'm sure you won't store 'red' or 'blue' in your current work with product options, right?

 

 

If we would have only one cool order class than we could use cool ajax order editor easy on admin. This is the goal of us but there are some incosistent different class data processors now.

 

I agree. IMHO inconsistences are no good at all in any software piece.

[♥bruyndoncx]

@@piernas

I feel your pain

 

@@wHiTeHaT

here is some code from my admin/invoice that we use for order picking

you can see how difficult it becomes to not now the language the order was placed in

 

		if ($order->info['payment_method'] == 'PayPal') {
		  echo '<b>BETAALD</b>';
		} else {
		  echo '<b>NIET BETAALD</b>';
		}
    $afhaling = '';
    for ($i = 0, $n = sizeof($order->totals); $i < $n; $i++) {
//		echo '--'.$order->totals[$i]['title'] .'++';
/*		  if  ( ($order->totals[$i]['title'] == ' (Kadoverpakking):') or
		        ($order->totals[$i]['title'] == ' (Gift Wrap):') ) {
*/
      if  (( (strpos(strtolower($order->totals[$i]['title']), 'kado') !== False ) and (strpos($order->totals[$i]['title'], 'Geen') == 0 )) or
		      ( strpos(strtolower($order->totals[$i]['title']),'gift') !== False) and ( strpos($order->totals[$i]['title'],'No Gift') == 0) ){
			echo '<br /><b>'. strtoupper( trim($order->totals[$i]['title'], ' ():') ).'</b>';
		  }
		  if ( (strpos($order->totals[$i]['title'], 'Ophaling in de winkel') !== False) or
		       (strpos($order->totals[$i]['title'], 'Store pickup') !== False) 
           ){
		    $afhaling = '<br /><b>AFHALING</b>';
		  }
		}
    //payment method indicates store pickup - shipment was free - thus the extra check
    if (!$afhaling and ( (strpos($order->info['payment_method'],'Betaling bij afhaling') !== False) 
        or (strpos($order->info['payment_method'],'Payment at pickup') !== False) 
        ) ) {
    		    $afhaling = '<br /><b>AFHALING</b>';    
    }
      echo $afhaling;

[piernas]

@@piernas

I feel your pain

 

 

At last! :)

And in your example you have only two languages. I can't think what will feel the mantainer of a four or five multilingual shop (w00t)

[piernas]

I've started an alternative orders class. I've found somehing I don't understand. In the shop side of the class the country is stored as an array with only one variable ('title').

 

This is used by checkout_process and by some payment modules. If I don't miss something this could perfectly be defined same as in admin area, as

'country' => $order['billing_country'] 

instead of

'country' => array('title' => $order['billing_country']),

Any reason for that?

[tgely]

You can find a lot of general function differencies and the reasons there are. The problem is that admin and shop site developed in separated way.

[piernas]

This difference is strange because it makes an array with country data but I couldn't find other variables for this array. Seems like it was planned to expand it and wasn't done? Maybe it was planned to add country ISO codes and such (something that would be useful in fact).

The more I inspect that class (and other pieces of code) the more I find unconnected bits of code.

[Bob Terveuren]

Hi

On the catalog side the $country array should hold  the title, two iso codes and the countries_id from the countries table in the database, depending on the store setup they may not all be used all of the time but they're in there 'just in case' they're needed by shipping & payment modules.

 

I'd guess that on the admin side the array was never constructed as the code there was only conceived to read the order data from the orders table rather than pull in all the data from address_book, table_zones etc. as that data was already pulled out during the checkout_process on the catalog side of things and save.

 

If you then add in something like order editor/create order to the admin you then  need to have the catalog version of the class so that you can get the iso data or in one order editor they have a couple of custom functions to get the country_id, zone_id

'country_id' => oe_get_country_id($order['billing_country']),
'zone_id' => oe_get_zone_id(oe_get_country_id($order['billing_country']), $order['billing_state']),

[piernas]

 

Hi

On the catalog side the $country array should hold  the title, two iso codes and the countries_id from the countries table in the database, depending on the store setup they may not all be used all of the time but they're in there 'just in case' they're needed by shipping & payment modules.

 

 

Yes I supposed the same but what was strange to me is that the class only manages the title and not the ISO codes and country id. Maybe it would be a good idea to complete that schema and por it to the admin class. It would help for editing orders as you say and also for making custom reports, for example.

 

Getting these into the class would improve it and make several functions useless and reduce code IMHO. Otherwise having a zero dimensional array in the clas has no sense.

[tgely]

Not only order class itself has problem but the orders table has a few saved fields than we should need.
Order language and payment class name are missed too. Payment name by language is a fake way to handle and without order language we can not handle status emails correctly by language.

[piernas]

@@Gergely 100% agree.

I've been looking deeper into the class and these are the idea about what orders table should contain. Please suggest if something is missing or if you think it should be done in other way.
 

LEGEND:

• Field currently exists
• Field is obsolete, redundant or contains localized data
• New purposed field
• Modified field

 

 

 

• orders_id
• language_id -> Really useful for communications with customers (email, personalized welcome or alert messages...). The field could be also stored on customer details table and change if the customer changes the shop language.
• customers_id
• customers_name
• customers_gender -> Really useful for communications with customers (email, personalized welcome or alert messages...). This field should be stored on customer details table and cloned here, or be retrieved by the class from customer details table.
• customers_company -> Currently Oscommerce does not have a way to differentiate if the customer is a phisical particular person or an enterprise. It's different for invoicing. I think this field needs some work. Maybe an individual/company field could do the work:
• customers_is_business (boolean) -> to be stored on the customers info table. Or it could be assumed that customers name = customers company if it exists and customers name= contact name. This should be also defined in invoice and packing slip too.
• customers_uid: Field to place VAT number (EU), national identification number or any other number associated with customer if needed.
• customers_street_address
• customers_suburb
• customers_city
• customers_postcode
• customers_state
• customers_address_format_id -> All of this information is redundant and should be removed. There's no use because it's already on billing address.
• customers_country
• customers_telephone NOTE It might be interesting to add a cell phone field (and also on customers table)
• customers_email_address
• delivery_name
• delivery_company
• delivery_street_address
• delivery_suburb
• delivery_city
• delivery_postcode
• delivery_state
• delivery_country -> This currently contains localized name. It should contain entry_country_id from countries table like in address_book.
• delivery_address_format_id -> I'm not sure about this one. Seems to be calculated at checkout. I'm not sure if it's needed to store in the database the obtained value or let the class calculate it.
• billing_name
• billing_company
• billing_street_address
• billing_suburb
• billing_city
• billing_postcode
• billing_state
• billing_country-> same as delivery_country
• billing_address_format_id -> same as said for delivery_address_format_id
• payment_method -> This one should be removed and replaced by payment_method_class or by some kind of unique id. Payment method handling is another complex class to examine in the future.
• cc_type
• cc_owner
• cc_number
• cc_expires -> These should be removed completely from the database https://www.pcisecuritystandards.org/index.php
• last_modified
• date_purchased
• orders_status
• orders_date_finished
• currency
• currency_value  Is it really useful?
• shipping_method: It's a weird question that there's no trace of shipping method used on an order. it could contain an method_id or shipping_module_class. Other properties may be useful to be added to this class.
   

About shipping methods there are other properties that could fit on the orders class. Shipping methods themselves may require a new, redefined class and a new way to store their data.

 

 

 

[tgely]

As I have experienced that developers doesnt like drastic changes. Keep in mind that many checkout (payments) use this class.

I dont know exactly how many checkout could be estimated.

The best thing would be if we add only the new green fields and be calm with red lines. This could be sure for bakwards compatibity. Later if the project will get more power than we could be able to destroy red lines.

 

 

[♥Gyakutsuki]

I am not agree with that 

• customers_street_address
• customers_suburb
• customers_city
• customers_postcode
• customers_state

because If we delete that, the address become dynamic or if a customer change this address, it change all the invoice if I understand your solution.

[piernas]

@@Gergely you're probably right, these can be drastic changes. I usually prefer drastic changes over surface fixes, but that's my personal taste. I was suggesting because if the next version will break addon compatibility it could be a good time to do it and when order list grows there's a significant amount of redundant data on the databases; but in the other hand as you say it won't hurt to leave them there.

 

Most of the green fields will just need a couple of simple functions and some changes on checkout process files, so I'll start workin on this as soon as I can have a free day.

 

But others (like shipping method or specially billing method) that could need their own classes and are widely used are more difficult to work with, so I'll just add the new fields and once the rest is okay we'll see what to do with them, don't you think so?

 

@@Gyakutsuki there are usually just one or maybe two addresses in one purchase. Shipping address (where the products must be sent, for individuals is usually their postal address and for companies is usually the address of their warehouse, factory or wherever they use the goods). And billing address (where the invoice must be sent, usually the tax domicile of a company and in most cases same as shipping for individuals). Both of them should be shown on the invoice.

 

But in oscommerce we have three addresses on each order: shipping, billing and customer address. Only shipping and billing are useful, and I can't imagine why the third one was added. By removing this third addres (not shown on invoice or packingslip) you don't loose any relevant information. Both needed addresses will still remain on the orders database table.

 

I'm currently not sure if that 'customers address' is taken from main address or from billing address, but in any case the info isn't needed.

 

 

It would be nice to hear what you think about some fields like the proposed customers_uid. Do you think it would be useful to have it into the orders table?  I already have a similar field for VAT numbers and even if validity checks are done outside the class I see useful to have a standard field where to store and search for that kind of information.

[♥Gyakutsuki]

Ok I understand better what do you mean.

[piernas]

i think one day, you go regret it if delete it.

 

Removing is easy!

 

Making new and better is more complicated.

Especially with the new EU tax rules it could become handy to have that customers address.

I also think your approach is not correct, sorry.

 

To run a 100% LEGAL ordering database it is very important you keep the OLD order data..... please keep that in mind.

It is also the last time i go mention this  (there where other people mentioning to make it able to edit the orders db).

 

Store edited orders as a "version", but keep the original order.

 

Original order id 1,  (parent_id 0)

Edited order order_id 22 (parent_id 1).

After you just switch the order_id's... and all is done and with much much lesser changes to the core.

For the language issues, i understand your points, but for that to solve i would just store the constant's like : define(MODULE_PAYPAL , 'Paypal') , then simply store MODULE_PAYPAL to db.

 

 

The orders db looks BIG for some people.... but it is nothing.

Trust me.... there are much more complex db's table's out there..... and they really scare you.

 

I'm not saying the database is complex; it's easy as using a nipple. I'm saying it is storing completely uselless duplicate data, just that.

 

You have billing address (the legal address for invoicing and taxing) and shipping address. What does represent customer address? I don't see handy to store useless data.

 

When the customer places an order he is only asked for shipping and billing address. Where does the payment process asks him to specify that "customer address"?

 

BTW I've done a quick test and an SQL dump for a 4,000 orders table is 22% heavier than one that does not includes that duplicated information.

 

This topic is not about order editing but about class optimization so the rest of your comments should be covered on another thread.

 

 

and why:

 

• cc_expires -> These should be removed completely from the database https://www.pcisecur...s.org/index.php

it is nice you put a reference to a site, but.... where on that reference i can read why cc_expires should be deleted, can you be more direct?

 

So.... if i not behind my machine... you place an order, and i not see your card is expired (but somehow the order went  true), i can take caution before send you the product....no?

 

 

I think you should read the link I posted above. Oscommerce does not comply PA-DSS so if you use the credit card feature in Oscommerce your business is not PCI DSS compliant. That single reason should be enough to remove those fields because it can only lead to legal troubles if you use them 'as is'.