Support for virtual fields (regression)

pablobm commented 4 years ago

When https://github.com/thoughtbot/administrate/pull/920 was merged, it broke apps that depended on the old behaviour.

These are two reports on this issue:

Report by @wkirby in https://github.com/thoughtbot/administrate/pull/920#issuecomment-599722598

This PR appears to break any support for virtual fields by enforcing the the resource must define a method to respond to all keys in the Dashboard class. Maybe this is the desired behavior, but the following use case was possible in 0.12.0, and was very useful:

class ReportDashboard < Administrate::BaseDashboard
  ATTRIBUTE_TYPES = {
    id: Field::Number,
    filename: Field::String,
    mimetype: Field::String,
    s3_key: Field::String,
    download: S3DownloadField.with_options({ download_path: :download_lab_report_path }),
    page_count: Field::Number,
    created_at: Field::DateTime,
    updated_at: Field::DateTime
  }.freeze

  COLLECTION_ATTRIBUTES = [
    :id,
    :filename,
    :download,
    :mimetype,
    :created_at
  ].freeze

  SHOW_PAGE_ATTRIBUTES = [
    :id,
    :download,
    :filename,
    :mimetype,
    :s3_key,
    :page_count,
    :created_at,
    :updated_at
  ].freeze

  FORM_ATTRIBUTES = [
    :lab,
    :filename,
    :mimetype,
    :s3_key,
    :page_count
  ].freeze
end

class S3DownloadField < Administrate::Field::Base
  def to_s
    data
  end

  def downloadable?
    options.key?(:download_path)
  end

  def destroyable?
    options.key?(:destroy_path)
  end

  def destroy_path(field, attachment)
    path_helper(:destroy_path, field, attachment)
  end

  def download_path(field, attachment)
    path_helper(:download_path, field, attachment)
  end

  private

  def path_helper(key, field, _attachment)
    path_helper = options.fetch(key)
    Rails.application.routes.url_helpers.send(path_helper, field.resource)
  end
end

This allowed us to inject a download link for our S3 resource without editing the underlying views, or modifying the display for any of the actual attributes.

Is there any advice on how to fix this without defining useless methods on our models?

Report by @Timmitry in https://github.com/thoughtbot/administrate/issues/1570#issuecomment-600472047

We are having similar problems - updating to 0.13 breaks all of our virtual fields (and we have many of those). Since 0.13 fixes a CVE, this is kind of an urgent problem 😕

We fixed the issue temporarily by adding an initializer to config/initializers with the following code:

# Monkey patch to make the … page work again
# The breaking change was https://github.com/thoughtbot/administrate/commit/dc856a917aa67e998860bb42664b5da94eb0e682#diff-a4a632998186059ef606368d710ac173
# Issue is open at https://github.com/thoughtbot/administrate/issues/1570
raise "Try to remove this monkey patch when updating Administrate" if Gem.loaded_specs["administrate"].version != Gem::Version.new("0.13.0")

module Administrate
  module Page
    class Base
      protected

      def get_attribute_value(resource, attribute_name)
        resource.public_send(attribute_name)
      rescue NameError
        nil
      end
    end
  end
end

Notes

To clarify, this is about creating fields for properties that don't exist at all: not in ActiveRecord, not as methods in the resource.

sedubois commented 4 years ago

A bit related to this, Jumpstart Rails runs a fork of Administrate with this extra commit: https://github.com/excid3/administrate/commit/ac3b8b81ab55d4799865c80924ca8ca0b357aac0

Maybe such a patch would enable the described scenario?

pablobm commented 4 years ago

The thing with that patch is that it is for a generator, rather than for code that runs day-to-day. Do they have anything else?

sedubois commented 4 years ago

Not that I’m aware of.

pablobm commented 4 years ago

Then I'm not sure that the linked commit fixes the problem. Instead, it appears to add detection of virtual attributes added with attribute. Also, this is not the only way to have virtual attributes; it can be done with just instance methods.

wvengen commented 2 years ago

It looks like getting the value from the resource in Administrate::Base::Page#attribute_field is a culprit: https://github.com/thoughtbot/administrate/blob/844c27091bab1a384b260a47b0a67dbff856640d/lib/administrate/page/base.rb#L28-L36

I can think of some solutions (but I guess there would be more):

Let the field get the value from the resource. This would allow custom fields to use their own method to get the value, and only call it when necessary. This may break the field api, though (but not for inputs using the input base class constructor - which I think would be most). (I think this could fix download in the example above.)
Add a virtual option to all fields, which expects a boolean argument. If it is true, the value would just be nil.
Add a general value (or getter) option to all fields, which expects a proc/... to get the value (defaulting to line 29 above). And perhaps when this option is nil, don't prefill a value at all (that could fix download in the example above)

I guess the last would be the most flexible - but it may lead to other wishes (perhaps a custom value-setter option).

I'm actually having a slightly different use-case for virtual fields: combining several model attributes in a single element using a custom field type (for me that's year and week of a publication that is always entered together, and it has a better user- experience to treat it as a single element).

pablobm commented 2 years ago

Thanks to all who helped explain this issue!

