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.

Sign up for GitHub

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

Jump to bottom

Fix issue with nil properties not being set and passing hash with string keys issue #164

Merged
merged 4 commits into from
May 11, 2021
Merged

Fix issue with nil properties not being set and passing hash with string keys issue #164

merged 4 commits into from
May 11, 2021

Conversation

ErickFabian
Copy link
Contributor

@ErickFabian ErickFabian commented Apr 14, 2021 •
edited
Loading

Fixes:

  • Properties being set to nil would not be updated

javierg
javierg requested changes Apr 14, 2021
View reviewed changes
Copy link
Member

@javierg javierg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are 2 issues, can you create 2 different PRs?

@ErickFabian
Copy link
Contributor Author

@javierg Done, this is the other PR #165

@ErickFabian ErickFabian changed the base branch from 3.0.3 to 3.1 May 10, 2021 18:07
@javierg
Copy link
Member

javierg commented May 10, 2021

@ErickFabian can you write a test please. I am not sure if this change will make any missing attribute on the model being removed.

@ErickFabian
Copy link
Contributor Author

@javierg I've added a spec for setting nil to a property on a doc

@ErickFabian ErickFabian requested a review from javierg May 10, 2021 18:38
javierg
javierg reviewed May 11, 2021
View reviewed changes
test/property_manager_test.rb Outdated

class PropertyManagerTest < Test::Unit::TestCase
test 'write_attribute with nil value' do
doc = TestDoc.new(name: 'name')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add more properties, other than the one being updated

@ErickFabian ErickFabian requested a review from javierg May 11, 2021 14:16
javierg
javierg approved these changes May 11, 2021
View reviewed changes
Copy link
Member

@javierg javierg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@ErickFabian ErickFabian merged commit ecccc4c into 3.1 May 11, 2021
@ErickFabian ErickFabian deleted the fix_update_properties branch May 11, 2021 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javierg javierg javierg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ErickFabian @javierg