Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for virtual fields #2658

Merged
merged 14 commits into from
Oct 25, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion lib/administrate/field/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@ def self.permitted_attribute(attr, _options = nil)

def initialize(attribute, data, page, options = {})
@attribute = attribute
@data = data
@page = page
@resource = options.delete(:resource)
@options = options
@data = read_value(data)
end

def html_class
Expand All @@ -52,6 +52,20 @@ def name
attribute.to_s
end

def read_value(data = nil)
if options.key?(:getter)
if options[:getter].respond_to?(:call)
options[:getter].call(self)
else
resource.try(options[:getter])
end
elsif data.nil?
resource.try(attribute)
else
data
end
end

def to_partial_path
"/fields/#{self.class.field_type}/#{page}"
end
Expand Down
8 changes: 8 additions & 0 deletions lib/administrate/field/deferred.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ def ==(other)
options == other.options
end

def getter
options.fetch(:getter, nil)
end

def associative?
deferred_class.associative?
end
Expand Down Expand Up @@ -57,6 +61,10 @@ def permitted_attribute(attr, opts = {})
end
end

def read_value(resource, attribute_name)
@deferred_class.read_value(resource, attribute_name, options)
end

delegate :html_class, to: :deferred_class
end
end
Expand Down
7 changes: 1 addition & 6 deletions lib/administrate/page/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,8 @@ def item_associations
private

def attribute_field(dashboard, resource, attribute_name, page)
value = get_attribute_value(resource, attribute_name)
field = dashboard.attribute_type_for(attribute_name)
field.new(attribute_name, value, page, resource: resource)
end

def get_attribute_value(resource, attribute_name)
resource.public_send(attribute_name)
field.new(attribute_name, nil, page, resource: resource)
end

attr_reader :dashboard, :options
Expand Down
4 changes: 4 additions & 0 deletions spec/example_app/app/dashboards/customer_dashboard.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ class CustomerDashboard < Administrate::BaseDashboard
email_subscriber: Field::Boolean,
lifetime_value: Field::Number.with_options(prefix: "$", decimals: 2),
name: Field::String,
nickname: Field::String.with_options(
getter: ->(field) { "Mr. #{field.resource.name}man" if field.resource.name.present? },
searchable: false
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking of an example that doesn't feel too artificial: how about a full_address field for Order? Something like this:

    full_address: Field::String.with_options(
      getter: ->(field) {
        r = field.resource
        [
          r.address_line_one,
          r.address_line_two,
          r.address_city,
          r.address_state,
          r.address_zip
        ].compact.join("\n")
      },
      searchable: false
    ),

Then its SHOW_PAGE_ATTRIBUTES can be something like this:

  SHOW_PAGE_ATTRIBUTES = FORM_ATTRIBUTES
    .except("address")
    .merge(
      "" => %i[customer full_address created_at updated_at],
      "details" => %i[line_items total_price shipped_at payments]
    )
    .freeze

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like searchable: false is excess. If getter has been provided it makes field as unsearchable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you thought about using decorators(like draper) for virtual fields?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, should have been more clear: remove this example (which I find too artificial) and we can leave the one for the orders, which I think is better 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point on searchable. Perhaps it should be false by default if getter is supplied? Otherwise it breaks the search.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Nitr - It's a bit tricky due to how Administrate works internally. Also we tend to avoid adding dependencies like draper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Nitr @pablobm
Thank you for the advice. I have made the changes. cab6096
I'm not sure if we should always set searchable to false when a getter is present, but since I can't think of any use cases at the moment, this seems appropriate.

orders: Field::HasMany.with_options(limit: 2, sort_by: :id),
log_entries: Field::HasManyVariant.with_options(limit: 2, sort_by: :id),
updated_at: Field::DateTime,
Expand Down
26 changes: 21 additions & 5 deletions spec/example_app/app/dashboards/order_dashboard.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,19 @@ class OrderDashboard < Administrate::BaseDashboard
address_city: Field::String,
address_state: Field::String,
address_zip: Field::String,
full_address: Field::String.with_options(
getter: ->(field) {
r = field.resource
[
r.address_line_one,
r.address_line_two,
r.address_city,
r.address_state,
r.address_zip
].compact.join("\n")
},
searchable: false
),
customer: Field::BelongsTo.with_options(order: "name"),
line_items: Field::HasMany.with_options(
collection_attributes: %i[product quantity unit_price total_price]
Expand All @@ -29,7 +42,7 @@ class OrderDashboard < Administrate::BaseDashboard
COLLECTION_ATTRIBUTES = [
:id,
:customer,
:address_state,
:full_address,
:total_price,
:line_items,
:shipped_at
Expand All @@ -50,8 +63,11 @@ class OrderDashboard < Administrate::BaseDashboard
address_zip
]
}.freeze
SHOW_PAGE_ATTRIBUTES = FORM_ATTRIBUTES.merge(
"" => %i[customer created_at updated_at],
"details" => %i[line_items total_price shipped_at payments]
).freeze
SHOW_PAGE_ATTRIBUTES = FORM_ATTRIBUTES
.except("address")
.merge(
"" => %i[customer full_address created_at updated_at],
"details" => %i[line_items total_price shipped_at payments]
)
.freeze
end
30 changes: 30 additions & 0 deletions spec/lib/fields/base_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -167,4 +167,34 @@
expect(field.required?).to eq(false)
end
end

describe "read_value" do
it "reads the value from the resource" do
resource = double(attribute: "value")
field = field_class.new(:attribute, :date, :page, resource: resource)

expect(field.read_value).to eq("value")
end

it "reads the value from the resource with a custom getter" do
resource = double(custom_getter: "value")
field = field_class.new(:attribute, :date, :page, resource: resource, getter: :custom_getter)

expect(field.read_value).to eq("value")
end

it "reads the value from the resource with a custom getter block" do
resource = double("Model", custom_getter: "value")
field = field_class.new(:attribute, :date, :page, resource: resource, getter: ->(field) { field.resource.custom_getter })

expect(resource).to receive(:custom_getter)
expect(field.read_value).to eq("value")
end

it "returns nil if the resource is nil" do
field = field_class.new(:attribute, :date, :page, resource: nil)

expect(field.read_value).to eq(nil)
end
end
goosys marked this conversation as resolved.
Show resolved Hide resolved
end
37 changes: 37 additions & 0 deletions spec/lib/fields/virtual_field_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
require "rails_helper"
require "administrate/field/base"

module Administrate
module Field
class VirtualField < Field::Base
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we can find an example in the dashboard where we can define this type of virtual field without having to drop it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What else is needed here? Documentation, maybe?
Or would an approach using an option like virtual_field: true to explicitly indicate virtual fields work better?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Answering on the main thread 🙂

def self.searchable?
false
end

def read_value(data = nil)
resource.inspect
end

def foobar
"This is a Foobar: #{data}"
end
end
end
end

describe Administrate::Field::VirtualField do
describe "#foobar" do
it "displays the foobar" do
resource = double("Model", inspect: "Inspecting")

field = Administrate::Field::VirtualField.new(
:virtual_field,
nil,
:show,
resource: resource
)

expect(field.foobar).to eq("This is a Foobar: Inspecting")
end
end
end
Loading