From 62570b673bd43964db8d9dfc09b6fddb1ec7b8be Mon Sep 17 00:00:00 2001 From: Holger Frohloff Date: Tue, 17 Nov 2015 18:07:27 +0100 Subject: [PATCH 1/6] Displays a helpful error message # Problem If a field in `COLLECTION_ATTRIBUTES` has no key in `ATTRIBUTE_TYPES` either because I misspelled it or I forget to add it to `ATTRIBUTE_TYPES` I will get a undefined method new for `nil:NilClass` in `Page::Base` # Solution Catches the `NoMethodError` and reraises it with a proper message. references #246 --- lib/administrate/page/base.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lib/administrate/page/base.rb b/lib/administrate/page/base.rb index 7beaff2d50..dad71f71cb 100644 --- a/lib/administrate/page/base.rb +++ b/lib/administrate/page/base.rb @@ -19,6 +19,8 @@ def attribute_field(dashboard, resource, attribute_name, page) dashboard. attribute_types[attribute_name]. new(attribute_name, value, page) + rescue NoMethodError + raise NoMethodError.new(error_message(attribute_name), :new) end def get_attribute_value(resource, attribute_name) @@ -28,6 +30,15 @@ def get_attribute_value(resource, attribute_name) end attr_reader :dashboard, :options + + private + + def error_message(attribute_name) + < Date: Wed, 18 Nov 2015 11:02:26 +0100 Subject: [PATCH 2/6] Display helpful message if attribute_name missing # Problem If a field in `COLLECTION_ATTRIBUTES` has no key in `ATTRIBUTE_TYPES` either because I misspelled it or I forget to add it to `ATTRIBUTE_TYPES` I will get a undefined method new for `nil:NilClass` in `Page::Base` # Solution Don't access hash values directly but use `fetch`. If the attribute is not found the block raises an error with a helpful error message. references #246 --- lib/administrate/page/base.rb | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/lib/administrate/page/base.rb b/lib/administrate/page/base.rb index dad71f71cb..a889814d2e 100644 --- a/lib/administrate/page/base.rb +++ b/lib/administrate/page/base.rb @@ -16,11 +16,14 @@ def resource_name def attribute_field(dashboard, resource, attribute_name, page) value = get_attribute_value(resource, attribute_name) - dashboard. - attribute_types[attribute_name]. - new(attribute_name, value, page) - rescue NoMethodError - raise NoMethodError.new(error_message(attribute_name), :new) + field = dashboard.attribute_types.fetch(attribute_name) do + fail <<-ERROR_MESSAGE.strip_heredoc + Attribute :#{attribute_name} not found in #{dashboard.class}. + Perhaps you misspelled it or it is missing in + #{dashboard.class}::ATTRIBUTE_TYPES? + ERROR_MESSAGE + end + field.new(attribute_name, value, page) end def get_attribute_value(resource, attribute_name) @@ -30,15 +33,6 @@ def get_attribute_value(resource, attribute_name) end attr_reader :dashboard, :options - - private - - def error_message(attribute_name) - < Date: Wed, 18 Nov 2015 13:01:37 -0500 Subject: [PATCH 3/6] Move the error message to the dashboard class This decreases the knowledge that collaborators need about the internal structure of the dashboard class. --- lib/administrate/base_dashboard.rb | 12 +++++++ lib/administrate/page/base.rb | 9 +---- lib/administrate/page/collection.rb | 2 +- spec/dashboards/customer_dashboard_spec.rb | 39 ++++++++++++++++++++++ 4 files changed, 53 insertions(+), 9 deletions(-) diff --git a/lib/administrate/base_dashboard.rb b/lib/administrate/base_dashboard.rb index f883a75dd1..a85920378c 100644 --- a/lib/administrate/base_dashboard.rb +++ b/lib/administrate/base_dashboard.rb @@ -18,6 +18,18 @@ def attribute_types self.class::ATTRIBUTE_TYPES end + def attribute_type_for(attribute_name) + attribute_types.fetch(attribute_name) do + fail "Attribute #{attribute_name} not in #{self.class}::ATTRIBUTE_TYPES" + end + end + + def attribute_types_for(*attribute_names) + attribute_names.each_with_object({}) do |name, attributes| + attributes[name] = attribute_type_for(name) + end + end + def form_attributes self.class::FORM_ATTRIBUTES end diff --git a/lib/administrate/page/base.rb b/lib/administrate/page/base.rb index a889814d2e..4f5d52296c 100644 --- a/lib/administrate/page/base.rb +++ b/lib/administrate/page/base.rb @@ -15,14 +15,7 @@ def resource_name def attribute_field(dashboard, resource, attribute_name, page) value = get_attribute_value(resource, attribute_name) - - field = dashboard.attribute_types.fetch(attribute_name) do - fail <<-ERROR_MESSAGE.strip_heredoc - Attribute :#{attribute_name} not found in #{dashboard.class}. - Perhaps you misspelled it or it is missing in - #{dashboard.class}::ATTRIBUTE_TYPES? - ERROR_MESSAGE - end + field = dashboard.attribute_type_for(attribute_name) field.new(attribute_name, value, page) end diff --git a/lib/administrate/page/collection.rb b/lib/administrate/page/collection.rb index 9c9a865364..757c796edc 100644 --- a/lib/administrate/page/collection.rb +++ b/lib/administrate/page/collection.rb @@ -14,7 +14,7 @@ def attributes_for(resource) end def attribute_types - dashboard.attribute_types.slice(*attribute_names) + dashboard.attribute_types_for(*attribute_names) end def ordered_html_class(attr) diff --git a/spec/dashboards/customer_dashboard_spec.rb b/spec/dashboards/customer_dashboard_spec.rb index 681a320bf5..d41c651ed7 100644 --- a/spec/dashboards/customer_dashboard_spec.rb +++ b/spec/dashboards/customer_dashboard_spec.rb @@ -24,6 +24,45 @@ end end + describe "#attribute_type_for" do + context "for an existing attribute" do + it "returns the attribute field" do + dashboard = CustomerDashboard.new + field = dashboard.attribute_type_for(:name) + expect(field).to eq Administrate::Field::String + end + end + + context "for a non-existent attribute" do + it "raises an exception" do + dashboard = CustomerDashboard.new + expect { dashboard.attribute_type_for(:foo) }. + to raise_error /Attribute foo not in CustomerDashboard/ + end + end + end + + describe "#attribute_types_for" do + context "for existing attributes" do + it "returns the attribute fields" do + dashboard = CustomerDashboard.new + fields = dashboard.attribute_types_for(:name, :email) + expect(fields).to match( + name: Administrate::Field::String, + email: Administrate::Field::Email, + ) + end + end + + context "for one non-existent attribute" do + it "raises an exception" do + dashboard = CustomerDashboard.new + expect { dashboard.attribute_types_for(:name, :foo) }. + to raise_error /Attribute foo not in CustomerDashboard/ + end + end + end + describe "#display_resource" do it "returns the customer's name" do customer = double(name: "Example Customer") From c71fa38b8d229c1cab9c621f87101b2f6362b5ee Mon Sep 17 00:00:00 2001 From: Julien Vanier Date: Wed, 18 Nov 2015 13:57:34 -0500 Subject: [PATCH 4/6] Reword error message --- lib/administrate/base_dashboard.rb | 2 +- spec/dashboards/customer_dashboard_spec.rb | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/administrate/base_dashboard.rb b/lib/administrate/base_dashboard.rb index a85920378c..623b765919 100644 --- a/lib/administrate/base_dashboard.rb +++ b/lib/administrate/base_dashboard.rb @@ -20,7 +20,7 @@ def attribute_types def attribute_type_for(attribute_name) attribute_types.fetch(attribute_name) do - fail "Attribute #{attribute_name} not in #{self.class}::ATTRIBUTE_TYPES" + fail "Attribute #{attribute_name} could not be found in #{self.class}::ATTRIBUTE_TYPES" end end diff --git a/spec/dashboards/customer_dashboard_spec.rb b/spec/dashboards/customer_dashboard_spec.rb index d41c651ed7..98d9a94f8b 100644 --- a/spec/dashboards/customer_dashboard_spec.rb +++ b/spec/dashboards/customer_dashboard_spec.rb @@ -24,6 +24,10 @@ end end + def missing_attribute_message(attribute, dashboard_class) + "Attribute #{attribute} could not be found in #{dashboard_class}::ATTRIBUTE_TYPES" + end + describe "#attribute_type_for" do context "for an existing attribute" do it "returns the attribute field" do @@ -37,7 +41,7 @@ it "raises an exception" do dashboard = CustomerDashboard.new expect { dashboard.attribute_type_for(:foo) }. - to raise_error /Attribute foo not in CustomerDashboard/ + to raise_error missing_attribute_message("foo", "CustomerDashboard") end end end @@ -58,7 +62,7 @@ it "raises an exception" do dashboard = CustomerDashboard.new expect { dashboard.attribute_types_for(:name, :foo) }. - to raise_error /Attribute foo not in CustomerDashboard/ + to raise_error missing_attribute_message("foo", "CustomerDashboard") end end end From 772774f8c4b930348868c8f07f461bd36172d88e Mon Sep 17 00:00:00 2001 From: Julien Vanier Date: Wed, 18 Nov 2015 15:09:16 -0500 Subject: [PATCH 5/6] Remove splats in internal methods --- lib/administrate/base_dashboard.rb | 2 +- lib/administrate/page/collection.rb | 2 +- spec/dashboards/customer_dashboard_spec.rb | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/administrate/base_dashboard.rb b/lib/administrate/base_dashboard.rb index 623b765919..5fc384f02b 100644 --- a/lib/administrate/base_dashboard.rb +++ b/lib/administrate/base_dashboard.rb @@ -24,7 +24,7 @@ def attribute_type_for(attribute_name) end end - def attribute_types_for(*attribute_names) + def attribute_types_for(attribute_names) attribute_names.each_with_object({}) do |name, attributes| attributes[name] = attribute_type_for(name) end diff --git a/lib/administrate/page/collection.rb b/lib/administrate/page/collection.rb index 757c796edc..1462faa1a9 100644 --- a/lib/administrate/page/collection.rb +++ b/lib/administrate/page/collection.rb @@ -14,7 +14,7 @@ def attributes_for(resource) end def attribute_types - dashboard.attribute_types_for(*attribute_names) + dashboard.attribute_types_for(attribute_names) end def ordered_html_class(attr) diff --git a/spec/dashboards/customer_dashboard_spec.rb b/spec/dashboards/customer_dashboard_spec.rb index 98d9a94f8b..a9350a1e0c 100644 --- a/spec/dashboards/customer_dashboard_spec.rb +++ b/spec/dashboards/customer_dashboard_spec.rb @@ -50,7 +50,7 @@ def missing_attribute_message(attribute, dashboard_class) context "for existing attributes" do it "returns the attribute fields" do dashboard = CustomerDashboard.new - fields = dashboard.attribute_types_for(:name, :email) + fields = dashboard.attribute_types_for([:name, :email]) expect(fields).to match( name: Administrate::Field::String, email: Administrate::Field::Email, @@ -61,7 +61,7 @@ def missing_attribute_message(attribute, dashboard_class) context "for one non-existent attribute" do it "raises an exception" do dashboard = CustomerDashboard.new - expect { dashboard.attribute_types_for(:name, :foo) }. + expect { dashboard.attribute_types_for([:name, :foo]) }. to raise_error missing_attribute_message("foo", "CustomerDashboard") end end From 58c6017e7942d37e278e2bdd3dc80464b8e3cc55 Mon Sep 17 00:00:00 2001 From: Julien Vanier Date: Wed, 18 Nov 2015 15:59:08 -0500 Subject: [PATCH 6/6] Extract attribute_not_found_message --- lib/administrate/base_dashboard.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/administrate/base_dashboard.rb b/lib/administrate/base_dashboard.rb index 5fc384f02b..a542b698ac 100644 --- a/lib/administrate/base_dashboard.rb +++ b/lib/administrate/base_dashboard.rb @@ -20,7 +20,7 @@ def attribute_types def attribute_type_for(attribute_name) attribute_types.fetch(attribute_name) do - fail "Attribute #{attribute_name} could not be found in #{self.class}::ATTRIBUTE_TYPES" + fail attribute_not_found_message(attribute_name) end end @@ -51,5 +51,11 @@ def collection_attributes def display_resource(resource) "#{resource.class} ##{resource.id}" end + + private + + def attribute_not_found_message(attr) + "Attribute #{attr} could not be found in #{self.class}::ATTRIBUTE_TYPES" + end end end