Automatically build missing values for valid fields. #6
Conversation
CrAsH1101
force-pushed
the
auto-build-for-update
branch
12 times, most recently
from
7e58012
to
8317cd1
Compare
|
@Liooo I've refactored my code so that original Let me know what do you think ;) |
CrAsH1101
force-pushed
the
auto-build-for-update
branch
from
8317cd1
to
3b83611
Compare
|
@Liooo Can you please provide some feedback on this PR? Thank you! |
|
Hey @CrAsH1101 sorry it's late 🙏 I'll check it when I have time so plz wait bit more (I forgot ruby and need some time to remember stuff 😂) |
CrAsH1101
force-pushed
the
auto-build-for-update
branch
2 times, most recently
from
65e6f3a
to
8c1dc32
Compare
|
@Liooo Ok, no problem. In the meantime, I've added tests to completely cover new methods added in this PR. Hope you like it ;) |
CrAsH1101
force-pushed
the
auto-build-for-update
branch
from
ba9d291
to
63b9f84
Compare
lib/dynabute/dynabutable.rb
Outdated
| name_or_id = {name: name, id: id}.compact | ||
| return nil if name_or_id.blank? && field.blank? | ||
| field_obj = field || Dynabute::Field.find_by(name_or_id.merge(target_model: self.class.to_s)) | ||
| fail Dynabute::FieldNotFound.new(name_or_id, field) if field_obj.nil? | ||
| field_obj = field | ||
| if field_obj.blank? | ||
| name_or_id = {name: name, id: id}.compact | ||
| return nil if name_or_id.blank? | ||
| field_obj = Dynabute::Field.find_by(name_or_id.merge(target_model: self.class.to_s)) | ||
| fail Dynabute::FieldNotFound.new(name_or_id, field) if field_obj.nil? | ||
| end |
There was a problem hiding this comment.
It'd be great if there's an argument validation logic here, that throws error when 0 or more than 2 arguments are present.
There was a problem hiding this comment.
Original implementation was just returning nil so I kept it that way. Please check now the method. I have updated tests too.
| if field_obj.has_many | ||
| if value_id | ||
| value_obj = value_obj.detect{|v| v.id == value_id} if value_obj | ||
| fail ValueNotFound unless value_obj | ||
| else | ||
| value_obj = build_dynabute_value(field: field_obj) | ||
| end |
There was a problem hiding this comment.
set_dynabute_attributes() for has_many field sounds more like it's replacing the whole values with the given array. having append_dynabute_value (which should throw error when called on non has_many field) seems more natural to me, what do you think?
There was a problem hiding this comment.
My idea was that you have single method for setting the values. It was intended to be like create_or_update concept.
Just like rails, when submitting a form for persisted object (id exists), it will trigger update action. In case the object is not persisted and there is no id value, form will submit create action and the new record will be added to the database.
So first time you call method for has_many field, it will add specified value, same as with strings and integers, but it will return an Array. Next set will just append to that Array as something already exists.
Let me know if you have something specific on your mind 😉
CrAsH1101
force-pushed
the
auto-build-for-update
branch
2 times, most recently
from
cd4aa54
to
65b070a
Compare
Allow update value for fields that do not have inserted record in appropriate value table.
CrAsH1101
force-pushed
the
auto-build-for-update
branch
from
65b070a
to
09c322a
Compare
Allow update value for fields that do not have inserted record in
appropriate value table.
Currently, initial creation of values records are done through
build_dynabute_valuemethod. If we want to update existing value, we can't usebuild_dynabute_valueas that would result in new value being created for the same field.The idea here is to automatically call
build_dynabute_valueif the record doesn't exist, and have helper functions to achieve that.