Custom fonticons and colors in the CustomButton(Set) model #39

skateman · 2017-07-11T12:22:37Z

This migration converts the old custom button icons to the new format where we specify a button class and a color instead of a number.

https://www.pivotaltracker.com/story/show/147779325

jameswnl · 2017-07-11T15:12:48Z

db/migrate/20170710132935_custom_button_to_classes.rb

+    serialize :options
+  end
+
+  def up


how about an enclosing say_with_time?

jameswnl · 2017-07-11T15:15:36Z

db/migrate/20170710132935_custom_button_to_classes.rb

+      next unless btn.options[:button_image]
+      icon = {}
+
+      case btn.options[:button_image].to_i


(Minor) Is to_i needed?

jameswnl · 2017-07-11T15:20:44Z

spec/migrations/20170710132935_custom_button_to_classes_spec.rb

+      custom_button.reload
+
+      expect(custom_button.options).to have_key(:button_image_class)
+      expect(custom_button.options).not_to have_key(:button_image)


how about add a case (another button) where you will have both :button_image_class and :button_image_color?

jameswnl · 2017-07-11T15:21:29Z

spec/migrations/20170710132935_custom_button_to_classes_spec.rb

+      expect(custom_button.options).to have_key(:button_image)
+      expect(custom_button.options).not_to have_key(:button_image_class)
+      expect(custom_button.options).not_to have_key(:button_image_color)
+    end


add another button case which has no : button_image_color?

@jameswnl button_image_color is optional

skateman · 2017-07-13T14:22:20Z

Merging this will probably break the tests in the ui-classic repo, but this should fix it: ManageIQ/manageiq-ui-classic#1685

skateman · 2017-07-14T11:31:08Z

@jameswnl can you take a look please?

lpichler · 2017-07-14T12:43:12Z

db/migrate/20170710132935_custom_button_to_classes.rb

@@ -0,0 +1,144 @@
+class CustomButtonToClasses < ActiveRecord::Migration[5.0]


maybe more explanatory naming?
something like:
migrate_<something>_to_<something>

migrate -> implicit enough from the folder name
everything else is there 😉

To @lpichler's point, almost all of the migrations start with a verb, so ConvertCustomButtonToClasses?

jameswnl · 2017-07-14T15:27:50Z

spec/migrations/20170710132935_custom_button_to_classes_spec.rb

+
+  context 'CustomButton' do
+    let(:params) { custom_button.options }
+    let!(:custom_button) { migration_stub(:CustomButton).create!(create_params) }


you probably intended to use custom_button_stub which is already defined for this stub, right?

jameswnl · 2017-07-14T15:46:27Z

spec/migrations/20170710132935_custom_button_to_classes_spec.rb

+    let!(:custom_button) { migration_stub(:CustomButton).create!(create_params) }
+
+    migration_context :up do
+      let(:create_params) { {:options => {:button_image => 1} } }


for this {:button_image => 1}, can we check for expect(params).to have_key(:button_color)?

Can we add one more case {:button_image => 6}, which we would check for expect(params).not_to have_key(:button_color)?

jameswnl · 2017-07-14T15:48:58Z

spec/migrations/20170710132935_custom_button_to_classes_spec.rb

+  let(:custom_button_stub) { migration_stub(:CustomButton) }
+  let(:miq_set_stub) { migration_stub(:MiqSet) }
+
+  shared_examples 'migrate up' do


Nice to use shared_example. However IMHO, in this case, I'd rather not to use it because

the lets are not easy to follow, plus not saving too much code

you won't be able to test the 2nd cases that I suggested.

jameswnl · 2017-07-14T15:49:39Z

spec/migrations/20170710132935_custom_button_to_classes_spec.rb

+
+  context 'CustomButtonSet' do
+    let(:params) { custom_button.set_data }
+    let!(:custom_button) { migration_stub(:MiqSet).create!(create_params) }


same here, use miq_set_stub

jameswnl · 2017-07-14T15:50:40Z

