Skip to content

Fix model name#44

Merged
lemontree55 merged 4 commits intomasterfrom
model-name
Jan 31, 2025
Merged

Fix model name#44
lemontree55 merged 4 commits intomasterfrom
model-name

Conversation

@lemontree55
Copy link
Owner

  • Fix inner model name in outer model
  • Change behavior of Model#to_h which Choice elements.

`model` helper defines a name for an embedded model, but that name was not used. Now, this model is identified using this name in outer model.
This avoid clash when embedding a same model more than once.
Choice content in resultant hash is no more the value of the chosen item,
but a hash which key is the item name associated with item value.
In #private_to_h, do not take root element when my_element is a Model, but
transform the model itsel to a Hash. Then,
replace upper key in Hash by model name in current model.
@adfoster-r7
Copy link
Contributor

adfoster-r7 commented Jan 6, 2025

Thanks for taking a look!

I rebased #45 on this PR and it looks like there's still a nil key somehow 🤔

{
  :LdapMessage => {
    :message_id=>1,
    :protocol_op=>{
    :bind_request=>{
      :version=>3,
      :name=>"Administrator@adf3.local",
      nil=>{
        :simple=>"p4$$w0rd"
      }
    }
   }
  }
}

@lemontree55
Copy link
Owner Author

I locally merged your branch on mine, and the result is as expected.

It seems you rebased 1 commit behind branch head. Branch head is c41fea5.

@adfoster-r7
Copy link
Contributor

adfoster-r7 commented Jan 6, 2025

Apologies, thanks for catching that - the test pass now! 🏆

I was reflecting on your original comment - and I was thinking that the current/original to_h behavior is likely what most users expect to_h to behave as - however the new functionality added in this PR would definitely be great for verifying the LDAP integration tests that I'm working on behave as expected

Since this PR would be an API breaking change in terms of behavior for users, I wonder if this something that would make sense as a backwards compatible/opt in API? Maybe result.as_json(include_choice_in_json: true) - similar to https://apidock.com/rails/ActiveModel/Serializers/JSON/as_json include_root_in_json, or a separate method entirely? Let me know your thoughts 🎉

Edit: I see now that this PR impacts more than just the to_h behavior - but also the accessor functionality as far as I can see, so my suggestion isn't viable - but the change would indeed be backwards breaking 👍

@lemontree55 lemontree55 merged commit beea229 into master Jan 31, 2025
7 checks passed
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.

2 participants