Remove unnecessary validation before clicking on Add/Save while adding/editing a Group or Role #6169

hstastna · 2019-09-09T14:10:05Z

This PR removes unnecessary code related to adding/editing a Group or Role because in older fields like in our cases and validation is usually provided AFTER clicking on Add/Save button - the same as in other screens => more consistency. Add/Save button respond now on any changes, and not also on the information if all the required fields are set.

This PR also fixes displaying extra flash message (Description can't be blank) which accidentally appears if we fill in only Description and click on Add button, while adding new Group. Caused by: #3164 The solution is setting Description, Role and/or Tenant (if it was chosen) even if rbac_group_validate? was not true. This issue is reproducible after applying the first commit of this PR (removing 'bad' variable).

More:
Related PR where we were dealing with the same issue: #5972 (comment)
Older PR which was adding bad variable: ManageIQ/manageiq#4566

After:

Note:
Choosing Project/Tenant is not required. If user is signed it, its Tenant is chosen by default. See:
https://github.com/ManageIQ/manageiq/blob/33a3327b395700ce245f9f770b4277e0ce08d2c2/app/models/mixins/tenancy_mixin.rb#L16

himdel · 2019-09-10T13:45:13Z

app/controllers/ops_controller/ops_rbac.rb

@@ -1411,7 +1404,7 @@ def rbac_group_validate?
    @assigned_filters = [] if @edit[:new][:filters].empty? || @edit[:new][:use_filter_expression]
    @filter_expression = [] if @edit[:new][:filter_expression].empty? || @edit[:new][:use_filter_expression] == false
    if @edit[:new][:role].nil? || @edit[:new][:role] == ""
-      add_flash(_("A User Group must be assigned a Role"), :error)
+      add_flash(_("A Role must be assigned to a Group"), :error)


I think this should be "A Group must be assigned a Role".
But if you want to switch it around, "A Role must be assigned to this Group", otherwise, it sounds like "fix the role", not "fix the group".

For me "A Group must be assigned a Role" does not make sense. I think there's still missing something. So I am better with "A Role must be assigned to this Group". Or "A Group must HAVE assigned a Role".

I'm fine with both of those :) Thanks :)

himdel · 2019-09-10T13:46:51Z

app/controllers/ops_controller/ops_rbac.rb

@@ -670,6 +670,7 @@ def rbac_edit_save_or_add(what, rbac_suffix = what)
    when :group then
      record = @edit[:group_id] ? MiqGroup.find_by(:id => @edit[:group_id]) : MiqGroup.new
      validated = rbac_group_validate?
+      rbac_group_set_record_description_role(record) # Set new Description, Role and Tenant for a new Group


first validate, then set extra fields?

Shouldn't this be the other way around?

This was like that, originally. And yes, it looks weird but the logic behind this is that in rbac_group_validate?, it is just checked that user filled in required fields like Description and Role. @edit[:new] is checked there, not record. And if everything is ok, then record is set according to what user filled in and then saved.

My change here is that I am putting the new info which is required (Description, Role, Tenant) to record even if rbac_group_validate? fails. It fails if any of required fields is missing, not if all of them. And if it fails, rbac_group_set_record_vars wouldn't be called so record would miss ALL the info user filled in properly. So then later here, record.description would be nil, instead of the name user filled in. This leads to displaying error message about missing Description which does not make sense because user filled in Description (and forgot to choose a role, for example).

Aah, perfect, thanks! :)

himdel · 2019-09-10T13:51:55Z

LGTM I think 👍 ,
except the new message is a bit confusing.

(Not tested yet)

himdel · 2019-09-10T15:30:59Z

Test failure looks unrelated, maybe a rebase? :)

h-kataria · 2019-09-10T22:37:57Z

LGTM I think ,
except the new message is a bit confusing.

(Not tested yet)

@hstastna @himdel How about just to be consistent with other screens "Role is required"

hstastna · 2019-09-11T08:53:04Z

@h-kataria Good point! I like it. Let's be more consistent. Where else can I check what's happening if I forget to add any Role? Thank you. ❇️

Remove unneccessary code related to adding/editing a Group

