Skip to content

[WIP] Fix widget sets with orphaned group #23404

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

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

[WIP] Fix widget sets with orphaned group #23404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de422f9
Remove widgets sets a deleted group was subscribed to
jrafanie Mar 31, 2025
1d4498d
Only look at subscribed groups that exist
jrafanie Mar 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion app/models/miq_group.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class MiqGroup < ApplicationRecord

validates :description, :presence => true, :unique_within_region => {:match_case => false}
validate :validate_default_tenant, :on => :update, :if => :tenant_id_changed?
before_destroy :ensure_can_be_destroyed
before_destroy :ensure_can_be_destroyed, :destroy_subscribed_widget_sets
after_destroy :reset_current_group_for_users

# For REST API compatibility only; Don't use otherwise!
Expand Down Expand Up @@ -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 •
edited
Loading

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

@kbrock kbrock May 16, 2025

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

end

def ensure_can_be_destroyed
errors.add(:base, _("The login group cannot be deleted")) if current_user_group?
errors.add(:base, _("The group has users assigned that do not belong to any other group")) if single_group_users?
Expand Down
2 changes: 2 additions & 0 deletions app/models/miq_widget.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@jrafanie jrafanie Jun 16, 2025

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.


Comment on lines +410 to +411
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.

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

Copy link
Member Author

@jrafanie jrafanie Jun 16, 2025

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.

user_objs = users_by_userid.values_at(*v).reject(&:blank?)
h[groups_by_id[k]] = user_objs if user_objs.present?
end
Expand Down
14 changes: 14 additions & 0 deletions spec/models/miq_widget_set_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,20 @@
end
end

it "group and owner associated to a deleted group" do
@ws_group.update(:group => group)
user.destroy
group.destroy!
expect(MiqWidgetSet.count).to eq(0)
end

it "group associated to a deleted group, nil owner" do
@ws_group.update!(:group => group, :owner => nil)
user.destroy
group.destroy!
expect(MiqWidgetSet.count).to eq(0)
end

it "when a group dashboard is deleted" do
expect(MiqWidgetSet.count).to eq(1)
@ws_group.destroy
Expand Down
Loading