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

Enable to add a new Group after first unsuccessful adding #3164

Merged
merged 1 commit into from Jan 5, 2018

Conversation

hstastna
Copy link
Contributor

@hstastna hstastna commented Jan 4, 2018

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1523214

Enable to add a new Group after first unsuccessful adding of the Group because
of missing Name, in Administrator > Configuration, Access Control tab > Groups.


Explanation:
The two simple conditions were added to prevent the error (see the BZ above), which was caused here:
https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/ops_controller/ops_rbac.rb#L1425
@edit[:new][:filter_expression] was nil, so the error occurred. It was set to nil here:
https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/ops_controller/ops_rbac.rb#L1252
The method rbac_group_set_record_vars is called right after the validation (see https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/ops_controller/ops_rbac.rb#L728) and even if it is not known if the validation was successful, the group record variables are set to new values, which is good only after successful validation (because of the if-else conditions in rbac_group_set_record_vars which are very specific). So the condition https://github.com/ManageIQ/manageiq-ui-classic/compare/master...hstastna:Add_Group_empty_name_validate?expand=1#diff-8078c760e06b820719715450b2a112e0R729 was added and also validation was extended by adding one simple line https://github.com/ManageIQ/manageiq-ui-classic/compare/master...hstastna:Add_Group_empty_name_validate?expand=1#diff-8078c760e06b820719715450b2a112e0R1424 to consider also the name of a new group.

This solution is simple and we don't have to reset variables @edit[:new][:filters] and @edit[:new][:filter_expression] (changed in rbac_group_set_record_vars method), if validation is not successful or if anything else breaks when creating/adding a new group (and we don't have to remember their values: we had to remember/save them, if we wanted to reset the variables after unsuccessful adding a group). And also none of the variables in the method are set (using fix in this PR) if rbac_group_validate? returns false, which is good. They are still set in the next try of adding a new group, after successful validation.

group_add

Before:
group_before

After:
group_after

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1523214

Enable to add a new Group after first unsuccessful adding of the Group
because of missing Name, in Administrator > Configuration, Access Control
tab > Groups.
@hstastna hstastna changed the title Enable to add a new Group after first unsuccessful adding [WIP] Enable to add a new Group after first unsuccessful adding Jan 4, 2018
@hstastna
Copy link
Contributor Author

hstastna commented Jan 4, 2018

@miq-bot add_label bug, gaprindashvili/yes

@miq-bot
Copy link
Member

miq-bot commented Jan 4, 2018

Checked commit hstastna@76b8fa1 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@hstastna hstastna changed the title [WIP] Enable to add a new Group after first unsuccessful adding Enable to add a new Group after first unsuccessful adding Jan 5, 2018
@miq-bot miq-bot removed the wip label Jan 5, 2018
@mzazrivec mzazrivec assigned mzazrivec and unassigned h-kataria Jan 5, 2018
@mzazrivec mzazrivec added this to the Sprint 77 Ending Jan 15, 2018 milestone Jan 5, 2018
@mzazrivec mzazrivec merged commit 996a978 into ManageIQ:master Jan 5, 2018
simaishi pushed a commit that referenced this pull request Jan 5, 2018
Enable to add a new Group after first unsuccessful adding
(cherry picked from commit 996a978)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1531621
@simaishi
Copy link
Contributor

simaishi commented Jan 5, 2018

Gaprindashvili backport details:

$ git log -1
commit 937dfe5954b5c4ca1ec0d662337991ee23e47532
Author: Milan Zázrivec <mzazrivec@redhat.com>
Date:   Fri Jan 5 12:14:55 2018 +0100

    Merge pull request #3164 from hstastna/Add_Group_empty_name_validate
    
    Enable to add a new Group after first unsuccessful adding
    (cherry picked from commit 996a978c44f30a5fd0ed9db1268662963e953d87)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1531621

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

6 participants