Remove the flash message which did not make sense to be displayed if user filled in a description and pressed Add button while adding new Group.

miq-bot · 2019-09-18T15:00:10Z

Checked commits hstastna/manageiq-ui-classic@f93d713~...db57f6e 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. 👍

himdel · 2019-09-23T09:13:51Z

Sorry for the delay, merging :)

miq-bot added the wip label Sep 9, 2019

hstastna changed the title ~~[WIP] Remove unnecessary bad variable and validation before clicking on Add/Save while adding/editing a Group or Role~~ [WIP] Remove unnecessary validation before clicking on Add/Save while adding/editing a Group or Role Sep 9, 2019

hstastna force-pushed the Remove_bad_ops_rbac branch 7 times, most recently from f74e5d1 to 98ad6ab Compare September 10, 2019 11:46

hstastna changed the title ~~[WIP] Remove unnecessary validation before clicking on Add/Save while adding/editing a Group or Role~~ Remove unnecessary validation before clicking on Add/Save while adding/editing a Group or Role Sep 10, 2019

miq-bot removed the wip label Sep 10, 2019

himdel reviewed Sep 10, 2019

View reviewed changes

hstastna force-pushed the Remove_bad_ops_rbac branch from 98ad6ab to 65e24ce Compare September 10, 2019 14:13

hstastna force-pushed the Remove_bad_ops_rbac branch from 65e24ce to 4813cff Compare September 10, 2019 15:43

Hilda Stastna added 2 commits September 18, 2019 16:42

Remove unnecessary variable and validation before clicking on Add/Save

f93d713

Remove unneccessary code related to adding/editing a Group

Remove extra flash message 'Description can't be blank'

db57f6e

Remove the flash message which did not make sense to be displayed if user filled in a description and pressed Add button while adding new Group.

hstastna force-pushed the Remove_bad_ops_rbac branch from 4813cff to db57f6e Compare September 18, 2019 14:42

hstastna mentioned this pull request Sep 19, 2019

Inconsistencies regarding responding of Add/Save button while adding/editing Group #5941

Closed

himdel self-assigned this Sep 23, 2019

himdel added the bug label Sep 23, 2019

himdel added this to the Sprint 121 Ending Sep 30, 2019 milestone Sep 23, 2019

himdel merged commit b183ad3 into ManageIQ:master Sep 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unnecessary validation before clicking on Add/Save while adding/editing a Group or Role #6169

Remove unnecessary validation before clicking on Add/Save while adding/editing a Group or Role #6169

hstastna commented Sep 9, 2019 •

edited

Loading

himdel Sep 10, 2019

hstastna Sep 10, 2019 •

edited

Loading

himdel Sep 10, 2019

himdel Sep 10, 2019

hstastna Sep 10, 2019 •

edited

Loading

himdel Sep 10, 2019

himdel commented Sep 10, 2019

himdel commented Sep 10, 2019

h-kataria commented Sep 10, 2019

hstastna commented Sep 11, 2019 •

edited

Loading

miq-bot commented Sep 18, 2019

himdel commented Sep 23, 2019

Remove unnecessary validation before clicking on Add/Save while adding/editing a Group or Role #6169

Remove unnecessary validation before clicking on Add/Save while adding/editing a Group or Role #6169

Conversation

hstastna commented Sep 9, 2019 • edited Loading

himdel Sep 10, 2019

Choose a reason for hiding this comment

hstastna Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

himdel Sep 10, 2019

Choose a reason for hiding this comment

himdel Sep 10, 2019

Choose a reason for hiding this comment

hstastna Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

himdel Sep 10, 2019

Choose a reason for hiding this comment

himdel commented Sep 10, 2019

himdel commented Sep 10, 2019

h-kataria commented Sep 10, 2019

hstastna commented Sep 11, 2019 • edited Loading

miq-bot commented Sep 18, 2019

himdel commented Sep 23, 2019

hstastna commented Sep 9, 2019 •

edited

Loading

hstastna Sep 10, 2019 •

edited

Loading

hstastna Sep 10, 2019 •

edited

Loading

hstastna commented Sep 11, 2019 •

edited

Loading