Skip to content

Add conditional fields support#86

Merged
philipqnguyen merged 1 commit intoprocore-oss:masterfrom
ojab:conditional_fields
Jun 5, 2018
Merged

Add conditional fields support#86
philipqnguyen merged 1 commit intoprocore-oss:masterfrom
ojab:conditional_fields

Conversation

@ojab
Copy link
Copy Markdown
Contributor

@ojab ojab commented Jun 1, 2018

See #81
One omission: methods should be declared before field declaration using them, i. e.

class UserBlueprinter < Blueprinter::Base
  # field :first_name, if: :my_method here will fail
  def my_method(user, options)
    # some code
  end

  field :first_name, if: :my_method
end

because methods are not available yet at field declaration time and I'm resolving Symbol to Method there. Is it worth fixing or I should just add a note to README?

Other one: AFAICS collection serialization is not tested at the moment, so I also skipped it.

@ojab ojab force-pushed the conditional_fields branch from 7ab0331 to 4f7a6fb Compare June 1, 2018 15:20
Copy link
Copy Markdown
Contributor

@philipqnguyen philipqnguyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making this PR. This will be a great addition. What do you think of the suggestions?

Comment thread lib/blueprinter/base.rb Outdated
if options[condition].is_a?(Symbol)
options[condition] = self.method(options.fetch(condition))
end
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be a way to resolve the conditional method at render execution time rather than at field declaration time, allowing conditional methods to be defined anywhere in the blueprint. Here's the idea, let me know what you think =)

What if you move this chunk of code into the skip? method you made in Field?

Comment thread lib/blueprinter/base.rb

def self.object_to_hash(object, view_name:, local_options:)
view_collection.fields_for(view_name).each_with_object({}) do |field, hash|
next if field.skip?(object, local_options)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need to pass self here. field.skip?(object, local_options, self)

Comment thread lib/blueprinter/field.rb
extractor.extract(method, object, local_options, options)
end

def skip?(object, local_options)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argument list would be altered to look like: skip?(object, local_options, blueprint)

Then you can place your above code here in this method:

    %i[if unless].each do |condition|
      if options[condition].is_a?(Symbol)
        options[condition] = blueprint.method(options.fetch(condition))
      end
    end

@philipqnguyen
Copy link
Copy Markdown
Contributor

philipqnguyen commented Jun 1, 2018

And as for collection testing... yes we need to get that in there. It would be in a separate PR though, so I wouldn't worry about it here.

@ojab ojab force-pushed the conditional_fields branch 4 times, most recently from 8567f44 to d18fd63 Compare June 3, 2018 12:48
@ojab
Copy link
Copy Markdown
Contributor Author

ojab commented Jun 3, 2018

Passing blueprint to Field at initialization time looks more clear to me.
Pushed that version, what do you think?

Copy link
Copy Markdown
Contributor

@philipqnguyen philipqnguyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing self during field initialization makes sense! Really like this approach.

I have a few requests regarding the organization of the specs.

end

variants.each do |other_type, other_value|
context "and :unless is #{other_type} returning #{other_value}" do
Copy link
Copy Markdown
Contributor

@philipqnguyen philipqnguyen Jun 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current location of this context block makes it look like :unless depends on first having :if passed to field. However, both :if and :unless should be at the same level of context right?

I'm thinking of something like:

    Inside Rails project
      Given blueprint has ::field with a conditional argument
        Given the conditional is :if
           ... other contexts / specs
        Given the conditional is :unless
           ... other contexts / spces
    Outside Rails project
      ... repeat from above ...

Copy link
Copy Markdown
Contributor Author

@ojab ojab Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it actually is: it's specs for both :if & :unless passed. Added specs for :unless only for clarity and fixed examples descriptions, now it looks like:

    Inside Rails project
      Given blueprint has ::field with a conditional argument
        Given the conditional is :if proc returning true
          and no :unless conditional
            serializes the conditional field
          and :unless conditional is proc returning true
            does not serialize the conditional field
          …
        Given the conditional is :unless proc returning true and no :if conditional
          does not serialize the conditional field
        Given the conditional is :if proc returning false
          and no :unless conditional
            does not serialize the conditional field
          and :unless conditional is proc returning true
          …

end

if value && !other_value
it { should eq(result_with_first_name) }
Copy link
Copy Markdown
Contributor

@philipqnguyen philipqnguyen Jun 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current rspec printout out for these it blocks are something like:

and :unless is proc returning false
  should eq "{\"first_name\":\"Meg\",\"id\":15}"

or

and :unless is proc returning true
  should eq "{\"id\":16}"`

When reading the rspec printout, we don't actually care whether first_name is there or not, as that is just the example. What we care is whether the field with the conditional is there or not.

I think a description for the it block should be there to clarify the printout.
it ('does not serialize the field') { should eq(result_without_first_name) }
or
it ('serializes the field') { should eq(result_with_first_name) }

which gives a printout of something like:

and :unless is proc returning true
  serialize the field

or

and :unless is proc returning false
  does not serialize the field

This comment applies to the other it blocks in this PR as well.

@ojab ojab force-pushed the conditional_fields branch 2 times, most recently from 7f1a97a to 1c14046 Compare June 5, 2018 07:27
@ojab ojab force-pushed the conditional_fields branch from 1c14046 to da8a69f Compare June 5, 2018 07:33
@philipqnguyen philipqnguyen merged commit 068484c into procore-oss:master Jun 5, 2018
@philipqnguyen
Copy link
Copy Markdown
Contributor

released in 0.6.0

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.

3 participants