Conditional attributes/associations (if/unless)

[beauby]

My take at conditional attributes/associations. Previous work has been done in this direction (#1266), but I figured it was easily expressed on top of #1370.
This PR adds support for if/unless options to attribute/association definitions in serializers. Those options take a symbol of a method name on the serializer.

Example:

class UserSerializer < ActiveModel::Serializer
  attributes :name, :description, :date_of_birth
  attribute :private_data, if: :is_current_user?

  def is_current_user?
    object.id == current_user.id
  end
end

Note: if this gets traction, I'll add tests.

Ref #1245, #1266, #1058, #922, #1141

[bf4]

public_send ?

[beauby]

Just updated it.

[bf4]

Looks like a good approach to me.

[bf4]

since the :key, :condition_type, :condition attributes to an initializer lumped as options (which I initially missed), why not just take in options and create our own accessors for key, condition_type, condition?

[beauby]

That's indeed a possibility. The rationale was to do the work only once.

[bf4]

By 'work' you mean defining accessors for the options? Ok, I get that. I think I'd rather add an attr_accessor than change the Struct definition if I needed to change the options. It took me a couple of reads of the object before I realized you were treating most of the struct attributes as options. (I'm not new to subclassing the initialize... I just usually use it for setting defaults for unset values, rather than as implicit accessors).

Do you foresee taking advantage of the struct's comparable properties? I'm trying to balance the clever vs. useful in my head.

[bf4]

Also ref #1333

[bf4]

yet another sign there's a common ancestor or module

[beauby]

Agreed, we could either make a Field class from which Attribute and Reflection would inheritate, or a ConditionalField mixin. Not sure what's best, and naming is obviously up for debate.

[bf4]

I think regular inheritance makes sense in this case (if we include more than just the if/else)

[bf4]

Looking good.. let's finish #1370 and add some tests and docs here :)

[bf4]

@beauby #1370 is merged, so let's rebase, add tests, and docs!

[beauby]

Kept it as a struct because tests in test/serializers/association_macros_test.rb made use of the comparability. Wouldn't mind modifying those in a subsequent PR.

[beauby]

PR ready.

[bf4]

grammar: s/i.e./e.g.

reason: meaning is to change 'that is' to 'for example'

[beauby]

I actually meant i.e., because I'm just defining what "field" means.

[bf4]

still think it should be e.g., but since it's intentional on your part and it's super unimportant, ok to merge when green

[beauby]

Agreed that it's not super important, but as I like grammar as well: a field is defined as "either an attribute or an association".

[bf4]

@beauby two small comments, one grammar, one internal api. 💯 LGTM

[Mohakjuneja]

Is the support for if/unless already implemented in the master? I don't want my subjects association to be included when the controller method is index
Here's what I tried-

class Admin::SemesterSerializer < ActiveModel::Serializer

  attributes :_id, :semester, :course_id

  has_many :subjects, serializer: Admin::SubjectSerializer, unless: :index

end

But I get the error undefined method `index' for #<Admin::SemesterSerializer

[NullVoxPopuli]

@Mohakjuneja the serializer doesn't have access to the controller's methods, as the passed in scope defaults to current_user

Have you seen these docs? https://github.com/rails-api/active_model_serializers/blob/master/docs/general/serializers.md#scope

maybe they could help?

[Mohakjuneja]

I figured it out! I apologize for being impatient and posting the issue too soon. AMS has consumed a lot of my time upgrading from 0.9.3 to the master. Thank you!

[NullVoxPopuli]

what did you do? (for those who may come by this issue in the future with the same problem?)

[Mohakjuneja]

I added the index: true part to the end of my controller method

respond_with semesters.where(:semester_num.lte => current_user.current_semester).desc(:semester_num), each_serializer: Admin::SemesterSerializer, index: true

And in the serializer, I accessed it using the instance_options[:index] method

[bf4]

@Mohakjuneja

AMS has consumed a lot of my time upgrading from 0.9.3 to the master. Thank you!

You would win many friends and much karma if you could help us write an updating guide! ref: #1005

[Mohakjuneja]

@bf4 I'm starting up my own venture (wish me luck!) but I'll be glad to help you with it! This community has been very supportive :)

[jDeppen]

I was able to determine the view by adding this to my controller
serialization_scope :view_context
and this to my serializer

[:name, :birthday, :address, :phone].each { |attr| attribute attr, if: :show? }
def show?
  view_context.action_name == 'show'
end

[Mohakjuneja]

It stopped working all of a sudden.
throws error at the if: symbol now on this line

SyntaxError (/home/mojo/Documents/Rails/hopin/app/serializers/api/v1/content_serializer.rb:5: syntax error, unexpected ':'

attribute :total, :subject_name if: :view_uploads?
                                     ^):
  app/serializers/api/v1/content_serializer.rb:5: syntax error, unexpected ':'

What just happened?

[NullVoxPopuli]

You need a comma before your :if :-)

[Mohakjuneja]

I found the error. But I don't know how to correct it.
It works if I only include one attribute
attribute :total, if: :view_uploads?

It's not working for this syntax anymore
attribute :total, :subject_name ,if: :view_uploads?
Throws this error
ArgumentError (wrong number of arguments (3 for 1..2)):

Does that mean I need to write another line for :subject_name?

[NullVoxPopuli]

attributes can be used for multiple attributes. :-)

[Mohakjuneja]

attributes :total, :subject_name, if: :view_uploads?
TypeError ({:if=>:view_uploads?} is not a symbol nor a string)

I'm sorry about taking so much of your time but I've tried everything

[NullVoxPopuli]

so maybe you do need to define each attribute on it's own line with it's own if condition, so:

attribute :total, if: :view_uploads?
attribute :subject_name, if: :view_uploads?

[Mohakjuneja]

I'll do that for now but I don't find that a very effective solution. We may need to take a look at it. Also I don't know why it worked two days ago but doesn't work anymore. Thank you @NullVoxPopuli :)

[bf4]

@Mohakjuneja that's what version control is for :) Let me know if I'm summarizing this correctly (ignoring syntax errors), then please open a new issue if we need further discussion.

works

attribute :total, if: :view_uploads?
attribute :subject_name, if: :view_uploads?

desired, doesn't work

attributes :total, :subject_name, if: :view_uploads? 

[Mohakjuneja]

@bf4 I've been thinking about the current syntax and I'm wondering if we should stick to it.
In cases where you want a certain attribute to be present in multiple cases but not all, this makes more sense.
for instance

attribute :total, if: [:view_uploads?, edit_content]
attribute :subject_name, if: :view_uploads?

What is your opinion about that?

[beauby]

I'd much rather allow lambdas as if/unless values than introduce a cumbersome "array of method name symbols" syntax (which wouldn't make it obvious whether it is a conjunction or a disjunction).

[majedbojan]

the attributes doesn't work with scope if your shown multiple attributes the correct way put it as arry

%w[attribute1 attribute2 attribute3].each { |att| attribute att, if: :condition? }