CustomButtonSet - make sure children follow button_order #18368

himdel · 2019-01-16T14:21:57Z

CustomButtonSet has currently 2 independent lists of children:

set_data[:button_order] is an array of ids with the right order,
children (or members) is a collection of custom buttons, in no order

Currently, in the UI, the tree is reading button_order, the GTL is reading members,
the SUI toolbar is using button_order, the OPS UI toolbar is using both.

It's essential to keep the 2 lists the same, and there are 2 ways of achieving it:

adding methods to add items to specific position, that will handle both button_order and children, and expose those via the API
deciding button_order is the canonical collection, and we can always replace children based on the ids in button order.

The first option would mean doing N+1 API requests when creating a button group with N assigned buttons,
the second option means we only have to edit the custom button set, without any per-button requests.

(We never create a new custom button as part of editing/creating a set.)

Fixes ManageIQ/manageiq-api#539
(together with ManageIQ/manageiq-api#541 exposing the collection so that we can query it via the API)

himdel · 2019-01-16T14:22:18Z

@lpichler @djberg96 can you take a look please?

himdel · 2019-01-16T14:24:53Z

Cc @ZitaNemeckova , with this change, you only need to send the button_order right.

ZitaNemeckova · 2019-01-16T14:33:57Z

Yes. With this only button_order is needed. It adds/removes buttons to/from button group. Removed buttons is unassigned. So works as requested.

lpichler · 2019-01-16T14:42:14Z

app/models/custom_button_set.rb

@@ -1,6 +1,11 @@
 class CustomButtonSet < ApplicationRecord
  acts_as_miq_set

+  before_save :update_children
+  def update_children
+    replace_children Rbac.filtered(CustomButton.find(set_data[:button_order]))


@himdel what about CustomButton.where(:id => ..) to don't throw exception when IDs don't exists ? I also prefer where because then we can pass just query (ActiveRecord::Relation) to RBAC. (it is our long long term goal to pass only queries to RBAC)

Ah, makes sense, fixed :)

himdel · 2019-01-16T14:48:54Z

what about CustomButton.where(:id => ..) to don't throw exception when IDs don't exists ?

.. actually, @lpichler I don't know...

If we don't throw an exception on invalid ids, it means button_order and children won't match after successful saves..

himdel · 2019-01-16T14:56:05Z

Added a second commit to explicitly throw(:abort) when we shouldn't save the custom button set.
(That happens when one of the IDs specified in button_order does not exist, or is not visible to the current user.)

skateman · 2019-01-18T10:20:06Z

app/models/custom_button_set.rb

@@ -1,6 +1,13 @@
 class CustomButtonSet < ApplicationRecord
  acts_as_miq_set

+  before_save :update_children
+  def update_children
+    replace_children Rbac.filtered(CustomButton.where(:id => set_data[:button_order]))


Let's imagine a hypothetical scenario where you have 2 users (A and B). User A has access to the buttons x,y but not to z while user B has access to the buttons y, z but not to x. There's a custom button set already available with buttons x and z. What happens if either user A or user B tries to add to this set the button y? I think if user A touches this piece of code, the button set will contain x,y but not z even if it was originally in the set. Same goes for user B but then we will loose button x. Not sure if this situation is possible, if yes then we have a "loss of precision" here.

Agreed, you're right. (I also have no idea how custom button sets act for different users, or indeed if rbac over custombuttons does anything. Anybody?)

Maybe we should just push RBAC filtering to the loading side, and skip it when saving?

The practical implication of the PR as it stands now (with Rbac.filtered) is:

a user won't be able to edit a custom button set if it contains any buttons that user can't see
(because that throw(:abort) should trigger)

I am not able to find any RBAC rule for CustomButton model. Are you referencing to the rule
"User can see some CustomButtons" according to CustomButton#userid ?

if there is existence of such rule, it seems that it is not implemented in Rbac.filtered call.

a user won't be able to edit a custom button set if it contains any buttons that user can't see

So should I implement it to the RBAC ?

So should I implement it to the RBAC ?

No, it already does that (because the sets will not be the same after filtering, so if will abort).

If there are currently no rbac rules for custom buttons, that's fine, that probably means we don't have a problem at all, right @skateman ?

