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

fixes #6375 - fix needed for Rails 3.2.8 only that ensures reference_id on parameter.rb matches the nested object id #1567

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@isratrade
Copy link
Member

isratrade commented Jul 7, 2014

No description provided.

@GregSutcliffe

This comment has been minimized.

Copy link
Member

GregSutcliffe commented Jul 8, 2014

nitpick: the commit message doesn't relate to the content of the commit, it's a restatement of the bug. It should be something like "fixes #6375 - fixes reference_id for the 3.2.8 ensure_reference_nil hack on parameter.rb" or something similar.

@isratrade isratrade changed the title fixes #6375 - Can't create parameters for operating system with id 1 using API fixes #6375 - fix needed for Rails 3.2.8 only that ensures reference_id on parameter.rb matches the nested object id Jul 8, 2014

@isratrade

This comment has been minimized.

Copy link
Member Author

isratrade commented Jul 8, 2014

@GregSutcliffe, re-commited with new commit message

@GregSutcliffe

This comment has been minimized.

Copy link
Member

GregSutcliffe commented Jul 8, 2014

Thanks, much better :)

@isratrade

This comment has been minimized.

Copy link
Member Author

isratrade commented Jul 10, 2014

btw, this hack/fix is only required for creating parameters using API v2.

@ares

This comment has been minimized.

Copy link
Member

ares commented Jul 10, 2014

Could you please describe why exactly is the ensure_reference_nil hack needed in the code comment or add link to a redmine issue? everytime I see it I'm wondering why do we need to nilify that and later on some objects set the correct value manually. The original issue http://projects.theforeman.org/issues/5129 does not contain much of explanation.

@@ -34,6 +34,8 @@ def strip_whitespaces
end

# hack fix for Rails 3.2.8. Not needed for 3.2.18.
# related to **accepts_nested_attributes_for** on UI form (not API)
# which incorrectly assigns foreign key to 1 when attributes are from STI class (DomainParamter, HostParameter, etc)

This comment has been minimized.

Copy link
@isratrade

isratrade Jul 10, 2014

Author Member

@ares, I updated the comment even though I didn't find the original bug issue on https://github.com/rails/rails

This comment has been minimized.

Copy link
@ares

ares Jul 16, 2014

Member

thank you, I'm wondering if we should check and apply the patch only if Rails.version == '3.2.8'

This comment has been minimized.

Copy link
@isratrade

isratrade Jul 17, 2014

Author Member

@ares, rather than putting if the conditional if Rails.version is 3.2.8 or less, it may be better just to make this a downstream patch, @ohadlevy ?

This comment has been minimized.

Copy link
@ohadlevy

ohadlevy Jul 17, 2014

Member

@isratrade as long we use SCL with rails 3.2.8 the patch is needed.

This comment has been minimized.

Copy link
@ares

ares Jul 21, 2014

Member

why not applying the patch only on version that is affected? also I'd prefer checking this need before we run ensure_reference_nil rather than setting back the nested_obj.id

This comment has been minimized.

Copy link
@isratrade

isratrade Jul 21, 2014

Author Member

@ares, updated with if Rails.version == '3.2.8' added

fixes #6375 - fix needed for Rails 3.2.8 only that ensures reference_…
…id on parameter.rb matches the nested object id
@ares

This comment has been minimized.

Copy link
Member

ares commented Jul 22, 2014

Thank you @isratrade, ACK

@ares ares removed the Needs re-review label Jul 22, 2014

@domcleal

This comment has been minimized.

Copy link
Contributor

domcleal commented Jul 22, 2014

Merged as 5dded91, thanks @isratrade.

@domcleal domcleal closed this Jul 22, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.