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

Introduce #assign_preserving method #41

Open
flvrone opened this issue Oct 10, 2019 · 3 comments
Open

Introduce #assign_preserving method #41

flvrone opened this issue Oct 10, 2019 · 3 comments
Labels
help wanted Extra attention is needed

Comments

@flvrone
Copy link

flvrone commented Oct 10, 2019

Which will preserve existing nested attributes :)
For me, it's quite a boilerplate code now, and it's useful, so I think it might be handy to be here out of the box.
Here's an example of something I already have:

class Site
  class Settings
    include StoreModel::Model

    NESTED_MODELS_TO_PRESERVE = %i[colours style].freeze

    attribute :twitter_handle, :string
    # ...

    attribute :colours, ColourScheme.to_type, default: -> { ColourScheme.new }
    attribute :style, Styling.to_type, default: -> { Styling.new }
    attribute :footer_pages, FooterPage.to_array_type, default: -> { [] }

    def assign_preserving(attrs)
      attrs = attrs.dup

      NESTED_MODELS_TO_PRESERVE.each do |model_key|
        next unless (nested_model = send(model_key))
        next unless (nested_attrs = attrs.delete(model_key))

        nested_model.assign_preserving(nested_attrs)
      end

      # NOTE: footer_pages are not being "preserved"
      #       and are overwritten if given, because it is an array
      assign_attributes(attrs)
    end
  end
end

And all the nested models have the same assign_preserving method, however their NESTED_MODELS_TO_PRESERVE might be blank.
Yeah, and I guess we might have something more sophisticated than a constant of all nested models.

@DmitryTsepelev
Copy link
Owner

Hi @FunkyloverOne!

Thanks for the reaching out, I like the idea! Let's design the API for it 🙂

I think it should be possible to patch the .attributedefinition to accept preserve: argument:

class Site
  class Settings
    include StoreModel::Model

    attribute :twitter_handle, :string
    # ...

    attribute :colours, ColourScheme.to_type, default: -> { ColourScheme.new }, preserve: true
    attribute :style, Styling.to_type, default: -> { Styling.new }, preserve: true
    attribute :footer_pages, FooterPage.to_array_type, default: -> { [] }
  end
end

In this case #assign_preserving can be moved to StoreModel::Model, and we should also make sure that if preserve: true is passed than type is either JsonType or ArrayType. What do you think?

@flvrone
Copy link
Author

flvrone commented Oct 10, 2019

Hey @DmitryTsepelev, one important thing is that we also need to have that #assign_preserving on a parent ActiveRecord model.

Arrays preservation is not that easy as singular JSONs. You see, we need a way of identifying each individual element, updating/removing it, adding new ones. It is supposed to be similar to ActiveRecord's accepts_nested_attributes_for API.

But we could start without arrays preservation support, it's already good enough 😄

@DmitryTsepelev
Copy link
Owner

Makes sense. We should probably change the name to something that can be easily associated with StoreModel (like assign_preserving_nested_store_models but less verbose)

@DmitryTsepelev DmitryTsepelev added the help wanted Extra attention is needed label Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants