Conditionally excluding childs #367

Closed
josegrad opened this Issue Nov 25, 2012 · 16 comments

Comments

Projects
None yet
5 participants
@josegrad

I'm generating JSON with Rabl but I can find a way to eliminate some childs based on a condition. How could I do that based with the structure I have below?

if @subcompanies
    child @subcompanies do
        attributes :id, :name

        child :indicators do <<<<< Dont include an indicador (and childs) if the indicator id == 22
          attributes :id

          child :dimensions do
              attributes :name

              child :elements do
                  attributes :id, :name
              end

          end   
        end 
    end
end

Thanks.

@databyte

This comment has been minimized.

Show comment Hide comment
@databyte

databyte Nov 25, 2012

Collaborator

Child takes a conditional argument like node does.

So you can write:

child :indicators, unless: lambda {|x| x.id == 22 } do |x|
  attributes :id
end

Though to be honest, 22 is a magic number and the condition reads oddly so you should really put something on the object itself. But maybe that was just a simple example for yourself.

Collaborator

databyte commented Nov 25, 2012

Child takes a conditional argument like node does.

So you can write:

child :indicators, unless: lambda {|x| x.id == 22 } do |x|
  attributes :id
end

Though to be honest, 22 is a magic number and the condition reads oddly so you should really put something on the object itself. But maybe that was just a simple example for yourself.

@josegrad

This comment has been minimized.

Show comment Hide comment
@josegrad

josegrad Nov 26, 2012

Thanks databyte.

Yeah before asking I was playing with if and unless but didn't get what I expected. Some examples below.

object @company
attributes :id, :name

child :indicators => "indicators" do 
    attributes :id => "id", :name => "name"
end

As expected produces:

{
    "id": 100, 
    "indicators": [
        {
            "indicator": {
                "id": 1431, 
                "name": "Dibujos"
            }
        }, 
        {
            "indicator": {
                "id": 1432, 
                "name": "Pasta"
            }
        }, 
        {
            "indicator": {
                "id": 1436, 
                "name": "Private company indicator"
            }
        }
    ], 
    "name": "Privado Base"
}

Then using unless:

child :indicators => "indicators", unless: lambda { |m| m.id == 1431 } do |m| 
    attributes :id => "id", :name => "name"
end

Produces:

{
    "id": 100, 
    "indicators": [
        {
            "indicator": {}
        }, 
        {
            "indicator": {}
        }
    ], 
    "name": "Privado Base"
}

I can see that one indicator element was removed. But The other remaining ones are not including the attributes they should.


Then in reality I want some childs of the indicators besides the attributes:

child :indicators => "indicators", unless: lambda { |m| m.id == 1431 } do |m| 
    attributes :id => "id", :name => "name"
    child :dimensions => "dimensions" do
        attributes :name
    end 
end

And that gives me the error:

