Migrate existing dialog field association data to use new relationship #80

d-m-u · 2017-09-27T14:02:36Z

The planned targeted auto-refresh enhancement to dialog fields requires the dialog fields to be able to be associated to one another. This migration takes our old system of relating dialog fields by order-dependent auto_refresh and trigger_auto_refresh fields and updates them to use the recently added DialogFieldAssociation table. It's worth noting that this may not upgrade all existing dialog associations correctly and that it may be necessary to manually check and possibly fix some of the associations not caught by this migration.

Fryguy · 2017-10-02T20:48:26Z

db/migrate/20170927135007_migrate_dialog_field_associations_to_use_new_relationship.rb

@@ -0,0 +1,31 @@
+class MigrateDialogFieldAssociationsToUseNewRelationship < ActiveRecord::Migration[5.0]
+  Rails.application.eager_load!


Fryguy · 2017-10-02T20:49:11Z

db/migrate/20170927135007_migrate_dialog_field_associations_to_use_new_relationship.rb

+  Rails.application.eager_load!
+
+  def up
+    Dialog.all.each do |dialog|


Create a stub model, as is done in other data migrations, for Dialog and DialogFieldAssociation

Fryguy · 2017-10-02T20:52:22Z

db/migrate/20170927135007_migrate_dialog_field_associations_to_use_new_relationship.rb

+  def up
+    Dialog.all.each do |dialog|
+      fields = dialog.dialog_fields
+      dialog_fields_with_associations = fields.select { |df| !df.auto_refresh.nil? || !df.trigger_auto_refresh.nil? }.sort { |n| n.position * -1 }


!df.auto_refresh.nil? || !df.trigger_auto_refresh.nil?

simplifies to

df.auto_refresh || df.trigger_auto_refresh

... Also, should those be auto_refresh? and trigger_auto_refresh? (with trailing ?)

.sort { |n| n.position * -1 }

simplifies to

.sort { |n| -n.position }

Fryguy · 2017-10-02T20:52:54Z

db/migrate/20170927135007_migrate_dialog_field_associations_to_use_new_relationship.rb

+
+  def up
+    Dialog.all.each do |dialog|
+      fields = dialog.dialog_fields


you might want an includes in the original query or this is going to be a nasty N+1

Fryguy · 2017-10-02T20:53:48Z

db/migrate/20170927135007_migrate_dialog_field_associations_to_use_new_relationship.rb

+    Dialog.all.each do |dialog|
+      fields = dialog.dialog_fields
+      dialog_fields_with_associations = fields.select { |df| !df.auto_refresh.nil? || !df.trigger_auto_refresh.nil? }.sort { |n| n.position * -1 }
+      triggers = dialog_fields_with_associations.select { |df| df.trigger_auto_refresh == true }


if trigger_auto_refresh is a boolean column, this simplifies to

.select(&:trigger_auto_refresh)

(same with the next line)

Fryguy · 2017-10-02T20:55:02Z

db/migrate/20170927135007_migrate_dialog_field_associations_to_use_new_relationship.rb

+      triggers = dialog_fields_with_associations.select { |df| df.trigger_auto_refresh == true }
+      responders = dialog_fields_with_associations.select { |df| df.auto_refresh == true }
+      triggers.each_with_index do |t, index|
+        responder_fields_for_t = if !triggers[index + 1].nil?


Prefer coding in the positive as opposed to ! nil? because double-negative are hard for people to reason about. So,

responder_fields_for_t = if triggers[index + 1] responders.select { |r| r.position ....

Fryguy · 2017-10-02T20:55:39Z

Don't forget the specs 😄 All data migrations require specs.

d-m-u · 2017-10-05T18:14:43Z

@eclarizio just working on the testing for this one now

d-m-u · 2017-10-12T17:50:05Z

@miq-bot assign @eclarizio

eclarizio · 2017-10-12T19:23:55Z

I'm guessing you meant to add me as a reviewer, since I don't really have any powers as an assignee 😛

@miq-bot assign @Fryguy

@Fryguy Looking at the miq-bot repo though, can we even set a reviewer with miq-bot?

Fryguy · 2017-10-26T16:03:18Z

.../migrations/20170927135007_migrate_dialog_field_associations_to_use_new_relationship_spec.rb

+    before(:each) do
+      dialog_group_stub.create!(:id => 2, :dialog_tab_id => 8, :position => 7)
+      dialog_tab_stub.create!(:id => 8, :dialog_id => 10, :position => 4)
+      dialog_stub.create!(:id => 10)


Do not use manual ids because it bypasses proper region testing.

Instead create the record, let ActiveRecord create a proper id, then use that id after the fact to update the associations.

Fryguy · 2017-10-26T16:19:56Z

.../migrations/20170927135007_migrate_dialog_field_associations_to_use_new_relationship_spec.rb

+  let(:dialog_field_association_stub) { migration_stub(:DialogFieldAssociation) }
+
+  migration_context :up do
+    before(:each) do


:each is redundant, so you can remove it.

Fryguy · 2017-10-26T16:22:09Z

db/migrate/20170927135007_migrate_dialog_field_associations_to_use_new_relationship.rb

+      index = field_position + dialog_group_position * 100 + dialog_tab_position * 10_000
+      positions << {:id => f.id, :position => index}
+    end
+    positions


prefer .collect over the

x = [] foo.each { x << y } x

pattern.

x = foo.collect { y }

Fryguy · 2017-10-26T16:25:36Z

db/migrate/20170927135007_migrate_dialog_field_associations_to_use_new_relationship.rb

+      field_position = f.position
+      dialog_group_position = DialogGroup.find(f.dialog_group_id).position
+      dialog_tab_position = DialogTab.find(DialogGroup.find(f.dialog_group_id).dialog_tab_id).position
+      index = field_position + dialog_group_position * 100 + dialog_tab_position * 10_000


Why these numbers? Are they just arbitrarily for segmenting? I only ask because I remember a customer with over 100 fields on a form, and so this could cause issues. Even so, since you are updating the position, I still think it's weird to segment and then leave it that way...I would expect something to post-process the "weird" numbers, and put them into "normal" numbers by sorting and mapping the indexes or something.

Fryguy · 2017-10-26T16:26:05Z

db/migrate/20170927135007_migrate_dialog_field_associations_to_use_new_relationship.rb

+
+  def responder_range(trigger_min, trigger_max)
+    min = trigger_min[:position] + 1
+    max = trigger_max.present? ? trigger_max[:position] - 1 : 10_000_000


Also curious about this 10 million number.

Fryguy · 2017-10-26T16:26:51Z

db/migrate/20170927135007_migrate_dialog_field_associations_to_use_new_relationship.rb

+  end
+
+  def down
+    DialogFieldAssociation.delete_all


Will the previous migration just drop the table? If so, then there's no need for this at all. (You could just put a comment stating that fact)

Fryguy · 2017-10-26T16:27:40Z

db/migrate/20170927135007_migrate_dialog_field_associations_to_use_new_relationship.rb

+  end
+
+  def dialog_fields_with_associations_by_dialog(dialog_fields_with_associations)
+    dialog_fields_with_associations.group_by { |df| Dialog.find(DialogTab.find(DialogGroup.find(DialogField.find(df.id).dialog_group_id).dialog_tab_id).dialog_id) }


This looks like an N+1 nightmare. There are much better approaches for querying all the data, then mixing and matching (we do something very similar with the RelationshipMixin for child objects. I can sit with you and show you.

Fryguy · 2017-10-26T16:28:34Z

db/migrate/20170927135007_migrate_dialog_field_associations_to_use_new_relationship.rb

+    dialog_fields.each do |f|
+      field_position = f.position
+      dialog_group_position = DialogGroup.find(f.dialog_group_id).position
+      dialog_tab_position = DialogTab.find(DialogGroup.find(f.dialog_group_id).dialog_tab_id).position


Also an N+1 issue (on both of these lines)

d-m-u · 2017-10-30T19:33:26Z

@gmcculloug can you take a quick looksee at this, 'specially the specs, again please?

gmcculloug · 2017-10-30T19:53:20Z

db/migrate/20170927135007_migrate_dialog_field_associations_to_use_new_relationship.rb

+      .where(:auto_refresh => true)
+      .or(DialogField.where(:trigger_auto_refresh => true))
+      .includes(:dialog_group => {:dialog_tab => :dialog})
+      .group_by { |f| f.dialog_group.dialog_tab.dialog }


@Fryguy had the suggest to protect against this relationship chain hitting a nil here and I would agree. If we use try and allow the fields to get grouped together until nil we can throw those fields away before processing.

Fryguy · 2017-10-30T20:51:33Z

.../migrations/20170927135007_migrate_dialog_field_associations_to_use_new_relationship_spec.rb

+
+  migration_context :down do
+    it "should delete dialog field associations" do
+      migrate


This should create a dialog_field_association record before the migrate, otherwise you are testing nothing.

Fryguy · 2017-10-30T20:52:02Z

minor problem with the down migration spec but otherwise looks good to me 👍

miq-bot · 2017-10-30T21:14:36Z

Checked commit d-m-u@a860e35 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

d-m-u changed the title ~~Migrate existing dialog field association data to use new relationship~~ [WIP] Migrate existing dialog field association data to use new relationship Sep 27, 2017

miq-bot added the wip label Sep 27, 2017

d-m-u force-pushed the migrate_existing_dialog_field_associations_to_new_association_relationship branch from 9a6e96c to fb00efd Compare September 27, 2017 15:08

d-m-u changed the title ~~[WIP] Migrate existing dialog field association data to use new relationship~~ Migrate existing dialog field association data to use new relationship Sep 27, 2017

d-m-u changed the title ~~Migrate existing dialog field association data to use new relationship~~ [WIP] Migrate existing dialog field association data to use new relationship Sep 27, 2017

d-m-u force-pushed the migrate_existing_dialog_field_associations_to_new_association_relationship branch from fb00efd to a3040d0 Compare September 27, 2017 17:43

Fryguy reviewed Oct 2, 2017

View reviewed changes

Fryguy added the data label Oct 2, 2017

Fryguy self-assigned this Oct 2, 2017

d-m-u force-pushed the migrate_existing_dialog_field_associations_to_new_association_relationship branch 3 times, most recently from 108a22a to ca220ba Compare October 3, 2017 18:07

miq-bot assigned eclarizio and unassigned Fryguy Oct 12, 2017

miq-bot assigned Fryguy and unassigned eclarizio Oct 12, 2017

d-m-u force-pushed the migrate_existing_dialog_field_associations_to_new_association_relationship branch from ca220ba to 33f510e Compare October 13, 2017 16:41

d-m-u changed the title ~~[WIP] Migrate existing dialog field association data to use new relationship~~ Migrate existing dialog field association data to use new relationship Oct 13, 2017

miq-bot removed the wip label Oct 13, 2017

d-m-u force-pushed the migrate_existing_dialog_field_associations_to_new_association_relationship branch 2 times, most recently from a244cf4 to 406045f Compare October 16, 2017 14:06

d-m-u changed the title ~~[WIP] Migrate existing dialog field association data to use new relationship~~ Migrate existing dialog field association data to use new relationship Oct 25, 2017

miq-bot removed the wip label Oct 25, 2017

Fryguy reviewed Oct 26, 2017

View reviewed changes

d-m-u force-pushed the migrate_existing_dialog_field_associations_to_new_association_relationship branch 6 times, most recently from 78e5e20 to 3687054 Compare October 30, 2017 18:50

gmcculloug reviewed Oct 30, 2017

View reviewed changes

d-m-u force-pushed the migrate_existing_dialog_field_associations_to_new_association_relationship branch 2 times, most recently from cbe0bd9 to 73f524a Compare October 30, 2017 20:33

Fryguy reviewed Oct 30, 2017

View reviewed changes

d-m-u force-pushed the migrate_existing_dialog_field_associations_to_new_association_relationship branch 2 times, most recently from e41ebc4 to 035c632 Compare October 30, 2017 21:04

Migrate existing dialog field association data to use new relationship

a860e35

d-m-u force-pushed the migrate_existing_dialog_field_associations_to_new_association_relationship branch from 035c632 to a860e35 Compare October 30, 2017 21:09

gmcculloug approved these changes Oct 30, 2017

View reviewed changes

chessbyte merged commit ee290b8 into ManageIQ:master Oct 30, 2017

chessbyte added this to the Sprint 72 Ending Oct 30, 2017 milestone Oct 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate existing dialog field association data to use new relationship #80

Migrate existing dialog field association data to use new relationship #80

d-m-u commented Sep 27, 2017 •

edited

Loading

Fryguy Oct 2, 2017

Fryguy Oct 2, 2017

Fryguy Oct 2, 2017 •

edited

Loading

Fryguy Oct 2, 2017

Fryguy Oct 2, 2017

Fryguy Oct 2, 2017

Fryguy commented Oct 2, 2017

d-m-u commented Oct 5, 2017

d-m-u commented Oct 12, 2017 •

edited

Loading

eclarizio commented Oct 12, 2017

Fryguy Oct 26, 2017

Fryguy Oct 26, 2017

Fryguy Oct 26, 2017

Fryguy Oct 26, 2017

Fryguy Oct 26, 2017

Fryguy Oct 26, 2017

Fryguy Oct 26, 2017

Fryguy Oct 26, 2017

Fryguy Oct 26, 2017

d-m-u commented Oct 30, 2017

gmcculloug Oct 30, 2017

Fryguy Oct 30, 2017

Fryguy commented Oct 30, 2017

miq-bot commented Oct 30, 2017

		@@ -0,0 +1,31 @@
		class MigrateDialogFieldAssociationsToUseNewRelationship < ActiveRecord::Migration[5.0]
		Rails.application.eager_load!

Migrate existing dialog field association data to use new relationship #80

Migrate existing dialog field association data to use new relationship #80

Conversation

d-m-u commented Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Oct 2, 2017 • edited Loading

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

Fryguy commented Oct 2, 2017

d-m-u commented Oct 5, 2017

d-m-u commented Oct 12, 2017 • edited Loading

eclarizio commented Oct 12, 2017

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

d-m-u commented Oct 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Oct 30, 2017

miq-bot commented Oct 30, 2017

d-m-u commented Sep 27, 2017 •

edited

Loading

Fryguy Oct 2, 2017 •

edited

Loading

d-m-u commented Oct 12, 2017 •

edited

Loading