Support for virtual fields

[goosys]

Support for virtual fields, based on #1586.

Usage

• It is possible to use a name as attribute_name that doesn't exist in the DB column or as a method in the model.

class CustomerDashboard < Administrate::BaseDashboard
  ATTRIBUTE_TYPES = {
    name: Field::String, # real field
    nickname: Field::String.with_options( # virtual field
      # You can specify a symbol if Customer#generate_nickname is defined
      getter: :generate_nickname,
      # Or, You can directly specify a function
      getter: ->(field) { "Mr. #{field.resource.name}man" if field.resource.name.present? },
      # Virtual fields cannot be searchable, so 'false' must be specified
      searchable: false 
    ),

---

(Note: The Sortable implementation has been split into #2659.)

---

What do you think? I'll make any necessary changes. Please review it.

[pablobm]

Thank you for this. I hope I can remember things clearly, let's see what we can do here... :eyes:

sortable option

First, you are also introducing the sortable option, which is a separate concern. That would be best served in a separate PR.

By all means, it's really good that you are implementing sortable (it's been requested before, see https://github.com/thoughtbot/administrate/issues/1314), but separate, smaller PRs are easier to review and reason about. I'll be happy to review that.

The virtual field

The solution you propose is different from the one that was lost when https://github.com/thoughtbot/administrate/pull/920 was merged, resulting in issue https://github.com/thoughtbot/administrate/issues/1586.

You propose an option :getter to define how a field will be accessed. In contrast, the lost functionality allowed the creation of fields that didn't need special options or interfaces.

So for example, it was possible to have this:

## field/foobar.rb
require_relative "base"

module Administrate
  module Field
    class Foobar < Field::Base
      def self.searchable?
        false
      end

      def foobar
        "This is a Foobar: #{resource.inspect}"
      end
    end
  end
end

## views/fields/foobar/_collection.html.erb
<%= field.foobar %>

In my view, the broken functionality was simpler to use and I would prefer that to return.

Also, I see that your code would only work when when @data.nil?. This could lead to unexpected behaviour, as it's perfectly normal for a non-virtual field to be nil instead of having a value. In my mind, it goes "if it's nil, perhaps it's a virtual field".

In comparison, the original functionality established distinction between virtual and non-virtual fields. In my mind, it goes "it is virtual or it is not, but there's no confusion".

I hope that makes sense.

What do you think? Would you be able to separate the sortable code into a different PR, and change this one to behave the way I describe?

[goosys]

@pablobm Apologies for the confusion. With the implementation of virtual fields, the issue with the sort buttons on the index screen became more noticeable, so I initially included both in the same PR. I've now split the PRs. I'll address the points you mentioned later, and I'd appreciate your review again at that time.

[goosys]

@pablobm

Also, I see that your code would only work when when @data.nil?. This could lead to unexpected behaviour, as it's perfectly normal for a non-virtual field to be nil instead of having a value. In my mind, it goes "if it's nil, perhaps it's a virtual field".

This is for compatibility reasons. We can fetch data exclusively from within the field using read_value. While the application will work perfectly in this state, many RSpec tests fail because they insert data using the data argument.

module Administrate
  module Field
    class Base
      def initialize(attribute, _data, page, options = {})
        @attribute = attribute
        @page = page
        @resource = options.delete(:resource)
        @options = options
        @data = read_value
      end

Should I fix all the RSpec tests? In that case, the data argument in initialize method would become unnecessary, so it would be better to either remove it or make it optional. However, this might cause compatibility issues with third-party Fields.

[goosys]

@pablobm

You propose an option :getter to define how a field will be accessed. In contrast, the lost functionality allowed the creation of fields that didn't need special options or interfaces. In my view, the broken functionality was simpler to use and I would prefer that to return.

I believe this case should not be an issue. For custom fields, you can override read_value to fetch data and format it using any method you prefer. I've written a sample and committed it, so please check it.

https://github.com/thoughtbot/administrate/pull/2658/commits/a83e2ff6ae76b7f289b393cbff6a6d03cac34a0c

[goosys]

Next, I would like to add documentation. However, it seems like this feature has become more complex and capable than I initially expected, so I plan to organize it a bit before documenting it. If possible, I would appreciate your continued assistance.

[pablobm]

@goosys - Sounds good :slightly_smiling_face:

[goosys]

@pablobm I've created documentation for all the use cases I can think of at the moment. Do you think there are any other use cases we should include? https://github.com/thoughtbot/administrate/pull/2658/commits/d1fd3a9b08eebe9c78f355a5d40c5935648ba41c

Also, if there are any mistakes in the English within the documentation, I’d appreciate it if you could correct them. Thank you!

[Nitr]

@goosys For all field types - I guess it won't work properly for associative types. It will try to includes association by key to avoid N+1 and it will fail.

[goosys]

@Nitr Thanks for the valuable feedback! I checked, and it seems to be working for now. Although multiple options besides the :getter need to be specified, nothing seems to be broken. (Also, it appears that the deprecated :class_name is still required.) Regarding the N+1, it can be avoided with the existing :includes option.

class CustomerDashboard < Administrate::BaseDashboard
  ATTRIBUTE_TYPES = {
    orders: Field::HasMany.with_options(limit: 2, sort_by: :id),
    recent_orders: Field::HasMany.with_options(
      getter: ->(field){ field.resource.orders.where(created_at: [3.days.ago...]).order(created_at: :desc).limit(2) },
      class_name: "Order",
      foreign_key: :customer_id,
      includes: :orders
    ),

It’s quite strange at this point, but I think we can improve it going forward. What do you think?

[pablobm]

I think this is it? (Apart from the two small suggestions now). Do you think there's anything else to cover or should we go and merge?

Tangentially: I need to un-deprecate the :class_name option as we have come across legitimate use cases.

[goosys]

@pablobm

I think this is it? (Apart from the two small suggestions now). Do you think there's anything else to cover or should we go and merge?

I believe there are still plenty of ways we could use this, depending on new ideas. But for now, I think it’s fine to go ahead and merge it.

[pablobm]

One last thing. Sorry! :sweat:

1. I saw a couple of read_value that I don't think are used anywhere and removed them. Also removed the default value data = nil from the normal read_value.
2. I added some nicety to the receipt downloader.

Changes are at https://github.com/thoughtbot/administrate/compare/main...pablobm:administrate:goosys-issue-1586-virtual-fields. Does this make sense? If so, could you please merge my two commits into your branch?

[goosys]

@pablobm Thank you for checking.

It seems the Deferred#read_value isn’t necessary. I wasn’t entirely sure, so I added it just in case. Sorry about that. Also, thanks for the other additions! I’ve merged the changes.

One thing I wanted to mention—while pushing to this PR several times, I noticed that the payment_index_spec was unstable, failing and passing inconsistently only with Ruby 3.0. I addressed it since it was happening quite often. Could it be that updating current_path is sometimes delayed? It’s strange because no other tests had this issue, just this one file. After modifying it to use have_current_path, it stopped failing locally.

[pablobm]

Re: current_path, no idea :sweat_smile: I wasn't seeing the problem so I was not aware. But if your change appears to fix things, then that's good :+1:

Merging at last! :rocket: