Add validations for MiqWidgetSet #20890

lpichler · 2020-12-11T15:02:00Z

In order to add MiqWidgetSet API Endpoint following validations and update on related objects have been extracted from UI dashboard controller:

Name cannot contain "|"
~~Name cannot be changed during UPDATE~~ (only API)
Unique description inside group
At least one widget in set_data
Widgets in set_data have to exist
group_id has to be present for non-read_only
Cannot delete readonly widget set

PR also adds initializaation of set_data and
methods update_membersanddelete_from_dashboard_order, add_to_dashboard_order `

@miq-bot add_label refactoring

Links

MiqWidgetSet API Endpoint

Fryguy · 2021-01-04T19:49:17Z

app/models/miq_widget_set.rb

  before_save    :keep_group_when_saving
+  after_save     :update_members
+
+  validates :group_id, :presence => true, :unless => ->(x) { x.read_only? }


Think you can do

Suggested change

validates :group_id, :presence => true, :unless => ->(x) { x.read_only? }

validates :group_id, :presence => true, :unless => :read_only?

good call jason.

Also, thanks Libor for validating group_id and not group - much better for performance in production but can act strange in development with factories

Fryguy · 2021-01-04T19:51:59Z

app/models/miq_widget_set.rb

+
+    if description_changed? && MiqWidgetSet.exists?(search_params)
+      errors.add(:error, "Description (Tab Title) must be unique for this group")
+    end


This feels like a regular uniqueness clause should work when added with a scope on owner_id...

Something like...

validates :description, :uniqueness => {:scope => :owner_id}

good call.

I do notice that this only really does something on description changed
so maybe

Suggested change

end

validates :description, :uniqueness_when_changed => {:scope => :owner_id}

Fryguy · 2021-01-04T19:53:13Z

@kbrock Can you please review? I'm not sure on the rest, and I think you've been working in this space recently.

kbrock

this looks nice - only a few nits.

my only complaints are code that Libor did not introduce (using sets, serializing the settings data instead of using habtm/through)

kbrock · 2021-01-06T04:31:49Z

app/models/miq_widget_set.rb

+  def update_members
+    replace_children(Array(set_data_widgets))
+    current_user = User.current_user
+    members.each { |w| w.create_initial_content_for_user(current_user.userid) } if current_user # Generate content if not there


For an update where the widget is already all set for the user, will create_initial_content_for_user() modify the member and cause an unnecessary save? or is it smart enough to not change it.

I never trust serialized values changed? values in rails 5.x (or maybe this wasn't fixed until 6.1)

For an update where the widget is already all set for the user, will create_initial_content_for_user() modify the member and cause an unnecessary save? or is it smart enough to not change it.

I never trust serialized values changed? values in rails 5.x (or maybe this wasn't fixed until 6.1)

create_initial_content_for_user contains report generation and it doesn't matter if we are re-generating it with same members - content of report generation could be different independently on members.

and replace_children(Array(set_data_widgets)) is not needed execute when
members.map(&:id) == set_data_widgets.ids so I fixed it.

I never trust serialized values changed? values in rails 5.x (or maybe this wasn't fixed until 6.1)

I am not sure how do you mean this, I don't see usage serialize attributes here.

thanks

from the method names I had assumed it did nothing if called multiple times. But instead it looks like it will change the data every time.

Do we really want to update the set data automatically on every save?

kbrock · 2021-01-06T04:33:08Z

app/models/miq_widget_set.rb

  before_save    :keep_group_when_saving
+  after_save     :update_members
+
+  validates :group_id, :presence => true, :unless => ->(x) { x.read_only? }


good call jason.

Also, thanks Libor for validating group_id and not group - much better for performance in production but can act strange in development with factories

kbrock · 2021-01-06T04:36:46Z

app/models/miq_widget_set.rb

+
+    if description_changed? && MiqWidgetSet.exists?(search_params)
+      errors.add(:error, "Description (Tab Title) must be unique for this group")
+    end


good call.

I do notice that this only really does something on description changed
so maybe

Suggested change

end

validates :description, :uniqueness_when_changed => {:scope => :owner_id}

kbrock · 2021-01-06T04:39:17Z

app/models/miq_widget_set.rb

+  def add_to_dashboard_order
+    return unless group_id
+
+    group = MiqGroup.find(group_id)


can this use an association?

belongs_to :group, :class_name => 'MiqGroup'

kbrock · 2021-01-06T04:41:31Z

app/models/miq_widget_set.rb

+    errors.add(:base, _("Unable to delete read-only WidgetSet")) if read_only?
+    throw :abort unless errors[:base].empty?


is this trying to catch errors not added by this class?

or should this all go into a block?

Suggested change

errors.add(:base, _("Unable to delete read-only WidgetSet")) if read_only?

throw :abort unless errors[:base].empty?

if read_only?

errors.add(:base, _("Unable to delete read-only WidgetSet"))

throw :abort

end

it looks that throw :abort is not needed here (I don't see errors.add(:base) on other places)

lpichler · 2021-01-07T19:07:08Z

@Fryguy, @kbrock thanks for suggestions - I added them - it can be re-reviewed

lpichler · 2021-01-08T15:41:34Z

I also added spec for validations and ManageIQ/manageiq-ui-classic#7561 is needed to merge first otherwise it will break UI.

kbrock

I am a little nervous about the implications of always having a group set.
I guess the waters get muddy because most users have a single current_group so not setting it probably worked well in the past.

If we need to do this, I like having group_id auto set in a before_validator hook rather than expect the caller to set this. (is it always set? or only for particular owners?)

kbrock · 2021-01-13T16:43:36Z

app/models/miq_widget_set.rb

@@ -14,6 +29,31 @@ def self.with_users
    where.not(:userid => nil)
  end

+  def update_members
+    replace_children(Array(set_data_widgets)) if members.map(&:id) != set_data_widgets.ids


A. Do we want to make this order independent, or does order matter?

Suggested change

replace_children(Array(set_data_widgets)) if members.map(&:id) != set_data_widgets.ids

replace_children(Array(set_data_widgets)) if members.map(&:id)&.sort != set_data_widgets.ids.sort

kbrock · 2021-01-13T16:48:20Z

app/models/miq_widget_set.rb

+  def update_members
+    replace_children(Array(set_data_widgets))
+    current_user = User.current_user
+    members.each { |w| w.create_initial_content_for_user(current_user.userid) } if current_user # Generate content if not there


from the method names I had assumed it did nothing if called multiple times. But instead it looks like it will change the data every time.

Do we really want to update the set data automatically on every save?

kbrock · 2021-01-15T01:51:37Z

app/models/miq_group.rb

+  def add_to_dashboard_order(widget_set_id)
+    self.settings ||= {:dashboard_order => []}
+    self.settings[:dashboard_order] ||= []
+    self.settings[:dashboard_order] |= [widget_set_id]


assuming this is the logic that was in the ui.

also thinking this is |= instead of += to fix any duplicate issues

yes it is logic from UI,
suggested change is not needed:

irb(main):004:0> a=[1];a+=[1];b=[2];b|=[2];puts "a:#{a.count} b:#{b.count}"; a:2 b:1

@kbrock

from the method names I had assumed it did nothing if called multiple times. But instead it looks like it will change the data every time.

Do we really want to update the set data automatically on every save?

We are not updating data every time - thanks to this guard condition.

kbrock · 2021-01-15T01:56:16Z

app/models/miq_widget_set.rb

+  def delete_from_dashboard_order
+    return unless group_id
+
+    group = MiqGroup.find(group_id)


is there a reason why group is looked up here but not in add_to_dashboard_order

also, this looks strange instead of having a belongs_to (it may be due to wanting to blow out the cache?)

kbrock · 2021-01-15T01:57:39Z

app/models/miq_widget_set/set_data.rb

+    validate :set_data do
+      errors.add(:set_data, "One widget must be selected(set_data)") if widget_ids.empty?
+
+      filtered_widget_ids = set_data_widgets.pluck(:id)


is there a reason not to use widget_ids here?
is it trying to verify the values in the database?

it looks like those ids are stored in the set_data hash.

maybe you are also validating that these records exist in the database?

yes as I you said because we need to validate whether those ids exist.

kbrock · 2021-01-15T02:05:44Z

spec/factories/miq_widget_set.rb

+    end
+
+    before(:create) do |x|
+      x.group_id = x.owner_id if x.owner_id


not comfortable with this.

Can we do this in a before_validation callback rather than have part of the object creation contract? Which to be honest, the purpose of this PR is to remove the object creation process from the ui-classic and put into the model.

For some reason, this feels like it glosses over an implementation detail and I'm more comfortable having this requirement exposed in the spec or as mentioned before, completely removed.

@Fryguy may feel otherwise.

yes removing thanks to new before_validation

kbrock · 2021-01-15T02:08:45Z

spec/factories/miq_widget_set.rb

@@ -2,5 +2,17 @@
  factory :miq_widget_set do
    sequence(:name)         { |n| "widget_set_#{seq_padded_for_sorting(n)}" }
    sequence(:description)  { |n| "widget_set_#{seq_padded_for_sorting(n)}" }
+
+    trait :set_data_with_one_widget do


not sure what this buys us.
the old code feels the same.

I also like having us add the widgets using the same methods we use to add them in the production app (not specs)

If something changes in the way that add works or a bug is introduced then it could slip through.
Yes, I know this could be said about a lot of factories, but this is directly manipulating a hash with funky workings.

yes removing thanks to new before_validation, thanks

kbrock · 2021-01-15T02:19:59Z

app/models/miq_widget_set.rb

+  end
+
+  def ensure_can_be_destroyed
+    errors.add(:base, _("Unable to delete read-only WidgetSet")) if read_only?


from what I'm reading, I think the raise :abort is necessary.

I added it 👍

kbrock · 2021-01-15T02:20:55Z

app/models/miq_widget_set.rb

@@ -88,6 +128,7 @@ def self.copy_dashboard(source_widget_set, destination_name, destination_descrip
                         :owner_type  => "MiqGroup",
                         :set_type    => source_widget_set.set_type,
                         :set_data    => source_widget_set.set_data,
+                         :group_id    => assign_to_group.id,


think this should come across automatically from owner_id (in the before_validation)

that's right - it is removed 👍

kbrock · 2021-01-15T02:22:30Z

app/models/miq_widget_set/set_data.rb

+    end
+
+    def widget_ids
+      SET_DATA_COLS.map { |x| set_data[x] }.flatten.compact


if this is new code, could you change to use flat_map

If this is existing code that you moved, then it is up to you

Suggested change

SET_DATA_COLS.map { |x| set_data[x] }.flatten.compact

SET_DATA_COLS.flat_map { |x| set_data[x] }.compact

I changed it

miq-bot · 2021-01-20T14:36:02Z

Checked commit lpichler@b203865 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
8 files checked, 0 offenses detected
Everything looks fine. 👍

lpichler · 2021-01-20T14:56:43Z

I am a little nervous about the implications of always having a group set.
I guess the waters get muddy because most users have a single current_group so not setting it probably worked well in the past.

If we need to do this, I like having group_id auto set in a before_validator hook rather than expect the caller to set this. (is it always set? or only for particular owners?)

we have it since #19491 and it is because having same name in different category.
Only seeded items don't have group_id neither owner_id.

lpichler · 2021-01-20T14:58:44Z

@kbrock thanks for you review and I added changes as you suggested 👍

lpichler requested review from agrare, Fryguy, gtanzillo and kbrock as code owners December 11, 2020 15:02

miq-bot added refactoring wip labels Dec 11, 2020

lpichler mentioned this pull request Dec 11, 2020

[WIP] Add WidgetSet endpoint ManageIQ/manageiq-api#968

Closed

2 tasks

lpichler force-pushed the validations_for_miq_widget_set branch 5 times, most recently from 377a955 to 356a1ec Compare December 16, 2020 13:06

lpichler changed the title ~~[WIP] Add validations for MiqWidgetSet~~ Add validations for MiqWidgetSet Jan 4, 2021

miq-bot removed the wip label Jan 4, 2021

Fryguy reviewed Jan 4, 2021

View reviewed changes

Fryguy assigned kbrock Jan 4, 2021

kbrock reviewed Jan 6, 2021

View reviewed changes

lpichler force-pushed the validations_for_miq_widget_set branch 3 times, most recently from 9141a6d to f932252 Compare January 7, 2021 19:00

lpichler changed the title ~~Add validations for MiqWidgetSet~~ [WIP] Add validations for MiqWidgetSet Jan 8, 2021

miq-bot added the wip label Jan 8, 2021

lpichler force-pushed the validations_for_miq_widget_set branch 2 times, most recently from 986f475 to 0dd0d5c Compare January 8, 2021 14:40

lpichler mentioned this pull request Jan 8, 2021

Fill group_id when widget set is saved in dashboard.rb ManageIQ/manageiq-ui-classic#7561

Closed

1 task

kbrock reviewed Jan 15, 2021

View reviewed changes

Add validations for miq_widget_set

b203865

lpichler force-pushed the validations_for_miq_widget_set branch from 0dd0d5c to b203865 Compare January 20, 2021 14:31

lpichler changed the title ~~[WIP] Add validations for MiqWidgetSet~~ Add validations for MiqWidgetSet Jan 20, 2021

miq-bot removed the wip label Jan 20, 2021

kbrock merged commit 5773337 into ManageIQ:master Jan 20, 2021

lpichler mentioned this pull request Jan 20, 2021

Improvements for MiqWidgetSet model #20972

Merged

This was referenced Jan 20, 2021

Widget groups #20974

Closed

Update widget specs to set owner and set_data ManageIQ/manageiq-ui-classic#7593

Closed

Fix specs related to widget set factories ManageIQ/manageiq-ui-classic#7594

Merged

himdel mentioned this pull request Feb 2, 2021

WidgetSet - fix db:seed, don't blow up when set_data is a HashWithIndifferentAccess #20999

Merged

kbrock mentioned this pull request Dec 3, 2021

Missing endpoint for manipulating with dashboards ManageIQ/manageiq-api#953

Open

	validates :group_id, :presence => true, :unless => ->(x) { x.read_only? }
	validates :group_id, :presence => true, :unless => :read_only?

	end
	validates :description, :uniqueness_when_changed => {:scope => :owner_id}

		errors.add(:base, _("Unable to delete read-only WidgetSet")) if read_only?
		throw :abort unless errors[:base].empty?

	replace_children(Array(set_data_widgets)) if members.map(&:id) != set_data_widgets.ids
	replace_children(Array(set_data_widgets)) if members.map(&:id)&.sort != set_data_widgets.ids.sort

	SET_DATA_COLS.map { \|x\| set_data[x] }.flatten.compact
	SET_DATA_COLS.flat_map { \|x\| set_data[x] }.compact

Add validations for MiqWidgetSet #20890

Add validations for MiqWidgetSet #20890

Conversation

lpichler commented Dec 11, 2020 • edited Loading

Links

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 Jan 4, 2021

kbrock 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

lpichler commented Jan 7, 2021 • edited Loading

lpichler commented Jan 8, 2021

kbrock 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

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

miq-bot commented Jan 20, 2021

lpichler commented Jan 20, 2021 • edited Loading

lpichler commented Jan 20, 2021

lpichler commented Dec 11, 2020 •

edited

Loading

lpichler commented Jan 7, 2021 •

edited

Loading

lpichler commented Jan 20, 2021 •

edited

Loading