Customizable :locked_at and :locked_until fields

[dks17]

• Couldn't test with mongoid 2 and 3 versions on my pc.
• Update rubocop and fix offencies.

Reference #26

[dks17]

Every time travis failures one of job buildings.

[dblock]

For rubocop in General we do rubocop -a ; rubocop --auto-gen-config.

[dblock]

I like the spirit of where this feature is going, however I would expect to be able to configure the field names "normally" and not through defining constants.

Mongoid::History.configure do |config|
  config.locked_at_field = :mongoid_locker_locked_at
  config.locked_until_field = :mongoid_locker_locked_until
end

And possibly per-class

class Foobar
  include Mongoid::Locker
  
  locker locked_at_field: :mongoid_locker_locked_at
end

If you need a reference of how to structure such configuration check out https://github.com/mongoid/mongoid-slug

[dks17]

I am afraid to use auto correction rubocop -a

I have chosen this way:

class Foobar
  include Mongoid::Locker
  
  locker locked_at_field: :mongoid_locker_locked_at
end

[dblock]

This seems to now require you call locker for every class? We don't want to force all users to do an upgrade like this.

There's no way to set this globally, which seems like a big deal for most people. I think you should implement a global config on top of this to resolve both this and the first issue.

[dks17]

Just question.
What do you think we should do to change field after it is already created by global default?

1. We have per model locker method where we configure after Mongoid-Locker is included.

class Foobar
  include Mongoid::Locker
  
  locker
  # or the same
  locker locked_at_field: :mongoid_locker_locked_at
end

2. And through global configuration.

Mongoid::History.configure do |config|
  config.locked_at_field = :mongoid_locker_locked_at
  config.locked_until_field = :mongoid_locker_locked_until
end

class Foobar
  include Mongoid::Locker
  # it is already configured by default
end

This is achieved within Locker module:

  def self.included(mod)
    mod.field :locked_at, type: Time
  end

3. And combination of both.

Mongoid::History.configure do |config|
  config.locked_at_field = :locked_at
  config.locked_until_field = :locked_until
end

class Foobar
  include Mongoid::Locker
  
  # it is already configured by default
  # field :locked_at, type: Time

  locker locked_at_field: :mongoid_locker_locked_at
  # field :mongoid_locker_locked_at
end

What we should do with already defined field :locked_at?
Should we provide for specific order of including any modules and using locker, devise methods to avoid the field :locked_at be removed or it is type or alias be changed?

We get some side effect or at least the warn:

WARN -- : Overwriting existing field locked_at in class Foobar.

In my first implementation I declared custom field through constant. It allowed us don't change any code in user application, just defined the constant in model where it really needed like devise case.

[dblock]

I think I understand the problem. This is something we faced in other gems, especially mongoid-history. The inclusion of the module currently defines the field and then a configure is called and then we want to rename the field. I see a few ways around it:

a) Raise an exception if configure updates the locked field after the first include Mongoid::Locker, force the user to call that in order. I think that's scenario 2.
b) Defer the definition of field until with_lock or locker is called. The drawback is that you don't have a locked field on the model until you need to lock it by default.

I think a) is fine and resolved a bunch of these problems. What do you think?

[dks17]

I think field definition should be extracted into model. User should be allowed define field in the model. In this way we avoid hard coded mod.field :locked_at, type: Time. Mongoid is ODM (like piece of schema for ActiveRecord) and we should not hide field inside the gem. Users should know which fields are used and for what purposes.

Yes. These changes will affect users which already used the gem. They should add only one line field :locked_at, type: Time to fix it. I think this is the way to implement that behavior. The sooner we do it the less users will face it in the future. That is what I think. And I hope it is possible.

By default:

class User
  include Mongoid:Locker

  field :locked_at, type: Time
end

Global configure

Mongoid::Locker.configure do |c|
  c.locked_at_field = :global_locker_locked_at
end

class User
  include Mongoid:Locker

  field :locked_at, type: Time # for devise
  field :global_locker_locked_at, type: Time # for locker
end

With locker

Mongoid::Locker.configure do |c|
  c.locked_at_field = :global_locked_at
end

class User
  include Mongoid:Locker

  field :locked_at, type: Time # for devise
  field :locker_locked_at, type: Time # for locker

  locker locked_at_field: :locker_locked_at
end

[dblock]

I am ok with it. Curious what @afeld thinks too (the original author). '

Maybe lock_with will now raise an exception if locker didn't define fields? Or maybe it can be lazy to preserve backwards compat?

Implement the above and add an UPGRADING doc ala https://github.com/mongoid/mongoid-history/blob/master/UPGRADING.md that describes what users have to do to this PR, bump major version?

[dks17]

I will try implement.

Maybe lock_with will now raise an exception if locker didn't define fields? Or maybe it can be lazy to preserve backwards compat?

I will add test for this:

it 'should return error if locked_at field is not defined' do
  Admin.field(:locked_until, type: Time)
  admin = Admin.create!

  expect do
    admin.with_lock do
      # no-op
    end
  end.to raise_error(Mongoid::Errors::UnknownAttribute)
end

it 'should return error if locked_until field is not defined' do
  Admin.field(:locked_at, type: Time)
  admin = Admin.create!

  expect do
    admin.with_lock do
      # no-op
    end
  end.to raise_error(Mongoid::Errors::UnknownAttribute)
end

Should we catch something special or it is enough?

[dblock]

This looks great code-wise.

I made some suggestions to cleanup docs, please make them.

There seems to be a bit of deletion going on in mongoid-locker_spec.rb. Are we losing tests? :)

[dblock]

This hasn't been released so just make it 1.0.0 (Next).

[dblock]

The locker method ..

maybe add

This can be useful when another popular library uses the same field for different purposes.

[dblock]

I would remove this paragraph, we don't like to point fingers at other small unpopular libraries who decide to call fields the same name :)

[dblock]

Mongoid::Locker no longer defines locked_at and locked_until fields when included. You must define these fields manually.

[dblock]

You can customize the fields used with a locker class method or via a global configure.

[dblock]

See ...

[dks17]

There seems to be a bit of deletion going on in mongoid-locker_spec.rb. Are we losing tests? :)

No.
I agree that is difficult to watch changes. I put all tests back into one file.

[dblock]

I am cool with refactoring tests btw!

[dblock]

I've merged this, excellent work.

@dks17 Do you want to help out with maintaining this gem? Make a release? If yes what's your rubygems email?

[dks17]

Thank you for the assist.

[dks17]

@dblock, yes, I would like to try help out with maintaining this gem.

[dblock]

Added you to collaborators (https://github.com/mongoid/mongoid-locker/invitations), added to Rubygems. Thanks. Maybe check out #56