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

Remove NameError rescue in page base #920

Merged
merged 2 commits into from Nov 1, 2019

Conversation

ndrluis
Copy link
Contributor

@ndrluis ndrluis commented Jul 2, 2017

Fixes #480

@ndrluis
Copy link
Contributor Author

ndrluis commented Jul 2, 2017

The rescue statement generates a false positive on payments feature specs, the timestamps migrations are created to fix this.

@nickcharlton
Copy link
Member

This looks good! Could I get you to fix the Hound comment above?

@pablobm
Copy link
Collaborator

pablobm commented Nov 1, 2019

Stale :-( Happy to see this reopen if someone wants to take over.

@pablobm pablobm closed this Nov 1, 2019
@pablobm
Copy link
Collaborator

pablobm commented Nov 1, 2019

Actually, I thought better of this one. I'm going to try see it through.

@pablobm pablobm reopened this Nov 1, 2019
@pablobm pablobm merged commit dc856a9 into thoughtbot:master Nov 1, 2019
@wkirby
Copy link

wkirby commented Mar 16, 2020

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?

@sedubois
Copy link
Contributor

@wkirby I also filed regressions from this PR here:
#1570

@pablobm
Copy link
Collaborator

pablobm commented Mar 26, 2020

@wkirby - Thank you for your report. I have filed a separate issue at #1586

@nickcharlton nickcharlton added bug breakages in functionality that is implemented dashboards how administrate presents fields and displays data labels Mar 26, 2020
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 dashboards how administrate presents fields and displays data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NoMethodError is silently ignored
6 participants