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

Don't convert nil to '' (empty string) #1478

Closed
wants to merge 1 commit into from

Conversation

santib
Copy link
Contributor

@santib santib commented Dec 2, 2019

If I have a record wil nil values, when updating anything else in the record, it will change my nil values for ''. This PR fixes that

@pablobm pablobm self-assigned this Dec 5, 2019
@pablobm
Copy link
Collaborator

pablobm commented Dec 5, 2019

Hello @santib, and thank you for contributing to Administrate!

I'm unsure about this change. It could be either way, but I feel that the current behaviour (converting nil strings to empty strings) would be the expected behaviour for users.

A bit of context first: when a text field is submitted to the backend and there's no value, there's no right or wrong in interpreting it as null/nil or as an empty string. That's the framework's choice, and at the same time it's legitimate for users to want it the other way around.

With Rails, I think an empty string is the expected value. For example, if I create a scaffold (I checked with Rails 6.0.1), I get the same behaviour you are seeing: empty values are interpreted as empty string rather than nils. Therefore, I think Administrate should behave the same way by default.

Fortunately, there's a way to override this default without having to change Administrate: you can define how to interpret submitted values by overriding resource_params. This is an example:

# Put this in your Admin::ApplicationController,
# or on individual admin controllers
private

def resource_params
  params.require(resource_name)
    .permit(dashboard_class::FORM_ATTRIBUTES)
    .transform_values do |value|
      value == "" ? nil : value
    end
end

I'm realising that resource_params is not documented. I'll fix that.

Does this make sense? Did I misunderstand you? Let me know.

@santib
Copy link
Contributor Author

santib commented Dec 5, 2019

@pablobm Thanks Pablo for the explanation, that sounds good to me 👍

@santib santib closed this Dec 5, 2019
@santib santib deleted the fix-empty-values branch December 5, 2019 14:29
@sedubois
Copy link
Contributor

sedubois commented Dec 5, 2019

@pablobm on a similar note if one has a nullable boolean field, there is no way (using Field::Boolean) to allow keeping the value as nil when editing the record, it will be forced to false.

AFAIK Administrate does not allow manipulating records that should be able to store both nil and empty field values.

@pablobm pablobm mentioned this pull request Dec 5, 2019
@pablobm
Copy link
Collaborator

pablobm commented Dec 5, 2019

@sedubois - Not sure if I understand correctly. Is this what you are trying to do?

Persisted value Input Should persist as
nil "0" nil
nil "1" true
false "0" false
false "1" true
true "0" false
true "1" true

If that's what you want, you may be able to achieve it by overwriting resource_params too. Something like this (quick implementation, no warranty!):

private

def resource_params
  params.require(resource_class.model_name.param_key).
    permit(dashboard.permitted_attributes).
    to_h.
    map { |k, v| [k, cast_input_value(k, v)] }.
    to_h
end

def cast_input_value(attr, value)
  if dashboard_class::ATTRIBUTE_TYPES[attr.to_sym] == Administrate::Field::Boolean
    current_value = requested_resource[attr]
    if current_value.nil? && value == "0"
      nil
    else
      value
    end
  else
    value
  end
end

If the truth table you are looking for is different, it should be achievable with a variation of this.

I have to admit this is not the prettiest sight. I'm thinking perhaps each field class should be able to tell how it will cast its values. I'll have to discuss that with other maintainers. But it should work! :-)

Let me know if that solves your problem.

@sedubois
Copy link
Contributor

sedubois commented Dec 6, 2019

@pablobm that could work but it offers no way to set the value back to nil once it's true/false. Alternately I thought the UI could allow specifying nil/false/true directly, if the field is nullable (instead of the only 2 choices currently possible), using 3 radio buttons instead of a checkbox.

As far as strings are concerned, my workaround so far has indeed been to set nil if the string is blank:

# app/controllers/admin/application_controller.rb
module Admin
  class ApplicationController < Administrate::ApplicationController
    ...

    # Hack: Store nil instead of empty String values when submitting forms
    # https://github.com/thoughtbot/administrate/issues/441
    def read_param_value(data)
      data.is_a?(String) and data.blank? ? nil : super
    end
  end
end

I haven't yet implemented/chosen how to fix the situation for booleans.

@pablobm
Copy link
Collaborator

pablobm commented Dec 7, 2019

I see. In that case you need to create a new field type, as per http://administrate-prototype.herokuapp.com/adding_custom_field_types. This will have its own field template, and you can make it look like radio buttons, or anything you like.

nickcharlton pushed a commit that referenced this pull request Dec 27, 2019
When reviewing #1478, it became apparent to me that controller APIs such as `resource_params` are not documented.

I think they should be part of Administrate's interface, and as such should be documented. Here's some documentation.

Additionally:

* Changed the example for `update` so that it's more useful. The current example doesn't render or redirect, so it may cause confusion to someone trying to emulate it.
* Mentioned `requested_resource` too.
* Fixed the indent for the `scoped_resource` example.
KingTiger001 added a commit to KingTiger001/admin-Rails-project that referenced this pull request Jan 15, 2023
When reviewing thoughtbot/administrate#1478, it became apparent to me that controller APIs such as `resource_params` are not documented.

I think they should be part of Administrate's interface, and as such should be documented. Here's some documentation.

Additionally:

* Changed the example for `update` so that it's more useful. The current example doesn't render or redirect, so it may cause confusion to someone trying to emulate it.
* Mentioned `requested_resource` too.
* Fixed the indent for the `scoped_resource` example.
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.

3 participants