Fixes #17918 - Smart class parameters should appear in audits #4165

orrabin · 2017-01-09T12:51:12Z

No description provided.

mention-bot · 2017-01-09T12:51:14Z

@orrabin, thanks for your PR! By analyzing the history of the files in this pull request, we identified @domcleal, @isratrade and @dLobatog to be potential reviewers.

tbrisker

Looks good in general, just a couple of comments inline

tbrisker · 2017-01-15T15:36:49Z

db/migrate/20170109115157_fix_lookup_key_auditable_type.rb

@@ -0,0 +1,12 @@
+class FixLookupKeyAuditableType < ActiveRecord::Migration
+  def up
+    Audit.where(:auditable_type => 'LookupKey').each do |audit|


This would potentially load a very large amount of audits from the DB to check their types. would it be possible to perform this in the database without loading all of the audits and going over them one by one? (maybe change all the puppetLKs in one query and all the variableLKs in a second?) If it isn't possible, please use find_each instead of each so at least this is done in batches.

tbrisker · 2017-01-15T15:39:46Z

app/models/concerns/audit_extensions.rb

@@ -67,6 +67,8 @@ def fix_auditable_type
    self.associated_type = "Host"         if associated_type =~ /Host::/
    self.auditable_type = auditable.type  if auditable_type == "Taxonomy" && auditable
    self.associated_type = auditable.type if auditable_type == "Taxonomy" && auditable
+    self.auditable_type = auditable.type  if auditable_type == "LookupKey" && auditable
+    self.associated_type = auditable.type  if auditable_type == "LookupKey" && auditable


Perhaps refactor these 4 lines to something like this?

self.auditable_type = self.associated_type = auditable.type if auditable && ['LookupKey', 'Taxonomy'].includes? auditable_type

orrabin · 2017-01-16T06:21:34Z

@tbrisker thanks, I fixed you comments.

tbrisker

One more sql optimization :)

tbrisker · 2017-01-16T09:09:30Z

db/migrate/20170109115157_fix_lookup_key_auditable_type.rb

+    lookup_key_ids = Audit.where(:auditable_type => 'LookupKey').pluck(:auditable_id).uniq
+
+    puppetclass_lookup_key_ids =  PuppetclassLookupKey.where(:id => lookup_key_ids).pluck(:id)
+    Audit.where(:auditable_type => 'LookupKey', :auditable_id => puppetclass_lookup_key_ids).update_all(:auditable_type => 'PuppetclassLookupKey')


How about something like:

Audit.joins('JOIN lookup_keys ON lookup_keys.id = audits.auditable_id').where(:auditable_type => 'LookupKey', :lookup_keys => {:type => 'VariableLookupKey'}).reorder(nil)

Also, make sure to reorder(nil) on all the queries to remove the default order scope.

houndci-bot · 2017-01-16T12:45:11Z

db/migrate/20170109115157_fix_lookup_key_auditable_type.rb

+
+    Audit.reorder(nil).joins('JOIN lookup_keys ON lookup_keys.id = audits.auditable_id')
+         .where(:auditable_type => 'LookupKey', :lookup_keys => {:type => 'PuppetclassLookupKey'})
+         .update_all(:auditable_type => 'PuppetclassLookupKey')


Place the . on the previous line, together with the method call receiver.

houndci-bot · 2017-01-16T12:45:11Z

db/migrate/20170109115157_fix_lookup_key_auditable_type.rb

+         .update_all(:auditable_type => 'VariableLookupKey')
+
+    Audit.reorder(nil).joins('JOIN lookup_keys ON lookup_keys.id = audits.auditable_id')
+         .where(:auditable_type => 'LookupKey', :lookup_keys => {:type => 'PuppetclassLookupKey'})


Place the . on the previous line, together with the method call receiver.

houndci-bot · 2017-01-16T12:45:11Z

db/migrate/20170109115157_fix_lookup_key_auditable_type.rb

+  def up
+    Audit.reorder(nil).joins('JOIN lookup_keys ON lookup_keys.id = audits.auditable_id')
+         .where(:auditable_type => 'LookupKey', :lookup_keys => {:type => 'VariableLookupKey'})
+         .update_all(:auditable_type => 'VariableLookupKey')


