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

Add support for editing polymorphic fields - Revisited #1020

Merged
merged 14 commits into from
Nov 17, 2017

Conversation

pablobm
Copy link
Collaborator

@pablobm pablobm commented Nov 1, 2017

This adds support for editing polymorphic fields. Props go to @pedantic-git, on whose work this is based. This PR supersedes #984.

An important aspect of this PR is how it handles the polymorphic value. At heart, polymorphic values are split in two: an id and a type. Using the globalid gem we can obtain a string that represents both. However, we still need to tell that this string is indeed a globalid and not a literal value when we get to Administrate::ApplicationController#resource_params. I handle this by splitting the field in two:

  • my_field[value]: the global id, which includes both type and instance id.
  • my_field[type]: literally "Administrate::Field::Polymorphic"

Then #resource_params can look at the param values and treat them appropriately. If it's a simple value such a string, it does nothing special. However, if it comes in this value/type form, we treat it as polymorphic, reading the global id.

@@ -16,7 +17,7 @@
end
end

it { should_permit_param(:foo, for_attribute: :foo) }
it { should_permit_param({ foo: [:type, :value] }, for_attribute: :foo) }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use %i or %I for an array of symbols.


expect(page).to have_link(customer.name)
expect(page).to have_flash(
t("administrate.controller.create.success", resource: "LogEntry")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a comma after the last parameter of a multiline method call.

LogEntry.create!({
action: 'create',
logeable: order,
})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent the right brace the same as the first position after the preceding left parenthesis.

@@ -53,6 +63,10 @@
address_state: Faker::Address.state_abbr,
address_zip: Faker::Address.zip,
)
LogEntry.create!({
action: 'create',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -53,6 +63,10 @@
address_state: Faker::Address.state_abbr,
address_zip: Faker::Address.zip,
)
LogEntry.create!({

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant curly braces around a hash parameter.

@@ -27,8 +28,11 @@
"Administrate::Field::Polymorphic",
display_associated_resource: product.name,
data: product,
attribute: 'product',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

end

double("Form Builder").as_null_object.tap do |form_builder|
allow(form_builder).to receive(:fields_for) do |*args, &block|

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused block argument - args. If it's necessary, use _ or _args as an argument name to indicate that it won't be used.

end

def permitted_attribute
{ attribute => [:type, :value] }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use %i or %I for an array of symbols.

end

def self.permitted_attribute(attr)
{ attr => [:type, :value] }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use %i or %I for an array of symbols.

@@ -120,7 +120,20 @@ def resource_includes

def resource_params
params.require(resource_class.model_name.param_key).
permit(dashboard.permitted_attributes)
permit(dashboard.permitted_attributes).
transform_values {|v| read_param_value(v) }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space between { and | missing.

@pedantic-git
Copy link
Contributor

Wow! This is amazing! Thanks for your additional work to bring this up to the right standard!

end
end
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra empty line detected at block body end.

logeable: {
type: "Administrate::Field::Polymorphic",
value: logeable.to_global_id.to_s,
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a comma after the last parameter of a multiline method call.

describe "with valid params" do
it "creates a new LogEntry" do
customer = create(:customer)
expect {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using {...} for multi-line blocks.

logeable: {
type: "Administrate::Field::Polymorphic",
value: logeable.to_global_id.to_s,
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a comma after the last parameter of a multiline method call.

@pablobm pablobm force-pushed the edit_polymorphic-pablobm branch from f6b6981 to 6003e99 Compare November 2, 2017 16:17
@pablobm pablobm force-pushed the edit_polymorphic-pablobm branch from 6003e99 to eaa7c5e Compare November 2, 2017 16:18
@pablobm
Copy link
Collaborator Author

pablobm commented Nov 2, 2017

I'm going to leave this to rest for a week, just in case there is any more feedback. I'll merge it Friday next week if there are no issues.

@ewilliam
Copy link

will this be merged soon? thanks @pablobm for your work!

@pablobm
Copy link
Collaborator Author

pablobm commented Nov 17, 2017

I shall...

make it so

@pablobm pablobm merged commit 8329b21 into master Nov 17, 2017
@tysongach tysongach deleted the edit_polymorphic-pablobm branch November 17, 2017 19:43
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.

4 participants