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

The class_name option is still necessary for namespaced associated classes in Field::HasMany #1978

Closed
andrewdsmith opened this issue May 7, 2021 · 12 comments · Fixed by #2235
Labels
bug breakages in functionality that is implemented

Comments

@andrewdsmith
Copy link

What were you trying to do?

Silence the class_name option deprecation warnings due to #1633.

What did you end up with (logs, or, even better, example apps are great!)?

I was able to remove all uses of the class_name option except the following:

module ActiveStorage
  class BlobDashboard < Administrate::BaseDashboard
    ATTRIBUTE_TYPES = {
      attachments: Field::HasMany.with_options(class_name: 'ActiveStorage::Attachment'),
      # snip
    }.freeze
    # snip
  end
end

Notably, I also have a dashboard for ActiveStorage::Attachment that has a Field::HasOne back to the blob that doesn't require the class_name option.

What versions are you running?

Rails: 6.0.3.7
administrate: 0.16

@andrewdsmith andrewdsmith added the bug breakages in functionality that is implemented label May 7, 2021
@pablobm
Copy link
Collaborator

pablobm commented May 13, 2021

Thank you for reporting. I have very little experience with ActiveStorage so I don't have any immediate solutions. I'll have to experiment.

Can you be more specific as to what error you get? For example, can you provide an exception backtrace?

For the moment, a way to silence this warning would be to override Administrate.warn_of_deprecated_option.

@andrewdsmith
Copy link
Author

Here's the exception backtrace from one of my tests that fails if I remove the class_name on the association:

  2) /admin/blobs GET /1 when the user is signed in when the user is an authorised administrator grants access
     Failure/Error: let(:make_request) { get polymorphic_path([:admin, resource]) }
      
     ActionView::Template::Error:
       uninitialized constant AttachmentDashboard
     Shared Example Group: "access granted" called from ./spec/requests/admin_spec.rb:26
     Shared Example Group: "authentication and authorization" called from ./spec/requests/admin_spec.rb:61
     # /var/lib/gems/2.5.0/gems/administrate-0.16.0/lib/administrate/field/associative.rb:37:in `associated_dashboard'
     # /var/lib/gems/2.5.0/gems/administrate-0.16.0/lib/administrate/field/has_many.rb:82:in `includes'
     # /var/lib/gems/2.5.0/gems/administrate-0.16.0/lib/administrate/field/has_many.rb:57:in `resources'
     # /var/lib/gems/2.5.0/gems/administrate-0.16.0/app/views/fields/has_many/_show.html.erb:21:in `__var_lib_gems_______gems_administrate________app_views_fields_has_many__show_html_erb__3547983027385624575_47346348155040'
     # /var/lib/gems/2.5.0/gems/administrate-0.16.0/app/helpers/administrate/application_helper.rb:16:in `render_field' 
     # /var/lib/gems/2.5.0/gems/administrate-0.16.0/app/views/administrate/application/show.html.erb:46:in `block in __var_lib_gems_______gems_administrate________app_views_administrate_application_show_html_erb__4380180792876114351_47346
348022480'
     # /var/lib/gems/2.5.0/gems/administrate-0.16.0/app/views/administrate/application/show.html.erb:37:in `each'
     # /var/lib/gems/2.5.0/gems/administrate-0.16.0/app/views/administrate/application/show.html.erb:37:in `__var_lib_gems_______gems_administrate________app_views_administrate_application_show_html_erb__4380180792876114351_47346348022480
