11

In the edit method of many controllers you initialize a new object and edit existing objects

class MagazinesController < ApplicationController
   def edit
      @magazine = Magazine.find(params[:magazine_id])
      @page = Page.find(params[:id])
      @new_page = @magazine.pages.new
   end
end

However in a view you will often want to cycle through the persisted objects and treat the new object separately

# magazines#edit
%h4 Existing pages
- @magazine.pages.each do |page|
  %p= link_to page, page.title

The problem

...is that the pages association contains both existing (persisted) pages but also the new page which we made via @new_page = @magazine.pages.new.

It's easy to deal with this however it's ugly

%h4 Existing pages
- @magazine.pages.each do |page|
  - if page.persisted?
    %p= link_to page, page.title

I would like to use some assocition method to select only those pages which are persisted:

%h4 Existing pages
- @magazine.pages.persisted.each do |page|
  %p= link_to page, page.title

Is there any way of doing this?

1
  • 4
    Did you tried @magazine.pages.select(&:persisted?)
    – taro
    Commented Nov 7, 2013 at 17:42

6 Answers 6

17

You can create in your Page model a persisted scope: scope :persisted, -> { where.not(id: nil) }, which avoids iterating on each associated page to check whether it's a new record or not.

6
  • That's what I was thinking too Florent. My only concern with this as a long term approach is that it hits the db which doesn't seem necessary given that the records are already in an array in memory. However, I'm wondering if the right approach might be to create a .persisted method on the model and then do a mapping def persisted pages.collect{|page| page if page.persisted?}. What do you think? The downside of course is that it ceases to be an AR relation, the nice thing is that it's fast. (I've missed our code convos btw ;) Commented Nov 8, 2013 at 9:36
  • Ahh, I see that my suggestion was exactly what @CDub suggested! Commented Nov 8, 2013 at 9:42
  • Don't know exactly how is your view, but in case in %h4 Existing pages it's the first time you iterate on pages, it's fine. At this moment the pages are not in memory yet, @magazine.pages is just an ActiveRelation object which is lazy loaded, so adding .persisted will change just the SQL query executed to fetch the record (by adding an extra condition on the id), not generate an extra SQL request (I miss our code convos too :) ).
    – Florent2
    Commented Nov 8, 2013 at 19:22
  • Good point - I forgot about lazy loading. Which route do you prefer out of interest - filtering the array or changing the query? Commented Nov 11, 2013 at 13:19
  • If I face the issue only in a single view, I would probably just put - next if page.new_record? after - @magazine.pages.each do |page|, (I admit that's ugly :) ) else would go with the persisted scope. Too bad that Rails inject the new record in the array :S EDIT: seems that you can use the native scoped instead of the custom persistedto achieve the same result (but the semantic is not straightforward).
    – Florent2
    Commented Nov 11, 2013 at 18:48
4

Both the suggestions from @Florent2 and @CDub are sound. However @florent2's suggestion meant hitting the database again (and potentially ditching any preset eager loading which I didn't want to do) and @CDub's suggestion didn't quite work out in terms of code. Here is what I ended up going with:

Returning only persisted records for a particular association

class Magazine < ActiveRecord::Base
  has_many :pages do 
    def persisted
      collect{ |page| page if page.persisted? }
    end
  end
end

this allows you to call .persisted on any ActiveRecord relation of pages associated with Magazine. It doesn't hit the database again as it simply filters through the pre-loaded objects returning the ones which are persisted.

Making the code reusable

Since I want to reuse this code on a regular basis I can pull it out into a module

module PersistedExtension
  def persisted
    select{|item| item if item.persisted?}
  end
end

It can then be included into the association methods using a lambda:

class Magazine < ActiveRecord::Base
  # ...
  has_many :pages, -> { extending PersistedExtension }

end

and I can call it intuitively:

@magazine = Magazine.first

@magazine.pages.persisted
# => array of pages which are persisted

# the new persisted association extension works on any AR result set
@magazine.pages.order('page ASC').persisted
2
  • 1
    I needed to compact the array because I was getting nil returned. collect{ |page| page if page.persisted? }.compact
    – dazonic
    Commented May 30, 2014 at 3:05
  • 11
    @dazonic select(&:persisted?) returns the same result and looks much cleaner. See documentation for Enumerable#select.
    – D-side
    Commented Mar 12, 2015 at 13:29
3

You could always reject pages that are new records...

%h4 Existing pages
- @magazine.pages.persisted.each do |page|
    %p= link_to page, page.title

where on Page you'd have something like:

def self.persisted
  reject {|page| page.new_record? }
end
1
  • This is exactly what I was thinking about. The downside is that it ceases to be an AR relation, the upside is that if the data's already in memory it doesn't bother hitting up the DB again. Commented Nov 8, 2013 at 9:43
2

I approach this problem differently. I do not create a new object in the controller, but instead do so directly in the form.

First, to run through your controller, why are you passing the page_id as your primary params[:id] to your Magazines controller? Looks to me that you want this:

class MagazinesController < ApplicationController
   def edit
      @magazine = Magazine.find(params[:id]).includes(:pages)
   end
end

Then, in your magazines#edit view, you'd do this:

%h4 Existing pages
- @magazine.pages.each do |page|
  %p= link_to page, page.title

= form_for @magazine do |f|
  = f.fields_for :pages, @magazine.pages.build do |builder|
    = builder.text_field :title
    # etc.

In that fields_for line, you're asking for magazine-form fields for pages, but then telling it to only render the fields for a specific, new page, which you are creating on the fly with @magazine.pages.build.

References:
fields_for
Nested Model Form Railscast (See also Part 2)

5
  • Hi Carlos. This is definitely a reasonable approach however I always prefer to instantiate objects in the controller than in the view. I can never remember whether there are consequences in this case but in general it feels cleaner. BTW I actually use cancan to load_and_authorize_resource - I put the object loading stuff in there just to illustrate (p.s. I enjoyed your goals blogpost) Commented Nov 8, 2013 at 9:41
  • Also you only push the problem downstream here. Once you've instantiated the new object it will still affect anything else which references the .pages association. The only difference is that it's only view code downstream of it which will be affected Commented Nov 8, 2013 at 10:33
  • Hmm, you are right about pushing the problem downstream. Having an implementation dependent on order of elements in the view is bad. That said, I don't like having a single controller action responsible for multiple instances variables on multiple classes. Such, in my code, is usually a sign that I'm approaching a problem incorrectly. Commented Nov 8, 2013 at 20:05
  • Is there any chance you could use a javascript-action, via an in-form link, to instantiate new fields for a page object? That way, the Ruby isn't responsible at all for creating a new object, and @magazine.pages never gets polluted. See link_to_add_fields here: railscasts.com/episodes/197-nested-model-form-part-2 Commented Nov 8, 2013 at 20:08
  • I could probably do something like that but unless there's a good reason not to, I always prefer to solve problems without JS before stepping things up. I feel that using JS in this instance will only muddy the waters - it's a generic problem Commented Nov 11, 2013 at 13:11
2

Rails 4 and 5 answer:

Just put this code in initializer (file in config/initializers directory, with .rb extension):

module MyApp
  module ActiveRecordExtensions
    extend ActiveSupport::Concern

    class_methods do

      def persisted
        select(&:persisted?)
      end

    end
  end
end

ActiveSupport.on_load :active_record do
  include MyApp::ActiveRecordExtensions
end

You can now call persisted on any model and association.

2
  • 2
    You should not be accessing ActiveRecord::Base directly, and instead use load hooks: ActiveSupport.on_load :active_record { include MyApp::ActiveRecordExtensions }
    – Koen.
    Commented Jul 28, 2016 at 21:38
  • Yep, it's a good idea to not force ActiveRecord::Base to load too early. Here is a blog post explaining why.
    – TeWu
    Commented Jul 29, 2016 at 9:07
2

Another cleaner syntax, using ActiveRecord where.not and still get back an ActiveRecord collection:

- @magazine.pages.where.not(id: nil).each do |page|
    ...

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.