Don't include deleted groups in widget generation #23491
Conversation
jrafanie
force-pushed
the
fix-grouped-subscribers
branch
from
51e1bb6 to
e70f4f2
Compare
app/models/miq_widget.rb
Outdated
| 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) |
There was a problem hiding this comment.
not sure if this should be moved up to grouped_users_by_id... I think it could be but don't know if that will break something else
There was a problem hiding this comment.
Should we log a warning or something in this case?
There was a problem hiding this comment.
I think it makes sense to change grouped_users_by_id to better meet our needs.
It is only used by this method.
It does look like it tried to handle the nil case with the 2 separate blank? tests. So I'm surprised any nil values sneaked through.
Also feel we want to eradicate the cases that did slip through.
def grouped_users_by_id
id_groups = Hash.new { |h, k| h[k] = [] }
memberof.compact.each_with_object(id_groups) do |ws, h|
h[ws.group_id] << ws.userid unless ws.userid.blank? || ws.group_id.blank?
end
endThere was a problem hiding this comment.
Thanks, that's what I wanted to hear. I didn't love my initial hack and spent more time in creating failing tests.
spec/models/miq_widget_spec.rb
Outdated
| end | ||
|
|
||
| it 'ignores a group that no longer exists' do | ||
| @user1.update(:miq_groups => [@group2]) |
There was a problem hiding this comment.
Is this so that when we delete group1 it doesn't fail due to being a non-empty group since user1 is in it?
There was a problem hiding this comment.
I removed this line by not using destroy since the idea was to leave a widget's subscription to be orphaned as the group was removed. Previously, I had to workaround doing group1.destroy by making sure the existing user was moved from group 1 to 2. By using delete, I don't need to do that.
spec/models/miq_widget_spec.rb
Outdated
|
|
||
| it 'ignores a group that no longer exists' do | ||
| @user1.update(:miq_groups => [@group2]) | ||
| @group1.destroy |
There was a problem hiding this comment.
Should this be delete? I noticed we used delete in the other test (or maybe we should be using destroys in the other tests?)
| @group1.destroy | |
| @group1.delete |
There was a problem hiding this comment.
good point... I noticed the deletes too but still did destroy out of habit. I can update them all in another commit.
There was a problem hiding this comment.
See above... I went with delete for the purposes of testing the widget <-> group subscription.
Fryguy
left a comment
Fryguy
left a comment
There was a problem hiding this comment.
Overall LGTM - code-wise this is good to merge, but I had a few questions first.
jrafanie
force-pushed
the
fix-grouped-subscribers
branch
from
e70f4f2 to
7521867
Compare
| groups = MiqGroup.where(:id => [@group1, @group2]) | ||
| expect(MiqGroup).to receive(:in_my_region).and_return(groups) | ||
| allow(groups).to receive(:where).and_return(groups) | ||
| @widget_report_vendor_and_guest_os.grouped_subscribers |
There was a problem hiding this comment.
This test was stubbing the world and not actually doing any assertions so I fixed it.
| h[ws.group_id] << ws.userid unless ws.userid.blank? || ws.group_id.blank? | ||
| next if ws.userid.blank? | ||
| next if ws.group_id.blank? | ||
| next unless group_ids.include?(ws.group_id) |
There was a problem hiding this comment.
The line above is the actual fix. ☝️
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
force-pushed
the
fix-grouped-subscribers
branch
from
ed1e4dc to
ddcdccf
Compare
|
Checked commits jrafanie/manageiq@b7f1723~...ddcdccf with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.62.0, and yamllint |
Follow up to discussion found in: ManageIQ#23491 It would be too noisy to bring this back for backports so it's a master only change.
|
Backported to |
Don't include deleted groups in widget generation (cherry picked from commit 107ae3f)
4 participants
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 #23404
CP4AIOPS-15687