(Though it makes sense to make the solution correct for when rbac gets involved anyway.)

@himdel yup, who knows when this can 💥 into our face when RBAC gets smarter 😕

👍 so, which behaviour makes most sense then?

We can allow a user to edit sets where they can't access all the buttons, and not remove those buttons. But that means a user can theoretically add or remove buttons they don't have access to.

Or we can only allow a user to edit a set where that user has access to all the buttons. But then, it means they can potentially do less than they could do now. (Well, when there is a rule to make some buttons inaccessible to that user anyway.)

@djberg96 is on PTO, but I'll talk to him on Tuesday about this, what else could we do on the API side.

himdel · 2019-01-18T11:26:02Z

But looks like some of my assumptions may be wrong, even though a CustomButton has no links back to the CustomButtonSet, for some reason something is breaking for new CustomButtonSet creation..

     ActiveRecord::RecordNotSaved:
       You cannot call create unless the parent is saved

So,
@miq-bot add_label wip

and I'm thinking...

in before_save, we should check that all the buttons exist (or are visible to the user) and abort if not
but replace_children should only happen in after_save
tests

EDIT: nope, looks like everything works with before_save, as long as we check that set_data is not nil
EDIT2: actually, no, nil only works when creating without button_order, causes spec/models/custom_button_set_spec.rb to fail, so, yeah, splitting

himdel · 2019-01-23T11:08:33Z

Updated to handle new, empty CustomButtonSets.

The only remaining spec failures come from ./spec/lib/task_helpers/imports/custom_buttons_spec.rb, still looking.

himdel · 2019-01-23T11:44:52Z

..And changed TaskHelpers::Import::CustomButtons to strip button_order before importing.
It was ignored anyway (which is IMO a bug, but the sample data doesn't have any ids for the buttons so, can't do a thing about it).

Should be green now :)

himdel · 2019-01-23T11:45:01Z

@miq-bot remove_label wip

skateman · 2019-01-23T11:47:02Z

app/models/custom_button_set.rb

+    end
+
+    children = Rbac.filtered(CustomButton.where(:id => set_data[:button_order]))
+    throw(:abort) if children.pluck(:id).sort != set_data[:button_order].sort


I think you want to use raise here with an actual exception and its explanation.

Well, as far as I can tell, throw :abort is the only way for before_save to abort that's compatible with rails 5.

(docs)

Or am I reading it wrong?

skateman · 2019-01-23T11:51:32Z

app/models/custom_button_set.rb

@@ -1,6 +1,19 @@
 class CustomButtonSet < ApplicationRecord
  acts_as_miq_set

+  before_save :update_children
+  def update_children
+    if set_data.nil? || set_data[:button_order].nil?


I think this could be a one-liner and you don't need the OR at all if "you gotta get up and try, and try, and try" 🤣

Suggested change

if set_data.nil? || set_data[:button_order].nil?

return remove_all_children if set_data.try(:[], :button_order).nil?

Can't be a one-liner, if remove_all_children ever returns false, it will abort in Rails 4.

But 👍 , updating to use try.

skateman · 2019-01-23T11:52:55Z

lib/task_helpers/imports/custom_buttons.rb

+            if klass.name == "CustomButtonSet"
+              order = obj.fetch_path('attributes', 'set_data', :button_order)
+              obj['attributes']['set_data'][:button_order] = nil if order.present?
+            end


I think this is a red flag for @martinpovolny, but I understand that we need something like this 😕

For some reason, none of CustomButtonSet === klass, klass.is_a? CustomButtonSet, klass.kind_of? ::CustomButtonSet are true, only string comparison works.

And that "custom button" import deals with 3 different types of entities (CustomButton, CustomButtonSet, and ResourceAction for some reason).

But yeah, I don't like it either :(

That seems really weird. Have you inspected the klass variable to see why that would happen? What does klass.class show?

@djberg96

