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.

Sign up for GitHub

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

Jump to bottom

Update the User data only if the validation passes #3345

Merged
merged 2 commits into from
Feb 1, 2018
Merged

Update the User data only if the validation passes #3345

merged 2 commits into from
Feb 1, 2018

Conversation

lgalis
Copy link
Contributor

@lgalis lgalis commented Jan 30, 2018 •
edited
Loading

Update the User data only if the validation passes

Links

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

Steps for Testing/QA

  1. Create an RBAC user that is assigned to one or more groups
  2. Edit the RBAC user and remove them from all of their assigned groups
  3. Click Save
  4. After receiving the error "A User must be assigned to a Group", navigate away from the user's details page.
  5. Select the user in the RBAC accordion to view their details

@miq-bot miq-bot added the wip label Jan 30, 2018
@lgalis lgalis force-pushed the user_no_group_validation_fix branch from 952a378 to da82e1b Compare January 31, 2018 05:18
@lgalis lgalis force-pushed the user_no_group_validation_fix branch from da82e1b to 1161b36 Compare January 31, 2018 20:56
@lgalis lgalis changed the title [WIP] Update the User data only if the validation passes Update the User data only if the validation passes Jan 31, 2018
@lgalis
Copy link
Contributor Author

lgalis commented Jan 31, 2018

@h-kataria - please review

@lgalis lgalis force-pushed the user_no_group_validation_fix branch from 1161b36 to 62358a3 Compare January 31, 2018 21:08
@miq-bot
Copy link
Member

miq-bot commented Jan 31, 2018

Checked commits lgalis/manageiq-ui-classic@46fc429~...62358a3 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@miq-bot miq-bot removed the wip label Jan 31, 2018
h-kataria
h-kataria reviewed Jan 31, 2018
View reviewed changes
app/controllers/ops_controller/ops_rbac.rb
@@ -722,7 +722,7 @@ def rbac_edit_save_or_add(what, rbac_suffix = what)
when :user
record = @edit[:user_id] ? User.find_by(:id => @edit[:user_id]) : User.new
validated = rbac_user_validate?
rbac_user_set_record_vars(record)
rbac_user_set_record_vars(record) if validated
Copy link
Contributor

Choose a reason for hiding this comment

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

@lgalis, change looks good, but i am curious why is invalid record being updated, shouldn't it skip save if record is not valid at https://github.com/lgalis/manageiq-ui-classic/blob/62358a3c6baef44122cb963fab2e576678aac7ba/app/controllers/ops_controller/ops_rbac.rb#L736

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rbac_user_set_record_vars updates the user's HABTM miq_groups link directly, without save being called for the User, this is how Rails' has_many and has_and_belongs_to_many associations work.
I also do not think there's a reason to set the record values if the UI validation is not successful. ( The validation for the Role doesn ot have this issue because the miq_user_role's features are not assigned directly).

@h-kataria h-kataria self-assigned this Feb 1, 2018
@h-kataria
Copy link
Contributor

looks good.

@h-kataria h-kataria added this to the Sprint 79 Ending Feb 12, 2018 milestone Feb 1, 2018
@h-kataria h-kataria merged commit fbcf766 into ManageIQ:master Feb 1, 2018
@lgalis lgalis deleted the user_no_group_validation_fix branch February 1, 2018 15:28
simaishi pushed a commit that referenced this pull request Mar 7, 2018
@h-kataria @simaishi
Merge pull request #3345 from lgalis/user_no_group_validation_fix
7b10336 
Update the User data only if the validation passes
(cherry picked from commit fbcf766)

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

simaishi commented Mar 7, 2018

Gaprindashvili backport details:

$ git log -1
commit 7b10336b56f23335e55dc7cd55329ade756dc412
Author: Harpreet Kataria <[email protected]>
Date:   Thu Feb 1 10:28:09 2018 -0500

    Merge pull request #3345 from lgalis/user_no_group_validation_fix
    
    Update the User data only if the validation passes
    (cherry picked from commit fbcf7664ddb6d978d45473ba721c9e6f8be9c82d)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1552873

@ZitaNemeckova ZitaNemeckova mentioned this pull request Apr 4, 2018
Merged
@ZitaNemeckova ZitaNemeckova mentioned this pull request Apr 13, 2018
Merged
@lgalis lgalis mentioned this pull request May 4, 2018
Closed
@himdel himdel mentioned this pull request May 9, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@h-kataria h-kataria h-kataria left review comments

Assignees

@h-kataria h-kataria

Labels
bug gaprindashvili/backported settings
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lgalis @miq-bot @h-kataria @simaishi