'
     # /var/lib/gems/2.5.0/gems/administrate-0.16.0/app/controllers/administrate/application_controller.rb:25:in `show' 
     # /var/lib/gems/2.5.0/gems/warden-1.2.8/lib/warden/manager.rb:36:in `block in call'
     # /var/lib/gems/2.5.0/gems/warden-1.2.8/lib/warden/manager.rb:34:in `catch'
     # /var/lib/gems/2.5.0/gems/warden-1.2.8/lib/warden/manager.rb:34:in `call'
     # /var/lib/gems/2.5.0/gems/rack-2.2.3/lib/rack/tempfile_reaper.rb:15:in `call'
     # /var/lib/gems/2.5.0/gems/rack-2.2.3/lib/rack/etag.rb:27:in `call'
     # /var/lib/gems/2.5.0/gems/rack-2.2.3/lib/rack/conditional_get.rb:27:in `call'
     # /var/lib/gems/2.5.0/gems/rack-2.2.3/lib/rack/head.rb:12:in `call'
     # /var/lib/gems/2.5.0/gems/rack-2.2.3/lib/rack/session/abstract/id.rb:266:in `context'
     # /var/lib/gems/2.5.0/gems/rack-2.2.3/lib/rack/session/abstract/id.rb:260:in `call'
     # /var/lib/gems/2.5.0/gems/railties-6.0.3.7/lib/rails/rack/logger.rb:37:in `call_app'
     # /var/lib/gems/2.5.0/gems/railties-6.0.3.7/lib/rails/rack/logger.rb:26:in `block in call'
     # /var/lib/gems/2.5.0/gems/railties-6.0.3.7/lib/rails/rack/logger.rb:26:in `call'
     # /var/lib/gems/2.5.0/gems/rack-2.2.3/lib/rack/method_override.rb:24:in `call'
     # /var/lib/gems/2.5.0/gems/rack-2.2.3/lib/rack/runtime.rb:22:in `call'
     # /var/lib/gems/2.5.0/gems/rack-2.2.3/lib/rack/sendfile.rb:110:in `call'
     # /var/lib/gems/2.5.0/gems/railties-6.0.3.7/lib/rails/engine.rb:527:in `call'
     # /var/lib/gems/2.5.0/gems/rack-test-1.1.0/lib/rack/mock_session.rb:29:in `request'
     # /var/lib/gems/2.5.0/gems/rack-test-1.1.0/lib/rack/test.rb:266:in `process_request'
     # /var/lib/gems/2.5.0/gems/rack-test-1.1.0/lib/rack/test.rb:119:in `request'
     # /var/lib/gems/2.5.0/gems/rails-controller-testing-1.0.5/lib/rails/controller/testing/integration.rb:16:in `block (2 levels) in <module:Integration>'
     # ./spec/requests/admin_spec.rb:60:in `block (4 levels) in <top (required)>'
     # ./spec/requests/admin_spec.rb:39:in `block (4 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # NameError:
     #   uninitialized constant AttachmentDashboard
     #   /var/lib/gems/2.5.0/gems/administrate-0.16.0/lib/administrate/field/associative.rb:37:in `associated_dashboard'

So it looks like that it's not handling the namespace because it should be ActiveStorage::AttachmentDashboard not just AttachmentDashboard.

@michaelwebb76
Copy link

Please please please do not deprecate this, it will break everything in our systems. We have multiple level namespaces like Raw::DataSource::ModelName that we're already having to monkey patch this gem to use, I would love to not have to monkey patch it further.

@jordanstephens
Copy link

I also encountered this issue with removing the class_name option for associations on namespaced classes.

In my case, the only places where Administrate failed to automatically determine the class_name were the cases where we had failed to specify the class_name option on the ActiveRecord association itself.

It's actually not clear to me how ActiveRecord was able to function without specifying the class_name option on the relations to begin with—since all of them are namespaced—but nonetheless adding the class_name option to the ActiveRecord associations seems to have enabled Administrate to get the information it needs without specifying the class_name on the Administrate field as well. That's great!

Now there is one case (for us) where we do still need to set the class_name option on the Administrate field, and that is where we do an association via a JSONB property. Because we aren't using an ActiveRecord association, Administrate is unable to find any ActiveRecord::Reflection object from which to extract the relation's class name. For us, this manifest as the following exception:

ActionView::Template::Error: undefined method `klass' for nil:NilClass

    [...]administrate-0.16.0/lib/administrate/field/associative.rb:11:in `associated_class'
    [...]administrate-0.16.0/lib/administrate/field/associative.rb:30:in `associated_class'
    [...]

One potential improvement for this situation might be for Administrate to first determine if there is any ActiveRecord::Reflection available, and only then print the deprecation warning.

I'm happy to submit a PR if there's any interest in this approach.

Thanks!

@sedubois
Copy link
Contributor

I encounter the same issue as described in the OP with Administrate 0.16.0. I needed to use attachments: Field::HasMany.with_options(class_name: 'ActiveStorage::Attachment') in ActiveStorage::BlobDashboard, blob: Field::BelongsTo.with_options(class_name: 'ActiveStorage::Blob') in ActiveStorage::VariantRecordDashboard, and then disabled the deprecation warning as follows:

# app/initializers/administrate.rb
module Administrate
  def self.warn_of_deprecated_option(name)
  end
end

NB: The ActiveStorage admin routes were configured as shown here.

@unosk
Copy link

unosk commented Jan 12, 2022

I applied the following patch without using the :class_name option and it worked fine

module Administrate
  module Field
    class Associative < Base
      module Patch
        def associated_class_name
          # If the resource is ActiveStorage::Attachment, 
          # original associated_class_name return 'Attachment', 
          # but associated_class will return ActiveStorage::Attachment.
          associated_class.name
        end
      end

      prepend Patch
    end
  end
end

@aliaksandrb
Copy link

A similar problem happens to me when dashboards are namespaced on v0.16.

Given, we have namespaced dashboards:

app/dashboards/system/package_dashboard.rb
app/dashboards/system/build_dashboard.rb
# package_dashboard.rb

require 'administrate/base_dashboard'

module System
  class PackageDashboard < Administrate::BaseDashboard
    ATTRIBUTE_TYPES = {
      id: Field::Number,
      build: Field::BelongsTo.with_options(
        class_name: 'System::Build'
      )
...

The dashboard above works fine, but crashes without class_name attribute:

 {:error=>"uninitialized constant BuildDashboard", :backtrace=>"/app/path/ruby/2.6.0/gems/activesupport-6.0.3.7/lib/active_support/inflector/methods.rb:282:in `const_get'