> klass => CustomButtonSet(id: integer, name: string, description: string, set_type: string, created_on: datetime, updated_on: datetime, guid: string, read_only: boolean, set_data: text, mode: string, owner_type: string, owner_id: integer, userid: string, group_id: integer, href_slug: string, region_number: integer, region_description: string) > klass.class => Class > klass.name => "CustomButtonSet" > CustomButtonSet => CustomButtonSet(id: integer, name: string, description: string, set_type: string, created_on: datetime, updated_on: datetime, guid: string, read_only: boolean, set_data: text, mode: string, owner_type: string, owner_id: integer, userid: string, group_id: integer, href_slug: string, region_number: integer, region_description: string) > CustomButtonSet.class => Class > CustomButtonSet.name => "CustomButtonSet" > klass.is_a? CustomButtonSet => false > CustomButtonSet.is_a? klass => false

¯\_(ツ)_/¯

Fun :)

> klass.object_id => 47105247798240 > CustomButtonSet.object_id => 47105247798240

I wonder if this could be caused by FactoryGirl, I'm running this via be rspec spec/lib/task_helpers/imports/custom_buttons_spec.rb

You mean FactoryBot? ;)

Anyway, I've been burned a few times by FactoryBot creating new instances internally, causing allow to fail. I'll have to look at the spec.

Heheh, I actually don't think being a girl should be considered offensive :). But yeah, that.

Thanks! :)

skateman · 2019-01-23T11:54:48Z

app/models/custom_button_set.rb

+    children = Rbac.filtered(CustomButton.where(:id => set_data[:button_order]))
+    throw(:abort) if children.pluck(:id).sort != set_data[:button_order].sort
+
+    replace_children Rbac.filtered(CustomButton.where(:id => set_data[:button_order]))


Maybe it's wrong, but can't we use find here, then you don't need the :id at all and AFAIK it fails with invalid IDs.

I changed that to where after feedback from @lpichler - ref

himdel · 2019-01-23T12:25:14Z

Aand added a spec, and split into before_save/after_save after all, since we need the set to exist before adding children.

himdel · 2019-01-23T12:25:42Z

@miq-bot remove_label wip

Seriously :)

ZitaNemeckova · 2019-01-24T09:24:37Z

Made changes so ManageIQ/manageiq-ui-classic#4913 doesn't need it so it doesn't need hammer/yes or gaprindashvili/yes. But thanks for making it for master :)

CustomButtonSet has currently 2 independent lists of children: - `set_data[:button_order]` is an array of ids with the right order, - `children` (or `members`) is a collection of custom buttons, in no order Currently, in the UI, the tree is reading button_order, the GTL is reading `members`, the SUI toolbar is using button_order, the OPS UI toolbar is using both. It's essential to keep the 2 lists the same, and there are 2 ways of achieving it: * adding methods to add items to specific position, that will handle both button_order and children, and expose those via the API * deciding `button_order` is the canonical collection, and we can always replace children based on the ids in button order. The first option would mean doing N+1 API requests when creating a button group with N assigned buttons, the second option means we only have to edit the custom button set, without any per-button requests. (We never create a new custom button as part of editing/creating a set.)

…rder don't exist without this, code like... ``` cbs.set_data[:button_order] = [1,2,3] cbs.save! ``` would suceed in saving a CustomButtonSet with that button order, even when some of the buttons don't exist. Since that would mean that children no longer match the button order, we need to throw. `throw(:abort)` is the only valid way of stopping the chain in Rails 5, and seems to work find in older rails too.

…ata nil no set_data => no children

…order instead adding a child won't actually affect any custom button toolbar that relies on the order (such as SUI)

…ing CustomButtonSet button_order is ignored for imports anyway, `custom_button_set_post` rewrites it. But having it set during `create!` causes `update_children` to fail because it can't find those nonexistent CustomButtons.

otherwise, creating a CustomButtonSet with nonempty button_order fails: ActiveRecord::RecordNotSaved: You cannot call create unless the parent is saved # ./app/models/mixins/relationship_mixin.rb:446:in `add_relationship'

miq-bot · 2019-02-12T18:09:44Z

Checked commits https://github.com/himdel/manageiq/compare/5069334e0f6f373f036f230e8ec0aa34bcf8c9e9~...1c4ea876a1dfb0a8a6ba3e0ac54c739a8484d1e8 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 1 offense detected

spec/models/generic_object_definition_spec.rb

⚠️ - Line 300, Col 7 - Lint/UselessAssignment - Useless assignment to variable - generic_group_set.

skateman · 2019-02-18T06:42:47Z

@himdel please ping me if you have time to discuss this 😉

skateman · 2019-02-24T09:43:31Z

I think this could be sorted out better with a rework of sets in our backend, but that's not the call of the UI team to make. So this is good to go from my side if it solves @ZitaNemeckova's problem with the API-driven generic object stuff. But I'm also pinging @kbrock, as far as I remember he said something about our sets being bad

gtanzillo

@skateman @lpichler @djberg96 I just finished reviewing this commit by commit and I am ok with the changes. The if klass.name == "CustomButtonSet" also make me a bit uneasy but it should be fine. Are you guy ok with merging?

djberg96 · 2019-03-01T16:31:02Z

@gtanzillo Yep, a bit janky, but let's go with it for now and refactor as needed.

after ManageIQ/manageiq/pull/18368 the purpose of `set_data[:button_order]` has been changed, and an error is raised after going through an array, where the button does not exist anymore

d-m-u · 2019-08-13T13:26:43Z

This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1737449, which is open for h, so we should backport (along with ManageIQ/manageiq-ui-classic#5319 and ManageIQ/manageiq-api#563 per conversation with @himdel.)

d-m-u · 2019-08-21T15:08:46Z

@miq-bot add_label hammer/yes

CustomButtonSet - make sure children follow button_order (cherry picked from commit 2c50993) https://bugzilla.redhat.com/show_bug.cgi?id=1745198

simaishi · 2019-09-05T00:49:13Z

Hammer backport details:

$ git log -1
commit c30fdb08163966a79f87f1dc98f0d2c8aad3cbc8
Author: Gregg Tanzillo <[email protected]>
Date:   Thu Mar 7 10:28:21 2019 -0500

    Merge pull request #18368 from himdel/custom_button_set_children
    
    CustomButtonSet - make sure children follow button_order
    
    (cherry picked from commit 2c5099368d8dd21e81c6ca01687900b66020db58)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1745198

This was referenced Jan 16, 2019

Add/Remove CustomButton(s) to CustomButtonSet ManageIQ/manageiq-api#539

Closed

[WIP] Make custom_buttons a subcollection of custom_button_sets ManageIQ/manageiq-api#541

Closed

lpichler reviewed Jan 16, 2019

View reviewed changes

ZitaNemeckova mentioned this pull request Jan 16, 2019

GOD's button group can have buttons assign/unassign during create/edit ManageIQ/manageiq-ui-classic#4913

Merged

skateman reviewed Jan 18, 2019

View reviewed changes

miq-bot changed the title ~~CustomButtonSet - make sure children follow button_order~~ [WIP] CustomButtonSet - make sure children follow button_order Jan 18, 2019

miq-bot added the wip label Jan 18, 2019

miq-bot changed the title ~~[WIP] CustomButtonSet - make sure children follow button_order~~ CustomButtonSet - make sure children follow button_order Jan 23, 2019

miq-bot removed the wip label Jan 23, 2019

skateman suggested changes Jan 23, 2019

View reviewed changes

miq-bot changed the title ~~CustomButtonSet - make sure children follow button_order~~ [WIP] CustomButtonSet - make sure children follow button_order Jan 23, 2019

miq-bot added the wip label Jan 23, 2019

miq-bot changed the title ~~[WIP] CustomButtonSet - make sure children follow button_order~~ CustomButtonSet - make sure children follow button_order Jan 23, 2019

miq-bot removed the wip label Jan 23, 2019

himdel mentioned this pull request Jan 23, 2019

CustomButtonSet - don't touch children/members directly, set button_order ManageIQ/manageiq-ui-classic#5187

Merged

ZitaNemeckova mentioned this pull request Jan 24, 2019

[WIP] Simplify when backend updates CustomButtonSet's children based on cha… ManageIQ/manageiq-ui-classic#5190

Closed

himdel added 2 commits February 12, 2019 12:07

himdel added 5 commits February 12, 2019 12:07

CustomButton#update_children - handle new custom buttons having set_d…

dafc6a7

…ata nil no set_data => no children

ServiceTemplate spec - don't use add_children on custom buttons, set …

31d6d32

…order instead adding a child won't actually affect any custom button toolbar that relies on the order (such as SUI)

CustomButtonSet - only set children in after_save

cb46b8c

otherwise, creating a CustomButtonSet with nonempty button_order fails: ActiveRecord::RecordNotSaved: You cannot call create unless the parent is saved # ./app/models/mixins/relationship_mixin.rb:446:in `add_relationship'

GOD spec - don't use add_member for custom button sets

1c4ea87

gtanzillo reviewed Mar 1, 2019

View reviewed changes

djberg96 approved these changes Mar 1, 2019

View reviewed changes

gtanzillo merged commit 2c50993 into ManageIQ:master Mar 7, 2019

gtanzillo added this to the Sprint 107 Ending Mar 18, 2019 milestone Mar 7, 2019

gtanzillo added the bug label Mar 7, 2019

djberg96 mentioned this pull request Mar 7, 2019

[WIP] Add type column to service_order table ManageIQ/manageiq-schema#328

Closed

himdel deleted the custom_button_set_children branch March 8, 2019 10:28

This was referenced Mar 8, 2019

Fix failing travis - stub my_server when dealing with custom button sets ManageIQ/manageiq-ui-classic#5319

Merged

GenericObject spec - fix failures after core PR ManageIQ/manageiq-api#563

Merged

romanblanco mentioned this pull request Jun 28, 2019

[WIP] Removing deleted Custom Buttons ManageIQ/manageiq-schema#370

Closed

ZitaNemeckova mentioned this pull request Jul 10, 2019

ServiceTemplate copy_template duplicates buttons #18954

Closed

d-m-u mentioned this pull request Aug 21, 2019

Fix the custom button set set_data hash links to the original buttons on copy #19169

Merged

miq-bot added the hammer/yes label Aug 21, 2019

simaishi pushed a commit that referenced this pull request Sep 5, 2019

Merge pull request #18368 from himdel/custom_button_set_children

c30fdb0

CustomButtonSet - make sure children follow button_order (cherry picked from commit 2c50993) https://bugzilla.redhat.com/show_bug.cgi?id=1745198

simaishi added hammer/backported and removed hammer/yes labels Sep 5, 2019

himdel mentioned this pull request Dec 3, 2019

CustomButtonSet - handle deleted custom buttons still in button_order #19573

Merged

	if set_data.nil? \|\| set_data[:button_order].nil?
	return remove_all_children if set_data.try(:[], :button_order).nil?

CustomButtonSet - make sure children follow button_order #18368

CustomButtonSet - make sure children follow button_order #18368

Conversation

himdel commented Jan 16, 2019

himdel commented Jan 16, 2019

himdel commented Jan 16, 2019

ZitaNemeckova commented Jan 16, 2019

lpichler Jan 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented Jan 16, 2019 • edited Loading

himdel commented Jan 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

lpichler Jan 18, 2019 • 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

himdel commented Jan 18, 2019 • edited Loading

himdel commented Jan 23, 2019

himdel commented Jan 23, 2019

himdel commented Jan 23, 2019

Choose a reason for hiding this comment

himdel Jan 23, 2019 • 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

himdel Jan 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Jan 23, 2019 • edited Loading

Choose a reason for hiding this comment

himdel Jan 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djberg96 Jan 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Jan 23, 2019 • edited Loading

Choose a reason for hiding this comment

himdel commented Jan 23, 2019

himdel commented Jan 23, 2019

ZitaNemeckova commented Jan 24, 2019

miq-bot commented Feb 12, 2019

skateman commented Feb 18, 2019

skateman commented Feb 24, 2019

gtanzillo left a comment

Choose a reason for hiding this comment

djberg96 commented Mar 1, 2019

d-m-u commented Aug 13, 2019

d-m-u commented Aug 21, 2019

simaishi commented Sep 5, 2019

lpichler Jan 16, 2019 •

edited

Loading

himdel commented Jan 16, 2019 •

edited

Loading

himdel Jan 18, 2019 •

edited

Loading

lpichler Jan 18, 2019 •

edited

Loading

himdel commented Jan 18, 2019 •

edited

Loading

himdel Jan 23, 2019 •

edited

Loading

himdel Jan 23, 2019 •

edited

Loading

himdel Jan 23, 2019 •

edited

Loading

himdel Jan 23, 2019 •

edited

Loading

djberg96 Jan 23, 2019 •

edited

Loading

himdel Jan 23, 2019 •

edited

Loading