Place the . on the previous line, together with the method call receiver.

houndci-bot · 2017-01-16T12:45:11Z

db/migrate/20170109115157_fix_lookup_key_auditable_type.rb

+class FixLookupKeyAuditableType < ActiveRecord::Migration
+  def up
+    Audit.reorder(nil).joins('JOIN lookup_keys ON lookup_keys.id = audits.auditable_id')
+         .where(:auditable_type => 'LookupKey', :lookup_keys => {:type => 'VariableLookupKey'})


Place the . on the previous line, together with the method call receiver.

orrabin · 2017-01-16T12:46:02Z

db/migrate/20170109115157_fix_lookup_key_auditable_type.rb

+
+    Audit.reorder(nil).joins('JOIN lookup_keys ON lookup_keys.id = audits.auditable_id')
+         .where(:auditable_type => 'LookupKey', :lookup_keys => {:type => 'PuppetclassLookupKey'})
+         .update_all(:auditable_type => 'PuppetclassLookupKey')


tried to get the two queries into one with update_all('auditable_type = lookup_keys.type') but: rails/rails#13496

orrabin · 2017-01-16T12:46:54Z

app/helpers/audits_helper.rb

@@ -136,6 +136,10 @@ def audited_type(audit)
                    "#{audit.associated_type || 'Global'}-#{type_name}"
                  when 'LookupKey'
                    'Smart Variable'


Keeping this because of old audits that don't have a connected lookup key and therefore I can't determine their type.

tbrisker · 2017-01-24T09:07:58Z

app/models/concerns/audit_extensions.rb

-    self.auditable_type = auditable.type  if auditable_type == "Taxonomy" && auditable
-    self.associated_type = auditable.type if auditable_type == "Taxonomy" && auditable
+    self.auditable_type = auditable.type if auditable && ["Taxonomy", "LookupKey"].include?(auditable_type)
+    self.associated_type = auditable.type if auditable && ["Taxonomy", "LookupKey"].include?(auditable_type)


after some debugging, looks like while this condition is always false, actually a side effect of the auditable call is what caused this to work correctly, possibly because it led to a reload of the auditable object. Otherwise the save fails. Some preliminary testing indicated that moving this callback from after_validation to before_save allows removing the second auditable call and saving the object correctly, but this needs to be checked to make sure this doesn't cause other things to break.

This line should be changed anyway for: http://projects.theforeman.org/issues/19354

One thing I could think of that could break is the callbacks we have today on before_save that might have counted on auditable being accessed in this callback.
I moved the callback and did a few create/update/delete that showed up correctly in the audits and didn't see anything break right away.

ShimShtein

Looks good, a few suggestions though.

ShimShtein · 2017-04-23T12:42:42Z

app/models/concerns/audit_extensions.rb

@@ -95,8 +95,8 @@ def fix_auditable_type
    # STI Host class should use the stub module instead of Host::Base
    self.auditable_type = "Host"          if auditable_type =~  /Host::/
    self.associated_type = "Host"         if associated_type =~ /Host::/
-    self.auditable_type = auditable.type  if auditable_type == "Taxonomy" && auditable
-    self.associated_type = auditable.type if auditable_type == "Taxonomy" && auditable
+    self.auditable_type = auditable.type if auditable && ["Taxonomy", "LookupKey"].include?(auditable_type)


You can drop auditable &&, if you fix two issues:

Remove duplicate deletion from puppetclass_importer, as you have already fixed this in b5f207d

Add default value to :type column in :lookup_key factory

&& auditable was already in the code, it's a good suggestion to refactor it but could lead to a lot of changes in unrelated tests and might affect taxonomy as well so I'd rather have it in a separate pr.

I'll move it to the end to avoid calling auditable again for anything but taxonomy and lookup keys.

ShimShtein · 2017-04-23T12:49:30Z

app/models/concerns/audit_extensions.rb

-    self.auditable_type = auditable.type  if auditable_type == "Taxonomy" && auditable
-    self.associated_type = auditable.type if auditable_type == "Taxonomy" && auditable
+    self.auditable_type = auditable.type if auditable && ["Taxonomy", "LookupKey"].include?(auditable_type)
+    self.associated_type = auditable.type if auditable && ["Taxonomy", "LookupKey"].include?(auditable_type)


This line looks a bit odd - you are setting :associated field to the type of :audited object.
It's unrelated to this PR, let's track it as a separate bug here.

ShimShtein · 2017-07-11T17:46:41Z

👍 ACK. Works as expected, no more comments.

orrabin · 2017-07-12T08:35:50Z

@tbrisker all your comments are now addressed.

tbrisker · 2017-07-12T09:03:58Z

Thanks @orrabin !

theforeman-bot added Needs testing Not yet reviewed labels Jan 9, 2017

tbrisker suggested changes Jan 15, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Jan 15, 2017

orrabin force-pushed the 17918 branch from b5f5e8f to 47c9ecb Compare January 16, 2017 06:20

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jan 16, 2017

tbrisker suggested changes Jan 16, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Jan 16, 2017

orrabin force-pushed the 17918 branch from 47c9ecb to 7cf6749 Compare January 16, 2017 09:22

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jan 16, 2017

orrabin force-pushed the 17918 branch from 7cf6749 to 82e8024 Compare January 16, 2017 12:45

houndci-bot reviewed Jan 16, 2017

View reviewed changes

orrabin commented Jan 16, 2017

View reviewed changes

orrabin force-pushed the 17918 branch 3 times, most recently from b7a63d8 to d2d021d Compare January 22, 2017 13:30

tbrisker added Waiting on contributor and removed Needs re-review labels Jan 22, 2017

tbrisker removed the Needs testing label Jan 22, 2017

orrabin force-pushed the 17918 branch from d2d021d to 227ea99 Compare January 23, 2017 15:36

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jan 23, 2017

tbrisker reviewed Jan 24, 2017

View reviewed changes

tbrisker added Waiting on contributor and removed Needs re-review Needs testing labels Jan 24, 2017

orrabin force-pushed the 17918 branch from 227ea99 to 6e1cdbe Compare April 13, 2017 07:47

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Apr 13, 2017

orrabin added Waiting on contributor and removed Needs re-review Needs testing labels Apr 13, 2017

ShimShtein reviewed Apr 23, 2017

View reviewed changes

orrabin force-pushed the 17918 branch from 6e1cdbe to 3feed4b Compare June 27, 2017 09:07

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jun 27, 2017

orrabin force-pushed the 17918 branch from 3feed4b to 7a3f9e9 Compare June 27, 2017 09:09

Fixes #17918,#19354 - Smart class parameters appear in audits

bfc3979

orrabin force-pushed the 17918 branch from 7a3f9e9 to bfc3979 Compare July 9, 2017 13:47

tbrisker approved these changes Jul 12, 2017

View reviewed changes

tbrisker merged commit 364c520 into theforeman:develop Jul 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #17918 - Smart class parameters should appear in audits #4165

Fixes #17918 - Smart class parameters should appear in audits #4165

orrabin commented Jan 9, 2017

mention-bot commented Jan 9, 2017

tbrisker left a comment

tbrisker Jan 15, 2017

tbrisker Jan 15, 2017

orrabin commented Jan 16, 2017

tbrisker left a comment

tbrisker Jan 16, 2017

houndci-bot Jan 16, 2017

houndci-bot Jan 16, 2017

houndci-bot Jan 16, 2017

houndci-bot Jan 16, 2017

orrabin Jan 16, 2017

orrabin Jan 16, 2017

tbrisker Jan 24, 2017

orrabin Jun 27, 2017

ShimShtein left a comment

ShimShtein Apr 23, 2017

orrabin Jun 27, 2017

ShimShtein Apr 23, 2017

ShimShtein commented Jul 11, 2017

orrabin commented Jul 12, 2017

tbrisker commented Jul 12, 2017

Fixes #17918 - Smart class parameters should appear in audits #4165

Fixes #17918 - Smart class parameters should appear in audits #4165

Conversation

orrabin commented Jan 9, 2017

mention-bot commented Jan 9, 2017

tbrisker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orrabin commented Jan 16, 2017

tbrisker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShimShtein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShimShtein commented Jul 11, 2017

orrabin commented Jul 12, 2017

tbrisker commented Jul 12, 2017