/app/path/ruby/2.6.0/gems/activesupport-6.0.3.7/lib/active_support/inflector/methods.rb:282:in `block in constantize'
/app/path/ruby/2.6.0/gems/activesupport-6.0.3.7/lib/active_support/inflector/methods.rb:280:in `each'
/app/path/ruby/2.6.0/gems/activesupport-6.0.3.7/lib/active_support/inflector/methods.rb:280:in `inject'
/app/path/ruby/2.6.0/gems/activesupport-6.0.3.7/lib/active_support/inflector/methods.rb:280:in `constantize'
/app/path/ruby/2.6.0/gems/activesupport-6.0.3.7/lib/active_support/core_ext/string/inflections.rb:68:in `constantize'
/app/path/ruby/2.6.0/gems/administrate-0.16.0/lib/administrate/field/associative.rb:37:in `associated_dashboard'
/app/path/ruby/2.6.0/gems/administrate-0.16.0/lib/administrate/field/associative.rb:23:in `display_associated_resource'
/app/path/ruby/2.6.0/gems/administrate-0.16.0/app/views/fields/belongs_to/_show.html.erb:21:in
_path_ruby_______gems_administrate________app_views_fields_belongs_to__show_html_erb__2234600750770227046_70180239460340'
/app/path/ruby/2.6.0/gems/actionview-6.0.3.7/lib/action_view/base.rb:274
...

Reproducible for both BelongsTo/HasMany field types.

pablobm added a commit to pablobm/administrate that referenced this issue Jun 6, 2022
If the associated class is `System::Build`, the previous code would
tell us that the name was `Build`. This code gets the right name.

Example taken from
thoughtbot#1978 (comment),
which I reproduced locally.
@pablobm
Copy link
Collaborator

pablobm commented Jun 6, 2022

Thank you all for your feedback. I appear to have time to look into this finally. I started by reproducing @aliaksandrb's example, and was able to get a first stab at a fix (pretty much by following @unosk's example).

This initial fix is at main...pablobm:namespaced-associations. Would some of you be able to try it out and see if it helps? Meanwhile I'll try some more complex stuff, like ActiveStorage, etc.

Let's try squash this bug together!

@aliaksandrb
Copy link

aliaksandrb commented Jun 7, 2022

@pablobm thanks for looking into it!

I've applied the patch provided, and it seems to be working fine for both BelongsTo/HasMany associations.
The dashboards that were crashing before are working as expected now 👌

Tested using administrate-0.17.0 + the patch above.

@aliaksandrb
Copy link

FYI: This looks related/might be affected by the change: #2209

pablobm added a commit to pablobm/administrate that referenced this issue Aug 4, 2022
If the associated class is `System::Build`, the previous code would
tell us that the name was `Build`. This code gets the right name.

Example taken from
thoughtbot#1978 (comment),
which I reproduced locally.
pablobm added a commit to pablobm/administrate that referenced this issue Aug 4, 2022
If the associated class is `System::Build`, the previous code would
tell us that the name was `Build`. This code gets the right name.

Example taken from
thoughtbot#1978 (comment),
which I reproduced locally.
@pablobm
Copy link
Collaborator

pablobm commented Aug 4, 2022

@andrewdsmith - I have been able to replicate your exact issue, and for me it's solved with the suggested fix. I have created at PR for it at #2235. Would you be able to check that it works for you too?

pablobm added a commit to pablobm/administrate that referenced this issue Aug 4, 2022
If the associated class is `System::Build`, the previous code would
tell us that the name was `Build`. This code gets the right name.

Example taken from
thoughtbot#1978 (comment),
which I reproduced locally.
pablobm added a commit to pablobm/administrate that referenced this issue Aug 4, 2022
If the associated class is `System::Build`, the previous code would
tell us that the name was `Build`. This code gets the right name.

Example taken from
thoughtbot#1978 (comment),
which I reproduced locally.
nickcharlton pushed a commit to pablobm/administrate that referenced this issue Aug 8, 2022
Fixes thoughtbot#1978

This includes the namespace of the associated class. If the associated class
is `System::Build`, the previous code would tell us that the name was `Build`.
This code gets the right name.
nickcharlton pushed a commit that referenced this issue Aug 8, 2022
Fixes #1978

This includes the namespace of the associated class. If the associated class
is `System::Build`, the previous code would tell us that the name was `Build`.
This code gets the right name.
@pablobm
Copy link
Collaborator

pablobm commented Aug 8, 2022

For now, we have merged the fix, which we think will solve this issue. Please let us know if you think otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug breakages in functionality that is implemented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants