Issue with Transactional Rollbacks during Test Environments with Spree Log Entries

[reidcooper]

I am proposing this fix to the Spree::LogEntry model.

When running my tests with DatabaseCleaner (using :transaction), after it asserts my first example using rspec, DatabaseCleaner kicks in to transactionally rollback all my test data. Unfortunately, due to this feature, I run into issues where the after_rollback is called on the Spree::LogEntry model which in turn tries to append some information that does not exist anymore since DatabaseCleaner had its way and cleared out my test data. This causes an issue where the record of a variant is not found anymore.

For those who use DatabaseCleaner, to ignore trying to create new Spree::LogEntries during test environments. I have attached a gist of a log with a backtrace of my byebug statement. Notice at the top of the gist, BEFORE @lpes_calculator; AFTER @lpes_calculator is wrapped around my expect statement.

I am open to potentially better solutions.

https://gist.github.com/reidcooper/2e09d5dc21f5783b26d0ae7a633a22f5

[houndci-bot]

Use proc instead of Proc.new.

[mamhoff]

There used to be #564, which is a more comprehensive solution to this thing. @jhawthorn What do you think, should we just revive that?

[cbrunsdon]

Have we ever talked about having a second storage mechanism for stuff like this, separate from our AR?

We're fighting against our technology with things like the log_entries, and it seems to me it would make more sense to shove it in some kind of document or object storage, its not like we ever actually do anything with it in terms of querying.

[jasonfb]

(discussed with Reid)

An abstracted storage mechanism — in which stores (no pun intended) could choose where to put their spree_log_entries — makes total sense to us. But it seems to me that for core code to dictate a document or object storage, that would expand the (already tight & small) stack requirements for setting up a new Solidus installation.

We recognize that this is a small piece of paper mache that covers up a bandage that itself was originally covering up what is (arguably) a code smell. It seems to us the root of the problem is that exceptions (in this case, the rollback) are relied on for too much flow control, which is essentially the root of the code smell. We fully understand this is not a fix to the root of the problem.

Also note it can be decorated as the decorated after_rollback seems to over-ride the core code definition. (this worked fine in our app)

So do with that information what you will.

[gmacdougall]

👍

We talked a little bit about removing this callback or removing the log entries altogether, but for now this solves a leaky database and I'm always in favour of that.