cubecart / v6

CubeCart Version 6
https://cubecart.com
72 stars 59 forks source link

Allow order numbers to be incremental with prefix and suffix #636

Closed havenswift-hosting closed 6 years ago

havenswift-hosting commented 9 years ago

See feature request: https://features.cubecart.com/topic/incremental-order-numbers

I know that this says it is experimental but I believe it should be taken out until completed as it doesnt work correctly.

It is possible to place an order and the order numbering works OK, the order gets sent through to whichever gateway you want but returns with the old style order numbers and so order isnt updated from Pending to processing ; there is a mix of what order numbers have been used between the orders listing (incremental) and the transactions (old) and worse still the product code details seem to be lost from the order after payment - you can see an order with detail lines and prices but no product details

abrookbanks commented 9 years ago

Thanks.

abrookbanks commented 9 years ago

See commit b4e9ae49312ebb5dcd1d3a423649c862ffd31a4f

Dirty-Butter commented 9 years ago

FYI - I have been using Chuggyskin's Incremental Order Numbers mod for years very successfully. Perhaps you can work something out with him to make it stock??????

Rob-Tea commented 8 years ago

Has this now been abandoned?. Its a shame as in the UK you have to invoice with a sequential number (to my understanding, see HMRC guidelines [http://www.hmrc.gov.uk/manuals/vatrecmanual/vatrec5020.htm] ). + sequential seems to be the logical approach for a clean store install.

Dirty-Butter commented 8 years ago

I still use the v5 code from Chuggyskins (at some point modified by Bsmither). I cannot, however, merge the mod code with CC code anymore - I have to comment out the newer order number code sections in 6.0.10 to use the mod.

Dirty-Butter commented 6 years ago

Referencing your last comment with images on #1867. I like the idea of keeping the randomized REFERENCE to the order, while the human (admin and customer) sees an incremental order number. I have a very low volume store, so my incremental modification based on ChuggySkins works well for me. I suspect a large volume store might run into problems with a true incremental order number system that is easily human read. Good luck with this!!!

abrookbanks commented 6 years ago

Thanks. I think my new approach will work. I'm only making changes to the views and not any mechanics.

Dirty-Butter commented 6 years ago

Sounds good! I would suggest that the admin be able to set the initial number of the sequence. For instance, my last order number was 2018-PC-4019. If I changed to your system right now, I would want the next order to be 4020. (the year turns over automatically with Chuggyskins, and I set the PC for plushcatalog - I also have the choice of starting numbering over for the new year or continuing with the sequence. I chose continuing.

abrookbanks commented 6 years ago

Duplicate of #193

PloughGuy commented 6 years ago

I have been working on a version of this recently. (I will now stop). For maximum flexibility, and to knock over other development requests in the queue, I was planning to allow the user to specify some decorations:

Benefits of prefix and suffix: Useful for people with multiple stores that want to transfer transactions to a central accounting system. Benefit of fixed length numbers - predictable formatting. Benefit of starting with any number - new number range for each year, (e.g. 1000000 this year, 2000000 next year.) - seeding 000132000 disguises that your store opened yesterday - makes it easy to move into CC from some other store with order number continuity.

If this was implemented using hooks, alternate order numbering schemes can be implemented as required. The Elboonian digits-in-reverse-order requirement, for example, can be met with relataive ease.

An Implementation Test Finally, I will share some SQL. This is the core of my solution. You are all smart folk, and I am sure you have already thought of this. However, I have heard rumblings in the ethers that a database-resident solution will impose performance problems. I have tested this, and it works a treat, is very fast and fully atomic. There are performance numbers below, and scripts to help you confirm it.

// Read the current value to an SQL variable, and add 1 to it for next time. This operation is atomic. // The table always contains the next order number. $mysqli->query("UPDATE next_order_number SET next_order_number = (@order_number := next_order_number) + 1 WHERE number_name = 'next_order_number'");

// Extract the SQL variable, which is local to this session.
$result = $mysqli->query("SELECT @order_number;"); $row = $result->fetch_assoc(); $order_number = $row['@order_number'];

// Use it. printf("
next order number is %d",$order_number)

Performance Tests Running it at the command line level from a shell script, I get 10,000 numbers in just over three seconds, for better than 3000 order numbers per second, atomically sourced from an innodb table. -------------------------------8<------------------------------- RussMBP17:test_sequential_orders russ$ time ./runem.sh 1 threads is 1 Localhost via UNIX socket Running 10000 iterations next order number is 119142

real 0m3.076s user 0m0.300s sys 0m0.359s RussMBP17:test_sequential_orders russ$ -------------------------------8<------------------------------- Note the hostname - this is running under OS X on a 2012 MacBook Pro 17", which I know you all covet.

Lets scale it up: Eight parallel threads. -------------------------------8<------------------------------- RussMBP17:test_sequential_orders russ$ time ./runem.sh 8 threads is 8 threads is 7 threads is 6 threads is 5 threads is 4 threads is 3 threads is 2 threads is 1 Localhost via UNIX socket Running 10000 iterations Localhost via UNIX socket Running 10000 iterations Localhost via UNIX socket Running 10000 iterations Localhost via UNIX socket Running 10000 iterations Localhost via UNIX socket Running 10000 iterations Localhost via UNIX socket Running 10000 iterations Localhost via UNIX socket Running 10000 iterations Localhost via UNIX socket Running 10000 iterations next order number is 109021 next order number is 109056 next order number is 109089 next order number is 109102 next order number is 109119 next order number is 109135 next order number is 109140 next order number is 109142

real 0m12.716s user 0m3.206s sys 0m3.447s -------------------------------8<------------------------------- We get 80,000 sequential order numbers from 8 threads in under 13 seconds or 6,153 per second. CPU utilization is about 40%,.

Ramping it up to 32 concurrent threads for 320,000 order numbers, I get: -------------------------------8<------------------------------- real 0m12.416s user 0m3.156s sys 0m3.512s -------------------------------8<------------------------------- This gives 26,000 order numbers per second. The time improves, presumably, because the PHP and MySQL initializations are amortised over a much larger sample. Once again, CPU load peaked at 40%, presumably limited by the speed of the SSD.

I expect the load to connect to the database, save the order_summary and order_inventory rows, and update stock levels for the order would well eclipse this, so the cost of managing sequential order numbers in a database table will not impose an onerous load, even for very-high-throughput stores.

Conclusion: We have nothing to fear from storing the next available number in a database table like grown-ups should, and using appropriate SQL to retrieve it atomically.

If someone has a more modern bit of hardware to run it on (2015, perhaps?) then the numbers would be interesting. Still not buying a new Mac until this one dies, my spare dies, and no more 17" MBPs are available on eBay.

The scripts are uploaded so you can play with it yourself. Remove the .txt from the end. runem,sh takes a single parameter which is the number of threads. The php program contains a constant which sets the number of iterations.

A rudimentary check for duplicates is to confirm that the largest reported number of a run is appropriately larger than the result at the start.

[runem.sh.txt] (https://github.com/cubecart/v6/files/1718525/runem.sh.txt) test_sequential_orders.php.txt

The table was created as follows: -------------------------------8<------------------------------- CREATE TABLE next_order_number ( number_id int(10) NOT NULL AUTO_INCREMENT COMMENT 'Primary Key - in case we need more numbers later', number_name text COLLATE utf8_unicode_ci NOT NULL COMMENT 'Next order''s order number', next_order_number int(10) NOT NULL, PRIMARY KEY (number_id) ) ENGINE=InnoDB AUTO_INCREMENT=2 DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci -------------------------------8<-------------------------------

PloughGuy commented 6 years ago

I believe Amazon uses those crazy order numbers because they have vast numbers of loosely coupled online stores for scalability and availability reasons. Each store generates its own distinct and scrambled order numbers to prevent duplication, which include redundancy coding and other security features.

The system CubeCart currently uses has a 1/10000 chance of generating a duplicate for two orders arriving in the same second. It is probably due to the relatively low traffic (two orders every hour would be good) that this has not caused problems already.

My preference would be to replace the existing order number solution completely with hooks and offer some choices. I bet the uptake on the long order numbers would be very low...

PloughGuy commented 6 years ago

Dirty-butter said

I suspect a large volume store might run into problems with a true incremental order number system that is easily human read

I work with SAP systems that can process thousands of orders per second. They use sequential order numbers (because its the law) and have no problems with it. Each of the application servers in a system grabs a couple of hundred numbers from the database in a chunk and issues them until depleted, whereupon it grabs a bunch more. I expect that unless Amazon itself wants to switch to CC, we will have no problems.

There is no reason to not go sequential except (in our case) effort. I have demonstrated the throughput above on an ancient laptop. Currently available XEON servers will do much better.

abrookbanks commented 6 years ago

Thanks for the very detailed response. I've started building this from the CubeCart_order_summary auto incremented id column. I'll code it so this column can be changed. The easiest approach would be to create a new column with stored procedure to format itbased on the auto incremented id column.

E.G. {prefix}{leading zeros}{auto increment id + starting integer}{postfix}.

PloughGuy commented 6 years ago

Al, you are a saint. Provided the Elbonians don't come to us wanting the least significant digits on the left, it sounds like a solution.

Let me know if you need any other bright ideas. Russ

PloughGuy commented 6 years ago

Let me also say that using the order_id as the counter is a stroke of genius. I almost can't think of any reason not to.

abrookbanks commented 6 years ago

Al, you are a saint.

I've not got it working fully yet. Hopefully I crack it this time. :)

Let me also say that using the order_id as the counter is a stroke of genius. I almost can't think of any reason not to.

Yes it makes sense. The challenge will be running the stored procedure for some merchants. It might be a bit manual but lets see... Also some MySQL users don't have permission to create stored procedures. That's tough luck though.

abrookbanks commented 6 years ago

I think we need to be using triggers and not stored procedures.

abrookbanks commented 6 years ago

This works a dream for an extra column called custom_id to prefix ABC with two leading zeros.

UPDATE `CubeCart_order_summary` SET `custom_id` = CONCAT('ABC', LPAD(`id`, 3, '0'));
DROP TRIGGER IF EXISTS `custom_oid`;
CREATE TRIGGER `custom_oid` BEFORE INSERT ON `CubeCart_order_summary` FOR EACH ROW SET NEW.custom_id = CONCAT('ABC', LPAD(LAST_INSERT_ID(), 3, '0'));
havenswift-hosting commented 6 years ago

Al - great that you are making good headway with this. Can you ensure that you document the usage of a trigger like this for users that dont have good MySQL usage.

As you said, some users may not have MySQL permissions to create triggers - they simply need to move to a good hosting company lol

abrookbanks commented 6 years ago

Yes definitely. I'll write a support article. If I can manage it dynamically I'll write the code to do it automatically. Seems to be just a combination of four options. Prefix, postfix, leading zeros and starting point.

havenswift-hosting commented 6 years ago

Even better - fields to enter as many of these as they want and you delete any existing trigger and create the new trigger on save !

abrookbanks commented 6 years ago

Yes I can automate it all. Nice!

PloughGuy commented 6 years ago

I need to clarify: You are creating a new column to hold the new order ID field which will be statically and permanently assigned, not derived at read-time. When a new row is inserted, the trigger will call a script to populate the field so no additional coding is required on insert. Which raises a potential ongoing maintenance issue - the trigger is invisible and unexpected by most users Unless it is formally documented in the calling code. Something should be provided to make it visible to the administrator for troubleshooting When the administrator changes a setting (such as the next order number) you regenerate the trigger. Is that the gist?

Presumably existing orders will retain their long order numbers. Retroactive numbering is bound to upset the G-men. So these will have to be populated into the new field for existing orders.

Ultimately, it should be possible for the existing long order numbers to become invisible if desired. Russ

On 13 Feb 2018, at 21:10, Al Brookbanks notifications@github.com wrote:

Yes I can automate it all. Nice!

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/cubecart/v6/issues/636#issuecomment-365216051, or mute the thread https://github.com/notifications/unsubscribe-auth/AeEGPRv03ktJt3rt60dUw2t9QChPNNzzks5tUV-dgaJpZM4FKC_A.

abrookbanks commented 6 years ago

Yes I've considered all that. It will be automated with no need for maintenance.

abrookbanks commented 6 years ago

Testing to be done but looking good so far. Movie here: http://static.cubecart.com/Flexible_Order_Numbers.mp4

Dirty-Butter commented 6 years ago

Regarding your warning about an order number becoming truncated to 99. Would a seller be able to easily change to more leading zeroes some time in the future as the number of orders approaches the existing number of digit limits? Would that mess up past order numbers? Would admins need a warning that they are approaching "running out of human readable numbers"? Sorry for newbie questions, but a newbie thinks differently than someone who knows what they're doing LOL.

abrookbanks commented 6 years ago

It's a good question. Do you think it should have a min value of say 5 or off?

It will also regenerate every order if it changes. Maybe it shouldn't.

PloughGuy commented 6 years ago

Almost there...

From my personal point of view, this is already acceptable.

But... Issue 1 - Persistence From the video, it appears that the order numbers are derived at display time. You change a setting and all existing numbers are changed. From an audit (and tax compliance) perspective, this is not correct.

I will have issued a tax-relevant invoice to a customer with a number on it that no longer matches my records. The entire order number, with prefix, suffix, digits, and offset must be indelibly recorded by us in the form that was presented to the customer. This will be a requirement for any VAT tax juristriction since this is the primary means of policing tax fraud. And we don’t want to annoy a tax fraud detector. Remember what happened to the other Al. Mr Capone was eventually sent up the river for tax fraud. Wouldn’t want to produce a product that aids and abets...

Issue 2 - Captions in Settings From the Dept of Nitpicking Details, the bit at the end is a “suffix”. “Postfix” is a mathematical notation.

The length caption should be “Incremental order number length with leading zeros”. As written it says we will put 6 zeros in front of the number. The note below might be clearer as ‘For order 1 a value of six will generate “000001”. Set to 0 to disable leading zeros.’ And put a “:” after “Caution:”

Other than that, it’s perfect.

Dirty-Butter commented 6 years ago

I can't give advice for what's best for others. I can tell you what I did and let you learn from my thinking process. When I started, I started with 1000, because I didn't want my store to look brand new. I used the PC for plushcatalog because I use ES for the same setup on our estates store. Our plushcatalog store is now in the 4000's, so it would be quite a while before I run out. I would not want old order numbers to be changed if I ever had to add more digits - way too confusing for me I think. And I DO like having the year of the order in the ID, but only if it changes automatically to the next year. sequentialsettings

PloughGuy commented 6 years ago

Butter’s remark regarding running out of displayable numbers is a good one. A few lines of code that issues a warning that we are within three months running out of numbers based on current order generation rate would be useful. I don’t think a minimum number is necessary but a warning if it set to 5 or less might be appropriate.

abrookbanks commented 6 years ago

Thanks both. I'll review these points and make some changes.

PloughGuy commented 6 years ago

Butter, Automating the year Is an interesting idea but we would need to put in a pattern rather than a number of digits for example a COBOL-style string like YYYY000000.

IMHO, That should be raised as a seperate request.

abrookbanks commented 6 years ago

screen shot 2018-02-14 at 16 31 18

screen shot 2018-02-14 at 16 31 11

Dirty-Butter commented 6 years ago

Looks great! Would it be possible to test out this code on 6.1.14?

abrookbanks commented 6 years ago

Only by using the 6.2.0 branch of code but it's no way ready for production yet.

Dirty-Butter commented 6 years ago

That's what I figured, but didn't hurt to ask. Very encouraged by your progress with this!

abrookbanks commented 6 years ago

Thanks. We are really do a new version with features.

PloughGuy commented 6 years ago

Looking fine, Al.

But of course...

Proposal 1 I think the "Starting Id" implies that this will be the first number. Asking for the number before the first number is exposing the internals and it is likely to cause a mental hiccup in the user. Asking for "First order number minus 1" makes it feel not quite finished.

It really should be "First order number". If the customer enters 1, then we record 0 as the starting id. If the user enters 1000, then the first order should be 1000, not 1001.

Proposal 2 The "Show me a sample" function is beautiful. Nicely done. Perhaps the function of the button should be "Save then Preview". Otherwise the ergonomic flow is odd as in enter settings, go to the bottom of the form to Save, then go up to Preview.

And a question Please confirm that we (as in "you") are now saving the fully formed order number in the order. The "Force format to all past orders" implies this, but the giant purple snorkelwhacker in my anxiety closet needs to know for sure.

Proposal 3 Perhaps "Apply" rather than "Force". Force sounds so brutal.

Deeply Wary Observation 1 The "Force/Apply format to past orders" function represents a risk from a Tax Audit point of view because it potentially breaks the connection between vendor records and customer records. Unless, of course, the original number is still available and visible. I feel deeply nervous about the compliance implications of providing this capability - breaking the relationship between customer and vendor documents in a VAT jurisdiction is generally verboten. And not all store-keepers will be aware of it.

A vendor that demonstrates a move from a non-compliant to a compliant stance is in a better position than one damages the audit trail...

PloughGuy commented 6 years ago

And another thing: If I have incremental order numbers in place, and I want to change the sequence for a new year, I should see the current next order number when I come into settings, so I have a better chance of not causing a regression.

Do we have a unique index on the incremental order number field to prevent duplicates? I presume a database error would be a better outcome than a big bunch of duplicated order numbers in the event of a cock-up.

abrookbanks commented 6 years ago

@PloughGuy Thanks there is some really constructive stuff here.

Response to Proposal 1 It won't be the first one as the previous orders won't be touched unless "Force format to all past orders" is checked. I'm struggling to come up with the best terminology for this.

Response to Proposal 2 The button doesn't save anything. It can't submit the page and load an ajaxy modal. For this reason preview after save seems to fit best albeit a bit clunky from a UX point of view. Actually it could but being asynchronous it may or may not work reliably.

Response to "And a question" Yes the format is saved. It's not generated in real time each time. If the format changes past orders will keep their old format. Say it starts on traditional then to incrememtal with no formatting and then to say having a prefix. he history will remain unchanged unless told to update. It's stores in a new column called custom_oid.

Response to Proposal 3 Agreed. I'll change to Apply which is a bit less fearsome.

Response to Deeply Wary Observation 1 I agree. I can see merchants asking for this feature regardless however so I think we need it. A merchant needs to be aware about the legalities of their operations. I can see a scenario where a new merchants gets three sales then find this feature. They might want to change them all and reissue invoices.

abrookbanks commented 6 years ago

@PloughGuy

Do we have a unique index on the incremental order number field to prevent duplicates?

Yes. It will always be unique as it is synced to the auto incremented id field.

abrookbanks commented 6 years ago

@PloughGuy

It really should be "First order number". If the customer enters 1, then we record 0 as the starting id. If the user enters 1000, then the first order should be 1000, not 1001.

There is confusion here. If we already have 500 orders the system and the first order number is set to 1000. The next order will be 1501. Maybe this needs to be dynamic somehow and/or terminologically different.

PloughGuy commented 6 years ago

As the people who increase the fibre content of their canned soups by adding asbestos can tell you, Truthful Labeling is not always the answer.

We really need this to be next order number. I see the challenge. The shop is humming along accepting orders, and the difference is potentially a moving target. Plus we want to prevent duplicates.

To guide the user, we can report the most recent order number in the text if ION is already enabled.

To determine the correct difference, we can lock the order-summary table for a few microseconds to freeze the order-ids, calculate the increment, save it away, then unlock. We are talking about a 1ms disruption.

We can even insist that the administrator closes the shop to do this. It is an uncommon event, after all.

Or are you wrestling with a different demon?

abrookbanks commented 6 years ago

I don't we can have a starting order number less than the current maximum.

PloughGuy commented 6 years ago

If I start with an offset of 1000 and I accept 1000 orders, then I try to set the offset to 1, what happens?

abrookbanks commented 6 years ago

It's a good question and something I was thinking about too funnily enough. The next value will have to be the same or higher.

PloughGuy commented 6 years ago

I really, really, really, really, really, really think the user should be dealing in absolute next_order_number numbers, because if we can't work it out, the user has no chance.

Really, really, really, really, really, really...

I desperately want this to be done right so it stays done and we never, ever have to come back to it again.

abrookbanks commented 6 years ago

I would say that once the offset is made and orders come in it can't be changed. I can't see a way to make this absolute without taking logic away from SQL to PHP which in turn means a total rewrite now.

PloughGuy commented 6 years ago

Fair enough. The user can change the prefix, the suffix, the number of digits. Should be enough.

From the initialisation POV, then, if the table has 10,000 orders in it and the offset is set to 0, the next order will be 10,001. Is that correct?

Dirty-Butter commented 6 years ago

The only reason I can see to make a future change after initially moving to sequential system would be if you realize you need more digits. It wouldn't make sense to me to try to change the starting number AGAIN - is that the issue you are trying to resolve?