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

Add deep symbolization to CustomButton visibility field #206

Merged

Conversation

AparnaKarve
Copy link
Contributor

The backend expects the CustomButton visibility field to be deep symbolized in order to perform Custom Button related operations.

Hence appropriate adjustment had to be made in the API as well.

@AparnaKarve
Copy link
Contributor Author

AparnaKarve commented Nov 13, 2017

This fix is required for ManageIQ/manageiq-ui-classic#2569 and ManageIQ/manageiq-ui-classic#2636

@AparnaKarve
Copy link
Contributor Author

/cc @abellotti

@abellotti
Copy link
Member

Thanks @AparnaKarve this is fine, is this needed for gaprindashvili or just master ?

@AparnaKarve
Copy link
Contributor Author

@abellotti This is needed for gaprindashvili

expect(response).to have_http_status(:ok)
expect(response.parsed_body).to include(expected)
expect(custom_button.visibility[:roles]).to eq(['_ALL_'])
Copy link
Member

Choose a reason for hiding this comment

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

would prefer that we check the custom_button we have to start with, maybe just expect(cb.reload.visibility[:roles]).to ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have addressed this in the last commit (5cb4355)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

commit revised further - 6d268dc

@AparnaKarve AparnaKarve force-pushed the add_deep_symbolizations_to_cb_visibility branch 2 times, most recently from 5cb4355 to 6d268dc Compare November 13, 2017 21:09
@abellotti
Copy link
Member

Thanks @AparnaKarve for the update 👍 will merge when 🍏

@AparnaKarve AparnaKarve force-pushed the add_deep_symbolizations_to_cb_visibility branch from 6d268dc to bdf948d Compare November 13, 2017 21:19
@miq-bot
Copy link
Member

miq-bot commented Nov 13, 2017

Checked commits AparnaKarve/manageiq-api@81a4fca~...bdf948d with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@abellotti abellotti added this to the Sprint 73 Ending Nov 13, 2017 milestone Nov 13, 2017
@abellotti abellotti merged commit 85c9383 into ManageIQ:master Nov 13, 2017
@AparnaKarve
Copy link
Contributor Author

@abellotti Thank you!

simaishi pushed a commit that referenced this pull request Nov 14, 2017
…b_visibility

Add deep symbolization to CustomButton `visibility` field
(cherry picked from commit 85c9383)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit d03bc5da2280a46685053013071d0855a0b402f5
Author: Alberto Bellotti <abellotti@users.noreply.github.com>
Date:   Mon Nov 13 16:44:25 2017 -0500

    Merge pull request #206 from AparnaKarve/add_deep_symbolizations_to_cb_visibility
    
    Add deep symbolization to CustomButton `visibility` field
    (cherry picked from commit 85c938362984af081d6eb16e92fc874ae471e09c)

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