Skip to content

Adds support for multiple column (backwards compatible)#26

Merged
pyromaniac merged 16 commits intomongoid:masterfrom
Bharat311:f-multi-col-support
Jun 23, 2014
Merged

Adds support for multiple column (backwards compatible)#26
pyromaniac merged 16 commits intomongoid:masterfrom
Bharat311:f-multi-col-support

Conversation

@Bharat311
Copy link
Copy Markdown
Contributor

This PR allows defining multiple orderable columns ( fixes #23 ) like:

class Book
  include Mongoid::Document
  include Mongoid::Orderable

  orderable
  orderable column: serial, default: true
end

The code sets up a class_attribute orderable_configurations which is a hash of all the configurations and then generates helpers like:

@book.move_serial_to :top 
@book.next_serial_items

This code change is backwards compatible, so existing implementations will also work.

@book.move_to! 3
@book.next_item

In these case, the default configuration (default: true) or the first configuration is used.

Tested it for Mongoid 2, 3, 4 on a Rails 3 app - https://github.com/bharat311/mongoid_orderable_demo

@Bharat311 Bharat311 changed the title Adds support for multiple column (backwards compatible) - fixes #23 Adds support for multiple column (backwards compatible) Jun 18, 2014
@pyromaniac
Copy link
Copy Markdown
Collaborator

Such an awesome work! Could you please just fix 1.8.7 support and I'll merge it. I just don't want to reject 1.8.7 support because it costs almost nothing to bring it to gem.

Just replace all the new-style hashes with the old ones (https://travis-ci.org/pyromaniac/mongoid_orderable/jobs/27887206) please. Thanks in advance.

@Bharat311
Copy link
Copy Markdown
Contributor Author

Hi @pyromaniac, thanks for the appreciation :)

Ruby 1.8.7 does not provide support for default arguments in #define_method, so i'll need to convert it to #class_eval for such methods.

Also, in ruby 1.8, to make an argument have a default value, the next argument(s) must also have a default value. so basically method such as:

def move_to!(column = :default, pos)
end

will also need to be changed.

I am agnostic to above changes, but not sure whether should we continue with the ruby 1.8 support ?

@pyromaniac
Copy link
Copy Markdown
Collaborator

We can drop it if it will be impossible to implement. So do something with it ;)

@Bharat311
Copy link
Copy Markdown
Contributor Author

Cool... I'll work on it over the next couple of days and see how it goes.

@pyromaniac
Copy link
Copy Markdown
Collaborator

Wonderful, thanks!

@johnnyshields
Copy link
Copy Markdown
Member

@pyromaniac let's drop Ruby 1.8 support. Ruby 1.8.x is 1 year past end-of-life, and it is not supported by Mongoid 3. I do not know of many other gems still supporting it.

@pyromaniac
Copy link
Copy Markdown
Collaborator

Let's do it!

@Bharat311
Copy link
Copy Markdown
Contributor Author

@pyromaniac - i have fixed the build for ruby 1.8 also, it should be good to merge now.

pyromaniac added a commit that referenced this pull request Jun 23, 2014
Adds support for multiple column (backwards compatible)
@pyromaniac pyromaniac merged commit 8616c22 into mongoid:master Jun 23, 2014
@pyromaniac
Copy link
Copy Markdown
Collaborator

So awesome, thanks!

@Bharat311
Copy link
Copy Markdown
Contributor Author

Thanks for the merge @pyromaniac

I think it would also be good to bump and release a new version of mongoid_orderable with the above changes, so i can use the released version directly.

@pyromaniac
Copy link
Copy Markdown
Collaborator

One more PR to merge

@j-miyake
Copy link
Copy Markdown

@Bharat311 - Hi, thank you for an awesome PR.
I would like to ask you how I workaround against a problem with this change.
I pulled the revision including those commits, then I got an error when I used inheritance. I have an abstract model and some inherited models for STI. I defined orderable configuration at the abstract model, for example:

class Foo
  include Mongoid::Document
  include Mongoid::Orderable
  orderable column: :position
end

class Bar < Foo
end

Before I pulled the latest revision, Bar class works with the orderable configuration I defined in Foo class.
However, after pulled, I got an error: NoMethodError: undefined method detect' for nil:NilClasswhen updating the position of aBardocument withmove_to!or specifying position number. The stacktrace when I usedmove_to!to modify the position of an instance ofBar` is here.

[1] pry> e.backtrace
=> ["/usr/local/rvm/gems/ruby-2.0.0-p451/bundler/gems/mongoid_orderable-8616c226798a/lib/mongoid/orderable/helpers.rb:10:in `default_orderable_column'",
 "/usr/local/rvm/gems/ruby-2.0.0-p451/bundler/gems/mongoid_orderable-8616c226798a/lib/mongoid/orderable/movable.rb:52:in `move_column_to'",
 "/usr/local/rvm/gems/ruby-2.0.0-p451/bundler/gems/mongoid_orderable-8616c226798a/lib/mongoid/orderable/movable.rb:6:in `move_to!'",
 . . .

I examined if the Bar class inherits the orderable configuration, then I found that it doesn’t. When I defined orderable in Bar directly, it works fine.

$ rails c
Loading development environment (Rails 4.0.0)
[1] pry(main)> class Foo
[1] pry(main)*   include Mongoid::Document  
[1] pry(main)*   include Mongoid::Orderable  
[1] pry(main)*   orderable column: :position  
[1] pry(main)* end  
=> [Foo]
[2] pry(main)> class Bar < Foo
[2] pry(main)* end  
=> nil
[3] pry(main)> Foo.orderable_configurations
=> {:position=>
  {:column=>:position,
   :index=>true,
   :scope=>nil,
   :base=>1,
   :field_opts=>{:type=>Integer}}}
[4] pry(main)> Bar.orderable_configurations
=> nil
[5] pry(main)> Bar.create position: 1
=> #<Bar _id: 53a91e497562751a31030000, position: 1, _type: "Bar">
[6] pry(main)> Bar.create position: 2
=> #<Bar _id: 53a91e4a7562751a31040000, position: 2, _type: "Bar">
[7] pry(main)> Bar.create position: 3
=> #<Bar _id: 53a91e4b7562751a31050000, position: 3, _type: "Bar">
[8] pry(main)> Bar.last.move_to! 2
NoMethodError: undefined method `detect' for nil:NilClass
from /usr/local/rvm/gems/ruby-2.0.0-p451/bundler/gems/mongoid_orderable-8616c226798a/lib/mongoid/orderable/helpers.rb:10:in `default_orderable_column'
[11] pry(main)> class Bar
[11] pry(main)*   orderable column: :position
[11] pry(main)* end  
=> [Bar]
[12] pry(main)> Bar.create position: 4
=> #<Bar _id: 53a91ec17562751a31060000, position: 4, _type: "Bar">
[13] pry(main)> Bar.create position: 5
=> #<Bar _id: 53a91ec27562751a31070000, position: 5, _type: "Bar">
[14] pry(main)> Bar.create position: 6
=> #<Bar _id: 53a91ec47562751a31080000, position: 6, _type: "Bar">
[15] pry(main)> Bar.last.move_to! 5
=> true
[16] pry(main)> Bar.find_by position: 5
=> #<Bar _id: 53a91ec47562751a31080000, position: 5, _type: "Bar">
[17] pry(main)> 

Are there any way to define the configuration in the abstract class and inherit it without defining again in the inherited classes?

Thanks!

@pyromaniac
Copy link
Copy Markdown
Collaborator

I thinks it might be fixed by using class_attribute :orderable_configurations.

@j-miyake
Copy link
Copy Markdown

@Bharat311 Bharat311 deleted the f-multi-col-support branch August 29, 2014 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Is it possible to add orderable to multiple columns on a single model?

4 participants