-
Notifications
You must be signed in to change notification settings - Fork 982
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 #4289 - fix issue deleting lookup values for hosts #1222
Conversation
@isratrade could you review this please? Jason mentioned in IRC that this might be rails/rails#5249 It'd be useful to add a test for it too, we did just merge 130f301 with a test of nested lookup values, so it might be a good starting point. |
I'm not so sure that rails bug is the same thing. I found https://github.com/yakaz/rails-bugs while searching around last night and it looked similar, but nothing that convinces me it is definitely the same issue. All I know is that with the original line 'lookup_values.find attr.delete(:id)' never returned anything, though attr.delete(:id) did. |
@domcleal is something like below acceptable? It succeeds only if ":_destroy => true" is included. The interesting part is the test works with or without this PR when being run on Fedora 19. That prompted me to go ahead and load up a Fedora 19 foreman server, and even using the same db, the failure doesn't happen there. Only on RHEL 6 in the web ui so far. Have not been able to get tests to run using SCL on RHEL 6 yet. Maybe something that was fixed between 3.2.8 and 3.2.13? A bit at a loss to explain it. test "should delete lookup_value if :_destroy => true" do |
@jmontleon, I added some tests in PR #1226 for adding, deleting, and updating lookup_values on host. I could not duplicate the issue. I tried Rails 3.2.3, 3.2.8, and 3.2.13 on Postgres and MySQL and didn't hit the error. In the console, where using the
however, the correct sql from
|
@jmontleon, what version of rails is it failing for you? |
@@ -28,7 +28,7 @@ def lookup_values_attributes= lookup_values_attributes | |||
lookup_values_attributes.each_value do |attribute| | |||
attr = attribute.dup | |||
if attr.has_key? :id | |||
lookup_value = lookup_values.find attr.delete(:id) | |||
lookup_value = LookupValue.where(:id => attr.delete(:id)).first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmontleon, this fix should be OK. The only caveat is the low chance that somebody POSTs, PUTs, or DELETEs a LookupValue id that is not included in the host.lookup_values. This would only happen outside the UI so I see it as a very low.
rails 3.2.8 from RH SCL 1.0. That said, I cannot for the life of me reproduce the failure today. We should probably just close this for now and if I see it happen again I can try to figure out what is causing it. |
Okay, thanks Jason. |
I do not really understand why the original line is not working. If I change it to find_by_id I get the error message below. I'm not sure why it is looking for managed_id and id, but this change works around it and I am then able to delete lookup_values from the host edit. Maybe there is a better solution?
"Operation FAILED: Mysql2::Error: Unknown column 'lookup_values.managed_id' in 'where clause': SELECT
lookup_values
.* FROMlookup_values
WHERElookup_values
.managed_id
= 1 ANDlookup_values
.id
= 1 LIMIT 1"