(FATAL) 866 ActionView::Template::Error (undefined method `dimensions' for [:indicators, "indicators"]:Array):    1:     2:     3: object @company    

If I don't use the unless (or if) then the childs are properly collected and displayed.


Furthermore I'm also adding a node conditionally and trying to have a deeper nesting. But for the same reason as the one above the node insertion fails.

child :indicators => "indicators", unless: lambda { |m| m.id == 1431 } do |m| 
    attributes :id => "id", :name => "name"
    node(:belongs_to_sector){|u| u.is_predefined == 9 ? 0 : 1}

    child :dimensions => "dimensions" do
        attributes :name
        child :elements => "elements" do
            attributes :id, :name
        end
    end 
end

With error:

(FATAL) 866 ActionView::Template::Error (undefined method `is_predefined' for [:indicators, "indicators"]:Array):    1:     2:     3: object @company 

I must be missing something...

Thanks databyte.

Yeah before asking I was playing with if and unless but didn't get what I expected. Some examples below.

object @company
attributes :id, :name

child :indicators => "indicators" do 
    attributes :id => "id", :name => "name"
end

As expected produces:

{
    "id": 100, 
    "indicators": [
        {
            "indicator": {
                "id": 1431, 
                "name": "Dibujos"
            }
        }, 
        {
            "indicator": {
                "id": 1432, 
                "name": "Pasta"
            }
        }, 
        {
            "indicator": {
                "id": 1436, 
                "name": "Private company indicator"
            }
        }
    ], 
    "name": "Privado Base"
}

Then using unless:

child :indicators => "indicators", unless: lambda { |m| m.id == 1431 } do |m| 
    attributes :id => "id", :name => "name"
end

Produces:

{
    "id": 100, 
    "indicators": [
        {
            "indicator": {}
        }, 
        {
            "indicator": {}
        }
    ], 
    "name": "Privado Base"
}

I can see that one indicator element was removed. But The other remaining ones are not including the attributes they should.


Then in reality I want some childs of the indicators besides the attributes:

child :indicators => "indicators", unless: lambda { |m| m.id == 1431 } do |m| 
    attributes :id => "id", :name => "name"
    child :dimensions => "dimensions" do
        attributes :name
    end 
end

And that gives me the error:

(FATAL) 866 ActionView::Template::Error (undefined method `dimensions' for [:indicators, "indicators"]:Array):    1:     2:     3: object @company    

If I don't use the unless (or if) then the childs are properly collected and displayed.


Furthermore I'm also adding a node conditionally and trying to have a deeper nesting. But for the same reason as the one above the node insertion fails.

child :indicators => "indicators", unless: lambda { |m| m.id == 1431 } do |m| 
    attributes :id => "id", :name => "name"
    node(:belongs_to_sector){|u| u.is_predefined == 9 ? 0 : 1}

    child :dimensions => "dimensions" do
        attributes :name
        child :elements => "elements" do
            attributes :id, :name
        end
    end 
end

With error:

(FATAL) 866 ActionView::Template::Error (undefined method `is_predefined' for [:indicators, "indicators"]:Array):    1:     2:     3: object @company 

I must be missing something...

@nesquena

This comment has been minimized.

Show comment Hide comment
@nesquena

nesquena Nov 26, 2012

Owner

Try this:

child({ :indicators => "indicators" }, unless: lambda { |m| m.id == 1431 }) do |m| 
    attributes :id => "id", :name => "name"
end

almost all the issues you mentioned stem from the (admittedly somewhat confusing) fact that you need to pass unless as a separate options hash. This will be fixed but for now, try it as above and it will fix the issues mentioned.

Owner

nesquena commented Nov 26, 2012

Try this:

child({ :indicators => "indicators" }, unless: lambda { |m| m.id == 1431 }) do |m| 
    attributes :id => "id", :name => "name"
end

almost all the issues you mentioned stem from the (admittedly somewhat confusing) fact that you need to pass unless as a separate options hash. This will be fixed but for now, try it as above and it will fix the issues mentioned.

@josegrad

This comment has been minimized.

Show comment Hide comment
@josegrad

josegrad Nov 26, 2012

Hi, thanks nesquena. That was included among all the things I tried. I gave it another try, so using:

object @company
attributes :id, :name

child({ :indicators => "indicators" }, unless: lambda { |m| m.is_predefined == 9 }) do |m| 
    attributes :id => "id", :name => "name" 
end

Produces the error:

(FATAL) 3897 ActionView::Template::Error (undefined method `is_predefined' for #<Company:0x000001036cb7a0>)

is_prefefined is a column of the indicators table but the m represented in the lambda is apparently the root object @Company and of course it fails.

Based on the wiki that should be the case for node but childs should allow us to navigate the associations. Well it does if I don't use lambda.

Hi, thanks nesquena. That was included among all the things I tried. I gave it another try, so using:

object @company
attributes :id, :name

child({ :indicators => "indicators" }, unless: lambda { |m| m.is_predefined == 9 }) do |m| 
    attributes :id => "id", :name => "name" 
end

Produces the error:

(FATAL) 3897 ActionView::Template::Error (undefined method `is_predefined' for #<Company:0x000001036cb7a0>)

is_prefefined is a column of the indicators table but the m represented in the lambda is apparently the root object @Company and of course it fails.

Based on the wiki that should be the case for node but childs should allow us to navigate the associations. Well it does if I don't use lambda.

@databyte

This comment has been minimized.

Show comment Hide comment
@databyte

databyte Nov 27, 2012

Collaborator

The passed in value is the root object indeed. The documentation for child nodes states that "you can also pass in the current object" which means the main object at the top.

Another recent issue had the same manipulation of conditionals in the template to present the data desired. I think it makes the template ugly fast and the logic hard to follow when the view itself contains logic branches itself.

As linked into that issue, I commented that a cleaner approach would be a presentation object or PORO (plain old ruby object) that contained the logic such that navigating to the "child" would call into the conditional you seek within the object and not the view. Similar to the idea of skinny controllers and fat models but instead of a really fat single model called company, you have a smaller "model" that helps control the presentation of company into the template.

Then you can reuse the same basic company model and company template with different presenters based on your needs. Maybe sometimes you need to check for is_predefined or sometimes it's if the company has more than 2 indicators. Who knows right?

Hope that makes sense.

@nesquena -

I had the pleasure of seeing Matz speak at RubyConf on stage and off. One of the things that he mentioned again (and previously as well) was intentionally designing parts of Ruby to be ugly so it would still work but make it feel "off". Which then makes it easier to identify what should change because you see the smell.

A week ago, I thought that passing in a conditional as an argument or some of the limited functionality in the DSL as something that needed to be fixed. But the more and more I see examples of truly complex templates, the more I think we should do better to provide great examples of model, presenter and view/template. Instead of model and template with many hacks and conditionals. Maybe it's purely a documentation and code sample problem?

I think we're due for a Skype/Hangout at some point to light a fire under this v1 milestone. How's Dec look? 😀

Collaborator

databyte commented Nov 27, 2012

The passed in value is the root object indeed. The documentation for child nodes states that "you can also pass in the current object" which means the main object at the top.

Another recent issue had the same manipulation of conditionals in the template to present the data desired. I think it makes the template ugly fast and the logic hard to follow when the view itself contains logic branches itself.

As linked into that issue, I commented that a cleaner approach would be a presentation object or PORO (plain old ruby object) that contained the logic such that navigating to the "child" would call into the conditional you seek within the object and not the view. Similar to the idea of skinny controllers and fat models but instead of a really fat single model called company, you have a smaller "model" that helps control the presentation of company into the template.

Then you can reuse the same basic company model and company template with different presenters based on your needs. Maybe sometimes you need to check for is_predefined or sometimes it's if the company has more than 2 indicators. Who knows right?

Hope that makes sense.

@nesquena -

I had the pleasure of seeing Matz speak at RubyConf on stage and off. One of the things that he mentioned again (and previously as well) was intentionally designing parts of Ruby to be ugly so it would still work but make it feel "off". Which then makes it easier to identify what should change because you see the smell.

A week ago, I thought that passing in a conditional as an argument or some of the limited functionality in the DSL as something that needed to be fixed. But the more and more I see examples of truly complex templates, the more I think we should do better to provide great examples of model, presenter and view/template. Instead of model and template with many hacks and conditionals. Maybe it's purely a documentation and code sample problem?

I think we're due for a Skype/Hangout at some point to light a fire under this v1 milestone. How's Dec look? 😀

@nesquena

This comment has been minimized.

Show comment Hide comment
@nesquena

nesquena Nov 27, 2012

Owner

Yeah I think you are right, I normally keep this type of complex logic in POROs as well. I think some basic simple conditionals are OK in a view but I do agree taking it too far is a definitive smell. It's no different then any other view. Think of HTML, ERB, HAML, etc. Anyone can stick way too much complexity into a view, and it is a smell. It's no different with RABL just as with any view.

I think you are totally right though we do need better examples of keeping the view clean and properly using presenters. RABL like any view was never intended to be hammered with complex logic. Perhaps we need to add a page to the wiki or even a note to the readme cautioning against excessive view logic and explaining how to use PORO to keep things clean and testable?

I totally agree about getting v1 on the road in december. Let's figure out a weekend or two in mid-december to work out all the details. I look forward to cleaning things up and getting out a solid 1.0 release.

Owner

nesquena commented Nov 27, 2012

Yeah I think you are right, I normally keep this type of complex logic in POROs as well. I think some basic simple conditionals are OK in a view but I do agree taking it too far is a definitive smell. It's no different then any other view. Think of HTML, ERB, HAML, etc. Anyone can stick way too much complexity into a view, and it is a smell. It's no different with RABL just as with any view.

I think you are totally right though we do need better examples of keeping the view clean and properly using presenters. RABL like any view was never intended to be hammered with complex logic. Perhaps we need to add a page to the wiki or even a note to the readme cautioning against excessive view logic and explaining how to use PORO to keep things clean and testable?

I totally agree about getting v1 on the road in december. Let's figure out a weekend or two in mid-december to work out all the details. I look forward to cleaning things up and getting out a solid 1.0 release.

@nesquena

This comment has been minimized.

Show comment Hide comment
@nesquena

nesquena Nov 27, 2012

Owner

@databyte Added a wiki here based on your latest explanation about presenters: https://github.com/nesquena/rabl/wiki/Managing-complexity-with-presenters. Let me know what you think

Owner

nesquena commented Nov 27, 2012

@databyte Added a wiki here based on your latest explanation about presenters: https://github.com/nesquena/rabl/wiki/Managing-complexity-with-presenters. Let me know what you think

@nesquena

This comment has been minimized.

Show comment Hide comment
@nesquena

nesquena Nov 27, 2012

Owner

Just to add to the topic here, I definitely recommend in this case of filtering the children, always use a named scope, or add a model method or create a presenter. For example, instead of:

child({ :indicators => "indicators" }, unless: lambda { |m| m.is_predefined == 9 }) do |m| 
  attributes :id => "id", :name => "name" 
end

Do this:

child :predefined_indicators => "indicators" do |m| 
  attributes :id => "id", :name => "name" 
end

and then:

class Company
  def predefined_indicators
    self.indicators.select { |i| i.is_predefined == 9 }
  end
end 

or in more complex cases using a presenter. This way things are better organized, easier to maintain and much more easily testable.

Owner

nesquena commented Nov 27, 2012

Just to add to the topic here, I definitely recommend in this case of filtering the children, always use a named scope, or add a model method or create a presenter. For example, instead of:

child({ :indicators => "indicators" }, unless: lambda { |m| m.is_predefined == 9 }) do |m| 
  attributes :id => "id", :name => "name" 
end

Do this:

child :predefined_indicators => "indicators" do |m| 
  attributes :id => "id", :name => "name" 
end

and then:

class Company
  def predefined_indicators
    self.indicators.select { |i| i.is_predefined == 9 }
  end
end 

or in more complex cases using a presenter. This way things are better organized, easier to maintain and much more easily testable.

@josegrad

This comment has been minimized.

Show comment Hide comment
@josegrad

josegrad Nov 27, 2012

I added the model method.

I knew this issue could be solved that way, or in other ways since it is not a serious problem. But I wanted to know if such selection was possible do as I was trying. I do completely agree about keeping logic and complexity out of the views, but somehow I cannot say this was really adding complex logic into the view. In any case I cannot argue that using a model method is way better than pushing the conditional right there.

This discussion opened my eyes to decorators and presenters. I have great plans for Draper already.

Thanks guys.

I added the model method.

I knew this issue could be solved that way, or in other ways since it is not a serious problem. But I wanted to know if such selection was possible do as I was trying. I do completely agree about keeping logic and complexity out of the views, but somehow I cannot say this was really adding complex logic into the view. In any case I cannot argue that using a model method is way better than pushing the conditional right there.

This discussion opened my eyes to decorators and presenters. I have great plans for Draper already.

Thanks guys.

@nesquena nesquena closed this Dec 6, 2013

@robguilfoyle

This comment has been minimized.

Show comment Hide comment
@robguilfoyle

robguilfoyle Dec 22, 2013

I don't necessarily agree with the conclusion of this. I think it adds more complexity to have another model method if you are trying to limit based on one conditional and it only concerns the api. Also what if you need to pass a variable to the instance method. In my case I have a Property model that has many leases, but I only want to see leases that the current user is attached too.

I don't necessarily agree with the conclusion of this. I think it adds more complexity to have another model method if you are trying to limit based on one conditional and it only concerns the api. Also what if you need to pass a variable to the instance method. In my case I have a Property model that has many leases, but I only want to see leases that the current user is attached too.

@catapultsoftworks

This comment has been minimized.

Show comment Hide comment
@catapultsoftworks

catapultsoftworks May 25, 2014

Similar issue here, but I want to conditionally exclude children based on properties of the child rather than parent. Example:

object @person
child :pets do
    extends 'api/pets/show'
end

The child block yields only the parent (person in this case). In 'api/pets/show', I can reference root_object and do my conditional, but this is not the best solution because a) it still produces an empty bracket set {} and b) the conditional does not necessarily apply if you view the child (pet) directly.

Using root_object in the pet show template, I get this:

[ 
  { first_name: "John",
     last_name: "Doe",
     pets: [ 
        { }
     ]
  }
]

I want pets to be [ ] with no object braces inside.

Is there a way to do this? If not, a great extension to RABL syntax would be to yield the child also, like this...

object @person
child :pets |person, pet| do
    extends 'api/pets/show' unless pet.is( :cat )
end

Similar issue here, but I want to conditionally exclude children based on properties of the child rather than parent. Example:

object @person
child :pets do
    extends 'api/pets/show'
end

The child block yields only the parent (person in this case). In 'api/pets/show', I can reference root_object and do my conditional, but this is not the best solution because a) it still produces an empty bracket set {} and b) the conditional does not necessarily apply if you view the child (pet) directly.

Using root_object in the pet show template, I get this:

[ 
  { first_name: "John",
     last_name: "Doe",
     pets: [ 
        { }
     ]
  }
]

I want pets to be [ ] with no object braces inside.

Is there a way to do this? If not, a great extension to RABL syntax would be to yield the child also, like this...

object @person
child :pets |person, pet| do
    extends 'api/pets/show' unless pet.is( :cat )
end
@nesquena

This comment has been minimized.

Show comment Hide comment
@nesquena

nesquena May 25, 2014

Owner

@catapultsoftworks you are aware of using a model method to do that filtering? So you could just add a method which excludes the records you don't want before you even get to the rabl template. That said, I'm not discounting your suggestion simply saying its an easy workaround.

Owner

nesquena commented May 25, 2014

@catapultsoftworks you are aware of using a model method to do that filtering? So you could just add a method which excludes the records you don't want before you even get to the rabl template. That said, I'm not discounting your suggestion simply saying its an easy workaround.

@catapultsoftworks

This comment has been minimized.

Show comment Hide comment
@catapultsoftworks

catapultsoftworks May 25, 2014

Thanks for the reply. I did try that, but maybe I'm not doing it right:

object @person
root_object.pets_filtered.each do |pet|
    extends 'api/pets/show', object: pet
end