spec/migrations/20170710132935_custom_button_to_classes_spec.rb

+    let!(:custom_button) { migration_stub(:MiqSet).create!(create_params) }
+
+    migration_context :up do
+      let(:create_params) { {:set_data => {:button_image => 1}, :set_type => 'CustomButtonSet'} }


similarly here, nice to test for the case of {:button_image => 6}

skateman · 2017-07-16T11:13:57Z

@jameswnl adjusted specs!

jameswnl · 2017-07-17T14:54:47Z

spec/migrations/20170710132935_custom_button_to_classes_spec.rb

+          custom_button.reload
+
+          expect(params).to have_key(:button_icon)
+          expect(params).not_to have_key(:button_color)


I'd keep expect(params).not_to have_key(:button_image) here as well

How about this one?
You can also replace these key checking lines by 1 line
expect(params.keys).to match_array([:button_icon])

jameswnl · 2017-07-17T14:55:28Z

spec/migrations/20170710132935_custom_button_to_classes_spec.rb

+        migrate
+        custom_button.reload
+
+        expect(params).to have_key(:button_icon)


I'd add expect(params).to have_key(:button_color) here

jameswnl · 2017-07-17T14:56:31Z

spec/migrations/20170710132935_custom_button_to_classes_spec.rb

+        migrate
+        custom_button.reload
+
+        expect(params).to have_key(:button_icon)


I'd add expect(params).to have_key(:button_color)

jameswnl · 2017-07-17T14:58:35Z

spec/migrations/20170710132935_custom_button_to_classes_spec.rb

+          custom_button.reload
+
+          expect(params).to have_key(:button_icon)
+          expect(params).not_to have_key(:button_color)


I'd add expect(params).not_to have_key(:button_image) here.
So it'll be very obvious to readers the difference between the 2 cases. Will be helpful to readers like me.

and this one...

jameswnl

Pretty close. a couple more suggestions

skateman · 2017-07-18T14:36:09Z

@jameswnl all fixed

jameswnl

almost there...

And about time to get @Fryguy 's attention...

jameswnl · 2017-07-18T15:06:35Z

spec/migrations/20170710132935_convert_custom_button_to_classes_spec.rb

+        expect(params).not_to have_key(:button_image)
+      end
+
+      context 'button set with default color' do


similarly here with the context ...

jameswnl · 2017-07-18T15:07:21Z

spec/migrations/20170710132935_convert_custom_button_to_classes_spec.rb

+        expect(params).not_to have_key(:button_image)
+      end
+
+      context 'button with default color' do


context is good for grouping multiple specs. So here is only 1 spec, may get flagged by other reviewers (I am feeling like a to ask more ...)
you can move the context text into the it statement.

skateman · 2017-07-18T15:21:21Z

@jameswnl I have a feeling that you haven't read http://www.betterspecs.org/ 😉, if I drop the context, I can't use the let(:create_params) and I don't really want to give up that 😉

jameswnl · 2017-07-18T16:02:41Z

@skateman 😭 I admit that I haven't read that plus a ton of others.

Yeah, and according to that and a bunch other docs, you would want to start your context with [when|with|without].
I also found someone in that particular point arguing against the 1 it in 1 context. But it's pretty common that people are very opinionated on spec styles (even arguing between When vs when). My goal is to keep specs clean and easy to read and I try to avoid iron-fisting those things.

So I'll LGTM if you want to keep it that way.

miq-bot · 2017-07-19T06:39:30Z

Checked commit skateman@b641025 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

skateman · 2017-07-19T06:39:43Z

@jameswnl added the suggested when. I was always planning to write some spec guidelines (at least for ui-classic), but never had the time and I got stuck with reviewing everyone else's tests 😉

skateman · 2017-07-21T17:32:59Z

@Fryguy can you please merge this?

dclarizio · 2017-07-24T15:40:50Z

@miq-bot assign @Fryguy

dclarizio · 2017-07-24T15:41:38Z

@Fryguy @chessbyte can we get this reviewed/merged soon? The ui-classic feature PR is dependent on this

dclarizio · 2017-07-26T20:04:33Z

@Fryguy @chessbyte bump!

skateman force-pushed the custom-button-fonticons branch from aca0836 to 9349223 Compare July 11, 2017 13:05

skateman changed the title ~~[WIP] Custom fonticons and colors in the CustomButton model~~ Custom fonticons and colors in the CustomButton model Jul 11, 2017

skateman changed the title ~~Custom fonticons and colors in the CustomButton model~~ [WIP] Custom fonticons and colors in the CustomButton model Jul 11, 2017

miq-bot added the wip label Jul 11, 2017

skateman force-pushed the custom-button-fonticons branch from 9349223 to d06e6e0 Compare July 11, 2017 14:19

jameswnl suggested changes Jul 11, 2017

View reviewed changes

skateman force-pushed the custom-button-fonticons branch from d06e6e0 to 2d0a7f2 Compare July 13, 2017 08:43

skateman changed the title ~~[WIP] Custom fonticons and colors in the CustomButton model~~ [WIP] Custom fonticons and colors in the CustomButton(Set) model Jul 13, 2017

skateman mentioned this pull request Jul 13, 2017

Introducing custom and colored fonticons for CustomButtons ManageIQ/manageiq-ui-classic#1685

Merged

skateman changed the title ~~[WIP] Custom fonticons and colors in the CustomButton(Set) model~~ Custom fonticons and colors in the CustomButton(Set) model Jul 13, 2017

miq-bot removed the wip label Jul 13, 2017

lpichler reviewed Jul 14, 2017

View reviewed changes

jameswnl suggested changes Jul 14, 2017

View reviewed changes

skateman force-pushed the custom-button-fonticons branch from 2d0a7f2 to 474a948 Compare July 16, 2017 11:13

jameswnl reviewed Jul 17, 2017

View reviewed changes

jameswnl suggested changes Jul 17, 2017

View reviewed changes

skateman force-pushed the custom-button-fonticons branch 2 times, most recently from a5894a5 to 3384922 Compare July 18, 2017 08:03

jameswnl suggested changes Jul 18, 2017

View reviewed changes

Custom fonticons and colors in the CustomButton(Set) model

b641025

skateman force-pushed the custom-button-fonticons branch from 3384922 to b641025 Compare July 19, 2017 06:38

skateman mentioned this pull request Jul 22, 2017

Display colored fonticons before custom buttons ManageIQ/manageiq-ui-service#851

Merged

miq-bot assigned Fryguy Jul 24, 2017

chessbyte added enhancement data and removed enhancement labels Jul 25, 2017

Fryguy merged commit e3543ba into ManageIQ:master Jul 26, 2017

Fryguy added this to the Sprint 66 Ending Aug 7, 2017 milestone Jul 26, 2017

		@@ -0,0 +1,144 @@
		class CustomButtonToClasses < ActiveRecord::Migration[5.0]

Custom fonticons and colors in the CustomButton(Set) model #39

Custom fonticons and colors in the CustomButton(Set) model #39

Conversation

skateman commented Jul 11, 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

Choose a reason for hiding this comment

skateman commented Jul 13, 2017

skateman commented Jul 14, 2017

lpichler Jul 14, 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

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

skateman commented Jul 16, 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

jameswnl left a comment

Choose a reason for hiding this comment

skateman commented Jul 18, 2017

jameswnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skateman commented Jul 18, 2017

jameswnl commented Jul 18, 2017

miq-bot commented Jul 19, 2017

skateman commented Jul 19, 2017

skateman commented Jul 21, 2017

dclarizio commented Jul 24, 2017

dclarizio commented Jul 24, 2017

dclarizio commented Jul 26, 2017

skateman commented Jul 11, 2017 •

edited

Loading

lpichler Jul 14, 2017 •

edited

Loading