Insert then an undo then a redo does not correctly invoke tx.checkPermission

[rjsmith]

The following sequence of tx transactions does not work, if you use a custom tx.checkPermission that expects a non-null doc argument for an insert action:

1. Transaction that inserts a doc into a collection
2. tx.undo()
3. tx.redo()

The tx.redo() fails because the tx.checkPermission call fails.

I am about to upload a github repo that reproduces the problem

[rjsmith]

Here is the repo, please read the README for full details of my investigation and proposed fix:

https://github.com/rjsmith/meteor-transactions-bug-repo

[JackAdams]

Richard, thank you so much for the time you've put into chasing down this bug and finding a fix. I really appreciate it. You're quite right about the cause of the bug.

I'll certainly look into removing the Meteor.isClient from the onTransactionExpired call. You make a good point about what happens when the tx.undo and tx.redo methods are called from the server.

I'm putting this on high priority and hope to get a new release out within 24 hours.

[rjsmith]

No problem - thanks for creating this package.

re: undo & redo, maybe allow a standard - pattern function(err, res) callback to be passed as a 2nd parameter?

[JackAdams]

Yes, I'll see what I can do. I really should have had this pattern in place from the beginning.

[JackAdams]

@rjsmith I can't thank you enough for your attention to detail in tracking this error down and suggesting a fix.

Moreover, the repo with the jasmine test was just the gentle prod I needed to get my house in order (i.e. start writing a proper test suite). A package like this should really not exist without decent test coverage, but I've been putting it off due to my inexperience with testing. Now that you've shown me a blueprint of how it can be done, I am without excuse. :-)

It's contributions like this that motivate me to implement things like this sooner rather than later.

If you're ever in Beijing, dinner's on me!

[JackAdams]

Forgot to mention: in 0.6.16, function (err, res) { ... } can be passed as the second (or first) parameter of the tx.undo() and tx.redo() functions. The default behaviour of firing tx.onTransactionExpired (on the client only) is unchanged for backwards compatibility, but can be switched off with tx.onTransactionExpired = null;. If the undo or redo was successful, res will be true, and false if the transaction has expired.

[rjsmith]

Cool, thanks.

re: testing .. agreed, especially for a package like this where it really is important it does the right thing, or at least refuses to do the right thing if anything might go wrong!

You might want to consider creating a separate test app repo, which adds meteor-transaction package, so that you can use the full set of velocity - enabled test frameworks, especially sanjo:jasmine, and perhaps xolvio:cucumber for GUi testing of the undo / redo widgets. You could then write any mix of client/server unit/integration tests you think is useful. You could use my bug repo as a starting point if that helped.

I have recently been involved with the Velocity core team, as I contributed a Velocity wrapper for the python - based robotframework end-to-end framework. They are a very helpful group, and very willing to help out. So, good luck !

[JackAdams]

Thanks again, Richard. I'll definitely be using your bug repo as a starting point for moving forward with testing.

You're absolutely right that the package should refuse to do anything if there's a potential problem. If I get that mongo 2-phase commit going properly, it will be a big step in the right direction, but having a decent test suite is non-negotiable if this package is going to end up in production apps.