Records need lock protection

[jordanbyron]

Currently it's very easy for two users to overwrite each others changes without knowing in. We need some type of locking mechanism to prevent this from happening.

[ptn]

This sounds interesting. Does this refer to pages or articles or both?

How about this:

1. Add a locked attribute to all models that need it.
2. Write a before_save callback that checks the locked? attribute and interrupts the transaction if it's true.

But what for the UI? We could allow users to edit even if a file is locked and instead save his changes as a draft, but this then brings merging into the mix; the alternative is to simply don't allow editing of a locked file. Which is preferable?

[jordanbyron]

Does this refer to pages or articles or both?

Ideally I'd like to make this a generic as possible so we can "lock" any record. I think this could have uses outside of Community so I'd like to keep the code as "gemifiable" as possible 😉

I think we want to block a user from accessing the record entirely if it is being edited by someone else. So in that case:

• I begin editing the about page, which updates the page's locked attribute (And perhaps locked_by)
• Greg tried to edit the same page, but is redirected back with a warning: "Jordan is editing this record"
• I finishing making my updates, and the lock fields are set to false and nil

@sandal any thoughts about this process?

[practicingruby]

@jordanbyron: Sounds right. One important feature: admins should be able to remove a lock. I.e they'd get displayed the same warning / error but have a link to click which would clear the lock. This is to prevent a record from being locked indefinitely and requiring an irb session to fix it.

Alternatively, we could put a reasonable time limit on locking, but that seems more complicated.

[jordanbyron]

@ptn why don't you finish issue #57 first, and once your done with that and no one else has claimed this ticket I'll assign it to you. Sound good?

[ptn]

I think this could have uses outside of Community so I'd like to keep the code as "gemifiable" as possible

So this goes into a module then, a la write_control.rb right?

And perhaps locked_by

Oh right, locked_by, forgot to mention that.

[ptn]

One important feature: admins should be able to remove a lock.

And when the user that had the lock tries to save after it having been removed, he is redirected to the corresponding index page with a flash informing him that he lost his lock and has to try again. In other words, the before_save callback would fail unless the user trying to save is equal to locked_by. That sounds right?

[ptn]

@jordanbyron Can it be the other way around? I could start with this and move on to #57 if it's still available; if someone beats me to it, I would of course continue to help with anything that's needed, since I had already said I'd work on that.

Can we do that?

[jordanbyron]

Sure I just didn't want to tie up two tickets at the same time. I'll assign this one to you and you can resume #57 once you are done here.

[practicingruby]

@ptn, I think the user should be kept on the edit page so that they have an opportunity to copy their contents, but I'm not sure whether that's essential. @jordanbyron can make the call.

[jordanbyron]

@sandal good call. So if an administrator "revokes" the lock bit, and the original user goes to update that record, they should be sent back to the edit page with their changes intact. Just like what happens when an validation error occurs. In fact, you'll probably want to use the ActiveRecord validations to handle this case. Then we don't have to do anything fancy.

[ericgj]

@jordanbyron @ptn I hate to bring this up at this late date, esp since the plugin looks so interesting and useful, but did you consider using postgresql's built-in locking mechanisms? Never done it myself, but just happened to look it up and what they call "advisory locks" seem kind of like just what is needed.

"PostgreSQL provides a means for creating locks that have application-defined meanings. These are called advisory locks, because the system does not enforce their use — it is up to the application to use them correctly...A common use of advisory locks is to emulate pessimistic locking strategies typical of so called "flat file" data management systems. While a flag stored in a table could be used for the same purpose, advisory locks are faster, avoid MVCC bloat, and can be automatically cleaned up by the server at the end of the session."

OTOH, might be messy to write the wrap the raw SQL for this, plus it wouldn't be usable outside of postgresql databases. Interesting to know it exists though.

[ptn]

@ericgj I didn't know that existed either.

Like you said, this wouldn't be usable outside of postgres, and the fact that we are plucking this code out into a proper gem kills the usefulness of database-specific features - we are postgres only, but client code of the gem surely will not.

Boo :-(

[jordanbyron]

@ericgj: but did you consider using postgresql's built-in locking mechanisms?

Sadly I did not, but that looks very cool and right up our alley. Personally I don't mind if we lock the gem into a Postgres only feature. If it really is fast and works as well as the little abstract makes it out to, then I think it's worth looking into.

@ptn would you mind playing around with the feature locally to see if it would work for us?

[ptn]

Personally I don't mind if we lock the gem into a Postgres only feature.

Oh, if being postgres-only is not a problem, then let's play with it. I'll let you know what I can find.

[ptn]

Guys, I'll need to postpone this until next week's Fixing Mendicant - Anything Goes, since I'll have more free time then. I'm sorry 😓

[jordanbyron]

@ptn that's ok. It looks like I won't have much time before next week to help you with this patch anyway. Let me know when you've got some more free time to work on this. 👷

[ptn]

Just a quick message to revive this: I did some research this night and I think that we can use try_advisory_lock and friends doing something similar to this article. I'm still working out the details of how the gem would manage the lock keys though.

[jordanbyron]

@ptn sounds good. Im going to close your first pull request so we can start from scratch with the new Postgres record locking approach. When you've made some progress send over a new request so I can check it out. Thanks again for reworking this 😄