Display helpful message if attribute_name missing #251
Conversation
# 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
# 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
|
Thanks for your feedback @monkbroc and @Graysonwright. Do we need tests for this? Would this be a feature test? I am asking for suggestions. |
|
@5minpause if you give me commit access to the repository https://github.com/ikuseiGmbH/administrate I'll write a test and push it to this pull request. |
|
@monkbroc I added you. I am curious what you'll do. Looking forward to learning something new. 😀 |
| fields = dashboard.attribute_types_for(:name, :email) | ||
| expect(fields).to match( | ||
| name: Administrate::Field::String, | ||
| email: Administrate::Field::Email |
There was a problem hiding this comment.
Put a comma after the last parameter of a multiline method call.
|
After looking at this more, it seems that the concern of attributes existing in a dashboard belongs in the dashboard class (surprise!). It's easier to test the behavior there (I added 4 unit tests for the dashboard class). My change also decreases the knowledge collaborators like page classes have to know about the internals of the dashboard class ( Regarding Hound: have you thought of making your line length a little longer? 80 seems rather short for Ruby. I've had better luck with 90 or 100. |
This decreases the knowledge that collaborators need about the internal structure of the dashboard class.
|
Whoa, this looks great! |
| it "raises an exception" do | ||
| dashboard = CustomerDashboard.new | ||
| expect { dashboard.attribute_types_for(:name, :foo) }. | ||
| to raise_error /Attribute foo not in CustomerDashboard/ |
There was a problem hiding this comment.
How would you feel about extracting a method here?
expect { dashboard.attribute_types_for(:name, :foo) }.
to raise_error(message_for("foo", "CustomerDashboard"))
def message_for(attribute, dashboard_class)
"Attribute #{attribute} could not be found in #{dashboard_class}::ATTRIBUTE_TYPES"
endNote: I also slightly changed the wording of the error message in that snippet, I think it reads a bit better.
There was a problem hiding this comment.
I agree about the wording. That's what I had originally but the Hound was against it since it's 95 characters long 😆
I'll put it back
|
Left a small comment, but this looks good to merge after that. |
| @@ -24,6 +24,50 @@ | |||
| end | |||
| end | |||
|
|
|||
|
|
|||
| @@ -24,6 +24,49 @@ | |||
| end | |||
| end | |||
|
|
|||
| def missing_attribute_message(attribute, dashboard_class) | |||
| "Attribute #{attribute} could not be found in #{dashboard_class}::ATTRIBUTE_TYPES" | |||
There was a problem hiding this comment.
I'm ok ignoring hound for this line.
|
Left one more small comment (in reply to hound above), then this is good to go! Thanks! |
|
Done. |
|
Thanks! Merged in as d212265. |
Changes: ``` * [#251] [FEATURE] Raise a helpful error when an attribute is missing from `ATTRIBUTE_TYPES` * [#298] [FEATURE] Support ActiveRecord model I18n translations * [#312] [FEATURE] Add a `nil` option to `belongs_to` form fields * [#231] [UI] Fix layout issue on show page where a long label next to an empty value would cause following fields on the page to be mis-aligned. * [#309] [UI] Fix layout issue in datetime pickers where months and years would not wrap correctly. * [#306] [UI] Wrap long text lines (on word breaks) on show pages * [#214] [UI] Improve header layout when there is a long page title * [#198] [UI] Improve spacing around bottom link in sidebar * [#206] [UI] Left-align checkboxes in boolean form fields * [#315] [UI] Remove the `IDS` suffix for `HasMany` form field labels * [#259] [BUGFIX] Make installation generator more robust by ignoring dynamically generated, unnamed models * [#243] [BUGFIX] Fix up a "Show" button on the edit page that was not using the `display_resource` method. * [#248] [BUGFIX] Improve polymorphic relationship's dashboard class detection. * [#247] [BUGFIX] Populate `has_many` and `belongs_to` select boxes with the current value of the relationship. * [#217] [I18n] Dutch * [#263] [I18n] Swedish * [#272] [I18n] Danish * [#270] [I18n] Don't apologize about missing relationship support. * [#237] [I18n] Fix broken paths for several I18n files (de, es, fr, pt-BR, vi). * [#266] [OPTIM] Save a few database queries by using cached counts ```
Problem
If a field in
COLLECTION_ATTRIBUTEShas no key inATTRIBUTE_TYPESeither because I misspelled it or I forget to add it toATTRIBUTE_TYPESI will get a undefined method new fornil:NilClassinPage::BaseSolution
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.Closes #246