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

fix issue with company renaming #9

Merged
merged 2 commits into from
Apr 11, 2016
Merged

Conversation

x4d3
Copy link
Contributor

@x4d3 x4d3 commented Apr 8, 2016

fix the bug in "her" gem by continuing the patching.
The error is the original gem is due to the next call that does not returns the hash
that hash is then given in the inject method as first argument, and since it is nil, cause an error

fix the bug in "her" gem by continuing the patching.
The error is the original gem is due to the next call that does not returns the hash
that hash is then given in the inject method as first argument, and since it is nil, cause an error
@ouranos ouranos added the bug label Apr 8, 2016
@ouranos
Copy link
Contributor

ouranos commented Apr 8, 2016

Good catch.

It'd be nice to make more obvious what was patched as it's hard to see without looking at the source. It's the usual problem with monkey patching as we often copy/paste the original implementation.

Maybe something like:

        # @private
        # TODO: Handle has_one
        def embeded_params(attributes)
          associations[:has_many].select { |a| attributes.include?(a[:data_key])}.compact.inject({}) do |hash, association|
            params = attributes[association[:data_key]].map(&:to_params)
            # PATCH - Return hash 
            next hash if params.empty?
            # EO PATCH
            if association[:class_name].constantize.include_root_in_json?
              root = association[:class_name].constantize.root_element
              hash[association[:data_key]] = params.map { |n| n[root] }
            else
              hash[association[:data_key]] = params
            end
            hash
          end

What do you think?

@x4d3
Copy link
Contributor Author

x4d3 commented Apr 8, 2016

I agree, I'll update.

@ouranos ouranos self-assigned this Apr 11, 2016
@ouranos ouranos merged commit 25304ac into maestrano:master Apr 11, 2016
@x4d3 x4d3 deleted the fix-renaming branch June 19, 2017 07:05
aluqueGH pushed a commit to aluqueGH/mno-enterprise that referenced this pull request Jul 10, 2018
Better build and better previewer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants