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

Fix tagging for Edit Group and Provisioning screens #6235

Merged
merged 2 commits into from Oct 10, 2019

Conversation

PanSpagetka
Copy link
Contributor

@PanSpagetka PanSpagetka commented Sep 25, 2019

Because of changes in ManageIQ/react-ui-components#132 tagging wasn't working properly for Edit Group and Provisioning screens.

Links [Optional]

ManageIQ/react-ui-components#132
ManageIQ/react-ui-components#152 NEEDED TO BE MERGED FIRST
#5946

Steps for Testing/QA [Optional]

Compute -> Cloud -> Instance -> Lifecycle -> Provision -> Purpose tab
Cog wheel in the right corner -> Access Control -> Groups -> Edit some group

@hstastna
Copy link
Contributor

hstastna commented Sep 30, 2019

@PanSpagetka I've tested changes in this PR but I am still getting the same error, when adding some tag under My Company Tags tab when adding/editing a Group:

[----] F, [2019-09-30T15:25:29.895665 #16611:2ad396e03be0] FATAL -- : Error caught: [NoMethodError] undefined method `name' for nil:NilClass
/home/hstastna/manageiq/manageiq-ui-classic/app/controllers/ops_controller/ops_rbac.rb:1085:in `rbac_group_get_form_vars'
/home/hstastna/manageiq/manageiq-ui-classic/app/controllers/ops_controller/ops_rbac.rb:754:in `rbac_field_changed'
/home/hstastna/manageiq/manageiq-ui-classic/app/controllers/ops_controller/ops_rbac.rb:221:in `rbac_group_field_changed'

And for Purpose tab, it does not work, too. After I choose some value from the drop down, assigned tag displays for a second and then it suddenly disappears in front of my eyes and "No Assigned Tags" is displayed instead.

Am I doing anything wrong while testing this PR? Do I need to do any extra steps before testing the scenarios? Thanks.

@PanSpagetka
Copy link
Contributor Author

Ehm, sorry, you need ManageIQ/react-ui-components#152 I will mark it more clearly in description

@hstastna
Copy link
Contributor

hstastna commented Oct 8, 2019

@PanSpagetka So finally, ManageIQ/react-ui-components#152 is merged but I am still getting errors. Am I missing something?

@PanSpagetka
Copy link
Contributor Author

@hstastna I had to update package.json in this PR, now it should be finally working

@miq-bot
Copy link
Member

miq-bot commented Oct 9, 2019

Checked commits PanSpagetka/manageiq-ui-classic@be384d4~...a82f8fe with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@hstastna
Copy link
Contributor

hstastna commented Oct 9, 2019

So for proper testing, it is good to remove node_modules from manageiq-ui-classic and to run yarn and bin/webpack.

@hstastna
Copy link
Contributor

hstastna commented Oct 9, 2019

@miq-bot add_reviewer @hstastna

@miq-bot miq-bot requested a review from hstastna October 9, 2019 12:02
@hstastna
Copy link
Contributor

hstastna commented Oct 9, 2019

@miq-bot remove_label unmergeable

Copy link
Contributor

@hstastna hstastna left a comment

Choose a reason for hiding this comment

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

Adding tags works fine ✨ in both places (for Groups and also for Purpose tab), no errors.
However, it works differently in each of these places: While adding/editing a Group, I can select more values for the same Category, but under Purpose tab, I simply cannot. If I try to do it, the existing value is rewritten with the new one. Is this intentional? Thanks.
multiselect_tag

@PanSpagetka
Copy link
Contributor Author

@hstastna Thanks for testing. Yes, this is intentional. For Add/Edit group you can add more tags, but for the Provisioning -> Purpose you can't.

@mzazrivec mzazrivec self-assigned this Oct 10, 2019
@mzazrivec mzazrivec added this to the Sprint 122 Ending Oct 14, 2019 milestone Oct 10, 2019
@mzazrivec mzazrivec merged commit c464470 into ManageIQ:master Oct 10, 2019
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.

None yet

4 participants