Error on checkout due to incorrect database data

[vkalchenko]

Hi, we had an issue during checkout due to inconsistent database data. We ran database cleanup before changing the website to production mode and forgot to clean up table avatax_queue. Due to this, we caught an exception in vendor/magento/module-sales/Model/Service/OrderService.php (191) during order placement. This was a data integrity issue SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry \'6-76\' for key \'AVATAX_QUEUE_ENTITY_TYPE_ID_ENTITY_ID\', query was: INSERT INTO avatax_queue (created_at, store_id, entity_type_id, entity_type_code, entity_id, increment_id, queue_status, attempts) VALUES (\'2017-01-18 16:23:23\', ?, ?, ?, ?, ?, ?, ?) and this is a result of queue model creation in ClassyLlama\AvaTax\Plugin\Sales\Model\Spi\InvoiceResource::aroundSave()

I want to say, even if the issue is not an extension bug (problem was related to data, not code) it would be great to simply add a try...catch block into aroundSave() method, just after $resultEntity = $proceed($entity);

This will allow customers to continue checkout even if AvaTax integration is not working properly. In our case, orders were not created, while customer money were captured and transactions are not rolled back automatically.

[erikhansen]

@vkalchenko Thanks for reporting this issue. I can definitely understand how you ran into this issue. I've also worked with merchants who want to delete test orders and reset their order/invoice/credit memo increment IDs before launching their site.

I do have a concern about simply wrapping the ClassyLlama\AvaTax\Plugin\Sales\Model\Spi\InvoiceResource::aroundSave() method in a try/catch block, as the inserting of the avatax_queue record is essential in order for this AvaTax extension to properly report taxes to the AvaTax API.

As opposed to changing the code, what do you think about adding a section to the www.classyllama.com/documentation/extensions/avatax-magento-2-module page, explaining that the avatax_queue table should be truncated if the merchant is wanting to reset order/invoice/credit memo increment IDs?

Even if a merchant doesn't read that documentation and creates a scenario where this happens, I think it's a reasonable to assume that a merchant should place a test transaction after making such a significant change like this.

[erikhansen]

Another option would be to wrap a try/catch block around that code and then if an error occurs, log the error to the exception log and then add an error to the message manager so that on the checkout confirmation page, the user will see an error message like this:

An error has occurred during the order placement. Please contact us with your order number so we can ensure your order was successfully placed.

[mttjohnson]

While I understand the interest in cleaning up the database when launching a site, most implementations I've seen regarding clearing out historical orders involve direct database queries, which have a lot of risks, this being one of those risks. Under any normal operation in Magento these errors would not be reproduced, and only happened because of the data integrity issues that were caused during the cleanup.

I don't think it would be wise to lightly ignore the error here in a suggested try/catch fashion as it points to the fact that the software was attempting to perform an operation that it should not have, and allowing it to continue without writing the data would cause further data integrity issues since it would not be able to report that order to AvaTax, and there would not be any obvious clue for the cause of the problem, or a way to recover from it. The system is dependent on the queue records in the same fashion that the order data is dependent upon recording all items and payment details on an order. The data getting recorded correctly is critical to the integrity of the order and the system as a whole. Businesses use and rely on AvaTax as a means of reporting their tax liability to governments, and any gaps in data that gets to AvaTax is as serious an issue as customer payments getting captured without an order getting written to the Magento database.

I looked into providing cascading deletes on a constraint and we can't implement a constraint in that way since we store both invoices and credit memo relations in the same table, and the architecture of the queue table would need to be reconstructed to in order to provide cascading deletes on the constraint. That would be my preferred way of dealing with your specific issue here as it would automatically remove the records in the queue table if the related records were deleted elsewhere in the system and would avoid any kind of data integrity issue all together. I haven't looked into what the effort would involve in re-architecting the table structure of the queue data to allow for cascading deletes, but that is my first idea for a better way to both provide the functionality you'd prefer existed of not causing any errors during checkout, but also prevent any data integrity issues.

The other thing that comes to mind is if there was a way to try and throw an error earlier in the checkout process to prevent the checkout process from getting to the payment capture stage if it would not be able to create a queue record. This would probably be easier to implement, and would prevent the issue you were seeing where payments were captured, but orders were not saved when it was too late into the checkout process. We could add a simple check to see if a queue record already exists for the entity_type_id and entity_id and provide a more informative error message preventing checkout, knowing that it would not be able to complete it correctly. While this isn't as complete of a solution as cascading deletes it would be easier to recover from than the current issue that comes up in the event of a data integrity violation. Also by providing an error message we could make it more clear what the problem is when you test your cleanup steps in a test environment before implementing it all in production where live customers would be affected.

[vkalchenko]

@mttjohnson Thanks for the feedback, I understand your point about direct database queries and rather agree with you. But in reality, as a Magento solution provider, our company is dealing with lots of new projects, and the database cleanup is not so rare operation, unfortunately. I think adding a simple check on earlier step should work and could be easier to implement.

[erikhansen]

@mttjohnson and @vkalchenko Thanks for your feedback.

@vkalchenko While this is not a high priority issue and I have no deadline for resolution, it is something we would like to address once we address some of the higher priority issues in our backlog, so I'm leaving this issue open.

[erikhansen]

@vkalchenko Rather than addressing this issue at the code level, I've decided that for now we're just going to address this via documentation. I added a section to our documentation to cover this: https://www.classyllama.com/documentation/extensions/avatax-magento-2-module#database-cleanup