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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

goosys
Copy link

@goosys goosys commented Sep 10, 2024

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.

image

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
Copy link
Collaborator

pablobm commented Sep 13, 2024

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

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 #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 #920 was merged, resulting in issue #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
Copy link
Author

goosys commented Sep 13, 2024

@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
Copy link
Author

goosys commented Sep 13, 2024

@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

image

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
Copy link
Author

goosys commented Sep 14, 2024

@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.

a83e2ff

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.

2 participants