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

saving nilable item with form #27

Closed
Djok39 opened this issue Jan 26, 2019 · 6 comments
Closed

saving nilable item with form #27

Djok39 opened this issue Jan 26, 2019 · 6 comments
Milestone

Comments

@Djok39
Copy link

Djok39 commented Jan 26, 2019

when save empty field it raises error

Should allow save nilable values in database
needed change in src/lucky_record/form.cr:

def set_{{ field[:name] }}_from_param(_value)
  parse_result = {{ field[:type] }}::Lucky.parse(_value)
  if parse_result.is_a? LuckyRecord::Type::SuccessfulCast
    {{ field[:name] }}.value = parse_result.value
  else
    {{ field[:name] }}.add_error "is invalid"
  end
end

to this:

def set_{{ field[:name] }}_from_param(_value)
  parse_result = {{ field[:type] }}::Lucky.parse(_value)
  if parse_result.is_a? LuckyRecord::Type::SuccessfulCast
    {{ field[:name] }}.value = parse_result.value
  else
    {% if field[:nilable] %}
      {{ field[:name] }}.value = nil
    {% else %}
      {{ field[:name] }}.add_error "is invalid"
    {% end %}
  end
end

or cast emty string as nil

@paulcsmith paulcsmith transferred this issue from luckyframework/lucky_record Feb 2, 2019
@snadon
Copy link

snadon commented Apr 25, 2019

I'm also having this issue and the suggest fix does work.

@paulcsmith Are you okay with this fix? I can make a PR.

@paulcsmith
Copy link
Member

paulcsmith commented Apr 25, 2019

@snadon I think the problem with this approach is that if you want an Int32? and pass it "123" it would not leave an error, which it should. Instead I think the Lucky parser should treat "" as nil. The logic for those is defined by each type and the base parser:

I think what we'd want to do is somehow change the Avram::Type to first check if the value is "" and if it is. convert it to nil and then call parse. If you want to take a stab at that LMK. Otherwise I can try to take a look later! Happy to chat on Gitter as well if you need some help

@snadon
Copy link

snadon commented Apr 25, 2019

Okay, so rapidly, if we cast Nil whenever an Int32 as a value of "":

# src/avram/charms/int32_extensions.cr
# line: 10
def self.parse(value : String)
  if value.empty?
    SuccessfulCast(Nil).new(nil)
  else
    SuccessfulCast(Int32).new value.to_i
end

and to make it compile, we need to add | Nil:

# src/avram/where.cr
# line: 6
def initialize(@column : Symbol | String, @value : String | Array(String) | Array(Int32) | Nil)
end

This fixes my problem but I cannot run the tests at the moment. I would need to install docker on my laptop. Do you think there are repercussions?

If by any chance this is good, I can make a PR (including changes for Float64 and Int64).

@paulcsmith
Copy link
Member

I think the change should be made in Avram::Type so that all object types get this behavior. Also adding the Nil to Avram::Where indicates maybe we need to handle it differently so that we don't leak nil to a bunch of different places. I'm not 100% sure how to approach it though since I haven't touched this part of the code.

If you wanna dive in more that is great, but if not I'll take a look later

@snadon
Copy link

snadon commented Apr 26, 2019

Thank you @paulcsmith for your great help on this issue.

I'm dropping this here for anybody who needs a workaround:

# Add this file to your mixins folder
# src/forms/mixins/handle_nil_fields.cr
module HandleNilFields
  extend self

  def handle_nil(field)
    if field.value.nil? || field.param.empty?
      field.reset_errors
      field.value = nil
    end
  end
end

And use it like this:

# foo is any Int32? field that can be null in the database
# forms/test_form.cr
class TestForm < Test::BaseForm
  include HandleNilFields
  fillable foo

  def prepare
    handle_nil foo
  end
end

@jwoertink
Copy link
Member

I believe this is fixed in master with the latest changes from #381.

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 a pull request may close this issue.

4 participants