@wkirby, @Timmitry - For the moment, a workaround might be to add a method of this name (in your example download) to the model. For example, just this empty method fixes the issue when I reproduce it in my setup:

def download
end

Can people confirm is this is enough for them while we get to a better solution (which admittedly will still take time)?

pablobm commented 2 years ago

@wvengen - Thank you for looking into this :-) Initially, I'm more inclined towards the first option. I have seen other use cases where having the field decide its value by looking at the whole resource would be useful, and this is exactly a case of that.

You are right that it would break the Fields API, so I would want this to happen in an ordered fashion that allowed for initial back-compatibility while plugins and users adapt, first showing warnings for a few versions, then dropping it altogether.

Only since recently, the whole resource is given to the field, so that's a start. What I'm not sure of is how we would phase out the second argument (data) and raise warnings, etc, so that people are made aware of the change without breaking their apps at first.

Any ideas? I'd welcome a PR if you have the time. A tentative PR to explore the solution would be great, so that there's no pressure to get it right first time, and we can evaluate solutions before committing to one.

wvengen commented 2 years ago

Thanks for your response, @pablobm. We're still evaluating the use of Administrate (coming from Brightcontent). If we go for it, we'll probably have a go at a PR (but it'll take some time).

pablobm commented 5 months ago

I had a quick look at this today in the context of something else. Same as I mentioned back in https://github.com/thoughtbot/administrate/issues/1586#issuecomment-996017463, I'm favouring an option where fields would look at the resource and produce a value, instead of having Administrate produce the value after making too many assumptions. This would allow fields having their own getters and setters, which would give them extra flexibility.

The following is a diff of where I got to. If someone wants to use it as a basis for a PR, you are welcome to do so! Otherwise, it'll help me remember what this is about when I return to this in the future.

diff --git a/lib/administrate/field/base.rb b/lib/administrate/field/base.rb
index ae7a599..f005443 100644
--- a/lib/administrate/field/base.rb
+++ b/lib/administrate/field/base.rb
@@ -32,6 +32,10 @@ module Administrate
         attr
       end

+      def self.read_value(resource, attribute_name)
+        resource.public_send(attribute_name)
+      end
+
       def initialize(attribute, data, page, options = {})
         @attribute = attribute
         @data = data
diff --git a/lib/administrate/field/deferred.rb b/lib/administrate/field/deferred.rb
index 1060973..40b8bce 100644
--- a/lib/administrate/field/deferred.rb
+++ b/lib/administrate/field/deferred.rb
@@ -10,6 +10,10 @@ module Administrate

       attr_reader :deferred_class, :options

+      def read_value(resource, attribute_name)
+        @deferred_class.read_value(resource, attribute_name)
+      end
+
       def new(*args)
         new_options = args.last.respond_to?(:merge) ? args.pop : {}
         deferred_class.new(*args, options.merge(new_options))
diff --git a/lib/administrate/field/url.rb b/lib/administrate/field/url.rb
index b5f1dad..22d2ed9 100644
--- a/lib/administrate/field/url.rb
+++ b/lib/administrate/field/url.rb
@@ -7,6 +7,17 @@ module Administrate
         true
       end

+      def self.read_value(object, attribute)
+        if attribute == :download_link
+          # Ideally this would be a different field type, but just for
+          # a quick prototype I'm using the Url field and using
+          # this little dirty condition to display a different value.
+          "This is a download link"
+        else
+          super
+        end
+      end
+
       def truncate
         data.to_s[0...truncation_length]
       end
diff --git a/lib/administrate/page/base.rb b/lib/administrate/page/base.rb
index 266deb5..7f6648e 100644
--- a/lib/administrate/page/base.rb
+++ b/lib/administrate/page/base.rb
@@ -32,13 +32,9 @@ module Administrate
       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_type = dashboard.attribute_type_for(attribute_name)
+        value = field_type.read_value(resource, attribute_name)
+        field_type.new(attribute_name, value, page, resource:)
       end

       attr_reader :dashboard, :options
diff --git a/spec/example_app/app/dashboards/customer_dashboard.rb b/spec/example_app/app/dashboards/customer_dashboard.rb
index 79786fa..f855706 100644
--- a/spec/example_app/app/dashboards/customer_dashboard.rb
+++ b/spec/example_app/app/dashboards/customer_dashboard.rb
@@ -17,7 +17,8 @@ class CustomerDashboard < Administrate::BaseDashboard
       searchable_fields: ["name"],
       include_blank: true
     ),
-    password: Field::Password
+    password: Field::Password,
+    download_link: Field::Url,
   }

   COLLECTION_ATTRIBUTES = ATTRIBUTE_TYPES.keys - %i[created_at updated_at]

goosys commented 5 months ago

When displaying the list screen, a sorting link is always added, so I think we need to take that into consideration as well. https://github.com/thoughtbot/administrate/blob/main/app/views/administrate/application/_collection.html.erb#L30

Additionally, I feel we need to redesign this partial to account for cases like the one mentioned https://github.com/thoughtbot/administrate/issues/2468 .

thoughtbot / administrate