I actually get some quite strange behavior in this case (even when I just use root_object.pets, so I know it's not my filter method). It shows a sub-object of "pets" instead. Something like this:

[ 
  { first_name: "John",
     last_name: "Doe",
     shots: [ 
        { }
     ]
  }
]

Assuming "shots" is a child of "pets." My example is getting strained a bit. :-) So... is my use of "extends" in this case is incorrect?

Thanks for the reply. I did try that, but maybe I'm not doing it right:

object @person
root_object.pets_filtered.each do |pet|
    extends 'api/pets/show', object: pet
end

I actually get some quite strange behavior in this case (even when I just use root_object.pets, so I know it's not my filter method). It shows a sub-object of "pets" instead. Something like this:

[ 
  { first_name: "John",
     last_name: "Doe",
     shots: [ 
        { }
     ]
  }
]

Assuming "shots" is a child of "pets." My example is getting strained a bit. :-) So... is my use of "extends" in this case is incorrect?

@nesquena

This comment has been minimized.

Show comment Hide comment
@nesquena

nesquena May 25, 2014

Owner

That approach doesn't work, since you've never called child there. Try:

object @person
child :pets_filtered => :pets do
  extends 'api/pets/show' 
end

and you should be good to go.

Owner

nesquena commented May 25, 2014

That approach doesn't work, since you've never called child there. Try:

object @person
child :pets_filtered => :pets do
  extends 'api/pets/show' 
end

and you should be good to go.

@catapultsoftworks

This comment has been minimized.

Show comment Hide comment
@catapultsoftworks

catapultsoftworks May 25, 2014

Perfect! I had to use the reference to root_object to make it work without error:

object @person
child root_object.pets_filtered => :pets do
    extends 'api/pets/show' 
end

Thanks very much for responding to my questions. RABL is a great help in my project!

Perfect! I had to use the reference to root_object to make it work without error:

object @person
child root_object.pets_filtered => :pets do
    extends 'api/pets/show' 
end

Thanks very much for responding to my questions. RABL is a great help in my project!

@nesquena

This comment has been minimized.

Show comment Hide comment
@nesquena

nesquena May 26, 2014

Owner

Glad you got it working!

Owner

nesquena commented May 26, 2014

Glad you got it working!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment