Skip to content
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.

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

CustomButtonSet - don't touch children/members directly, set button_order #5187

Merged
merged 1 commit into from
Mar 7, 2019
Merged

CustomButtonSet - don't touch children/members directly, set button_order #5187

merged 1 commit into from
Mar 7, 2019

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Jan 23, 2019

Depends on ManageIQ/manageiq#18368

The idea is, we shold not try to manipulate both button_order and members for CustomButtons,
members should always follow button_order.

The core PR adds an after_save to CustomButtonSet that always updates the set of children to match button_order.

This PR removes all code which updates members/children, and makes sure button_order gets updated instead.


This probably needs some testing,
I'm not quite sure what..

      org_mems = @custom_button_set.members # clean up existing members
      org_mems.each do |m|
        uri = CustomButton.find(m.id)
        uri.save
      end

(used twice)

was supposed to achieve, find+save sound like a no-op.

@himdel
Copy link
Contributor Author

himdel commented Jan 23, 2019

Cc @martinpovolny

mems = automation_set.members
if mems.length > 1
mems.each do |m|
automation_set.remove_member(custom_button) if m.id == custom_button
Copy link
Contributor Author

Choose a reason for hiding this comment

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

funnily enough, comparing integers and CustomButton instances probably didn't work

@miq-bot
Copy link
Member

miq-bot commented Jan 23, 2019

Checked commit https://github.com/himdel/manageiq-ui-classic/commit/d7a7948bfff3ee5df5df3b26282a8008c7ac6884 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@martinpovolny martinpovolny merged commit 76a8d95 into ManageIQ:master Mar 7, 2019
@martinpovolny martinpovolny added this to the Sprint 107 Ending Mar 18, 2019 milestone Mar 7, 2019
@martinpovolny martinpovolny self-assigned this Mar 7, 2019
@himdel himdel deleted the custom-button-order branch March 8, 2019 10:27
@himdel
Copy link
Contributor Author

himdel commented Dec 3, 2019

ManageIQ/manageiq#18368 went in hammer after all,
so adding hammer/yes

https://bugzilla.redhat.com/show_bug.cgi?id=1770300 (not a full fix though)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants