Skip to content

[WIP] Fix widget sets with orphaned group #23404

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jrafanie
Copy link
Member

@jrafanie jrafanie commented Mar 31, 2025

TODO: I have no idea why we have ownership AND group association for a widget set and why the owner can be nil and the group pointing to an orphaned group id. This is fixing the symptoms but not really solving the problem yet. We need to understand why these things were happening and how to properly fix it going forward.

see also:

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff

@@ -342,6 +342,10 @@ def current_user_group?
id == current_user_group.try(:id)
end

def destroy_subscribed_widget_sets
MiqWidgetSet.where(:group_id => id).destroy_all
Copy link
Member

@kbrock kbrock Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if we delete this group, a user widget set will probably want to stick around.
So if user_id is around, want to figure out the user's new group and set the widget set's group to the user's new set?

--
I do want a test around user_id and group_id set

MiqWidgetSet.where(:group_id => id, :user_id => nil).destroy_all

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update: if we destroy this group, destroy any widget set linking to this

Comment on lines +410 to +411
next unless groups_by_id.key?(k) # Make sure the group associated with a widget set / dashboard hasn't been removed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't the next line (blank) take care of some of this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, user can exist but the widget set's group_id can be orphaned.

@kbrock
Copy link
Member

kbrock commented Apr 1, 2025

psudo code - have not tested yet (will revsit when we ask questions)

Widget Set

current table

user group action
nil nil keep - default
nil valid keep - group dashboard
nil invalid delete - these don't exist
valid nil delete - these don't exist
valid invalid delete - main issue
valid valid/no membership delete
valid valid keep
invalid any delete
--- users that have dashboard (that is not in the list of the user's groups)
SELECT miq_sets.id, miq_sets.name, miq_sets.read_only,
       miq_sets.owner_type, miq_sets.owner_id, miq_sets.userid, miq_sets.group_id
FROM miq_sets
WHERE
  miq_sets.set_type = 'MiqWidgetSet'
  AND miq_sets.userid IS NOT NULL
  AND (
    -- invalid user
    (
      NOT EXISTS (SELECT 1 FROM users WHERE users.userid = userid)
    -- invalid group
    ) OR (
      NOT EXISTS (
        SELECT 1 FROM users
        join miq_groups_users on users.id = miq_groups_users.user_id
        WHERE users.userid = miq_sets.userid and miq_groups_users.miq_group_id = miq_sets.group_id
      )
    )
  )
ORDER BY userid, group_id;

--- sanity check: groups the users are in
SELECT users.id, users.userid, miq_groups_users.miq_group_id, miq_groups.description
FROM users
JOIN miq_groups_users on users.id = miq_groups_users.user_id
JOIN miq_groups ON miq_groups.id = miq_groups_users.miq_group_id
WHERE users.userid in (select userid from miq_sets where set_type = 'MiqWidgetSet')
ORDER by users.userid;

Actions

remove group from a user

DELETE miq_sets
  WHERE miq_sets.set_type = 'MiqWidgetSet'
  AND miq_sets.userid = $userid
  AND miq_sets.group_id = $old_group_id

delete user

DELETE miq_sets
  WHERE miq_sets.set_type = 'MiqWidgetSet'
  AND miq_sets.userid = $old_userid

delete group

DELETE  miq_sets
  WHERE miq_sets.set_type = 'MiqWidgetSet'
  AND   group_id = $old_group_id
-- intentionally not using owner_id (want group and user dashboards)

@kbrock
Copy link
Member

kbrock commented Apr 8, 2025

ok, updated queries to point to miq_sets for the widget sets

@jrafanie
Copy link
Member Author

jrafanie commented Apr 10, 2025

A revised version the above pseudo code was tested to verify the dashboards for invalid users, users no longer in the group (group exists but user isn't in it anymore or group does NOT exist).

DELETE FROM miq_sets
WHERE
  miq_sets.set_type = 'MiqWidgetSet'
  AND miq_sets.userid IS NOT NULL
  AND (
    -- invalid user
    (
      NOT EXISTS (SELECT 1 FROM users WHERE users.userid = userid)
    -- invalid group
    ) OR (
      NOT EXISTS (
        SELECT 1 FROM users
        join miq_groups_users on users.id = miq_groups_users.user_id
        WHERE users.userid = miq_sets.userid and miq_groups_users.miq_group_id = miq_sets.group_id
      )
    )
  );

For followup, here and elsewhere, we'll need to:

  1. Drop dashboards for users that are removed from a group
  2. Drop dashboards for users who are deleted
  3. Drop dashboards for users linked to a group that doesn't exist (similar to 1)
  4. Handle situations where a group is deleted. For user owned dashboards, their group_id should be populated and any references to the deleted group should be removed. For group owned dashboards, the owner should be the checked to see if that group was an owner. That should be handled via the has_many :miq_widget_sets, :as => :owner, :dependent => :destroy on miq_group.rb.
  5. Should a data migration be created to fix these?

@Fryguy
Copy link
Member

Fryguy commented Apr 10, 2025

  1. Drop dashboards for users that are removed from a group
  2. Drop dashboards for users who are deleted
  3. Drop dashboards for users linked to a group that doesn't exist (similar to 1)

These probably will need to be done async to deleting the user/group, maybe as a callback it's put on the queue or maybe we create a purger.

  1. is the most important one, I think...if a user is removed from a group then we need to nullify the association out because the user still exists.
  1. Should a data migration be created to fix these?

Probably for any existing ones, but maybe a purger is more appropriate to keep it clean in an ongoing manner?


I think there needs to be a 3.5 for dealing with deleting groups.

@jrafanie
Copy link
Member Author

jrafanie commented Apr 11, 2025

I think there needs to be a 3.5 for dealing with deleting groups.

Good point. That's the original code for this PR so I neglected to put it.

EDIT: Edited the comment above to remember it when I return.

@kbrock kbrock self-assigned this May 19, 2025
@jrafanie
Copy link
Member Author

I took a look at this again. I'm not sure how to deal with users leaving groups since miq_groups_users doesn't have a model so we can't see what's changed and update the widget set to reflect the new group. Not only that but the widget set also includes the group id in the description:

 #<MiqWidgetSet:0x000000012e939250
  id: 30,
  name: "default",
  description: "default dashboard for user joe in group id 6",
  set_type: "MiqWidgetSet",
  created_on: Mon, 16 Jun 2025 19:11:00.924030000 UTC +00:00,
  updated_on: Mon, 16 Jun 2025 19:11:00.924030000 UTC +00:00,
  guid: "294a0738-1f4c-41a3-a7aa-a215251db970",
  read_only: nil,
  set_data: "[FILTERED]",
  mode: nil,
  owner_type: nil,
  owner_id: nil,
  userid: "joe",
  group_id: 6>,

I think the only way to fix it is to check for orphaned widget sets after updating a user's groups:

index 794cd5d35c..a6df376a38 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -49,6 +49,7 @@ class User < ApplicationRecord
                                 :allow_nil => true, :message => "must be a valid email address"},
                     :length => {:maximum => 255}
   validates :current_group, :inclusion => {:in => proc { |u| u.miq_groups }, :allow_nil => true, :if => :current_group_id_changed?}
+  after_save :update_widget_sets_on_miq_group_change

   # use authenticate_bcrypt rather than .authenticate to avoid confusion
   # with the class method of the same name (User.authenticate)
@@ -319,6 +320,9 @@ class User < ApplicationRecord
   end

   def miq_groups=(groups)
+    @miq_groups_ids_previously_was = miq_groups_ids.sort
+    @miq_groups_ids_changed = groups.collect(&:id).sort == @miq_groups_previously_was
+
     super
     self.current_group = groups.first if current_group.nil? || !groups.include?(current_group)
   end
@@ -332,6 +336,12 @@ class User < ApplicationRecord
     save!
   end

+  def update_widget_sets_on_miq_group_change
+    if @miq_groups_ids_changed
+
+    end
+  end
+
   def admin?
     self.class.admin?(userid)
   end

@@ -407,6 +407,8 @@ def grouped_subscribers
groups_by_id = MiqGroup.in_my_region.where(:id => grouped_users.keys).index_by(&:id)
users_by_userid = User.in_my_region.where(:userid => grouped_users.values.flatten.uniq).index_by(&:userid)
grouped_users.each_with_object({}) do |(k, v), h|
next unless groups_by_id.key?(k) # Make sure the group associated with a widget set / dashboard hasn't been removed
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is the only fix. The problem was that grouped_users from widget sets can include groups that don't exist anymore. I need to think about it some more.

jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 23, 2025
If a group was previously subscribed to the widget, we shouldn't
generate content for it.  We should properly update widgets
when groups are removed but for now, this allows the content generator
to work with widgets having orphaned group references.

Extracted from ManageIQ#23404

CP4AIOPS-15687
@jrafanie
Copy link
Member Author

Ok, so I punted on fixing the workflow of group subscribers of widgets as there are a few ways groups subscribe to them. For now, I extracted a PR just to fix the widget generation: #23491

jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 23, 2025
If a group was previously subscribed to the widget, we shouldn't
generate content for it.  We should properly update widgets
when groups are removed but for now, this allows the content generator
to work with widgets having orphaned group references.

Extracted from ManageIQ#23404

CP4AIOPS-15687
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 27, 2025
If a group was previously subscribed to the widget, we shouldn't
generate content for it.  We should properly update widgets
when groups are removed but for now, this allows the content generator
to work with widgets having orphaned group references.

Extracted from ManageIQ#23404

CP4AIOPS-15687
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 27, 2025
If a group was previously subscribed to the widget, we shouldn't
generate content for it.  We should properly update widgets
when groups are removed but for now, this allows the content generator
to work with widgets having orphaned group references.

Extracted from ManageIQ#23404

CP4AIOPS-15687
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants