Lookup a category_id if a tag control passes it in #16965

syncrou · 2018-02-07T18:53:25Z

When editing a tag control for service dialogs, we were always looking up by the name
coming in from the dialog. For a new record this sufficed, for an edit it caused edits to fail.

This changes the order of lookups to first check the category_id if it comes in,
and to fallback to the name if there is no category_id

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

syncrou · 2018-02-07T18:58:12Z

@miq-bot assign @eclarizio

@miq-bot add_label bug, services, gaprindashvili/yes

cc - @d-m-u @gmcculloug

d-m-u · 2018-02-07T19:13:54Z

app/models/dialog_field_importer.rb

-    if category_name
+    category_id = dialog_field_attributes["options"][:category_id]
+    if category_id
+      dialog_field.category = dialog_field_attributes["options"][:category_id]


Stupid nit, but doesn't it also need to set the description in this case?

d-m-u · 2018-02-07T21:00:24Z

app/models/dialog_field_importer.rb

-    if category_name
+    category_id = dialog_field_attributes["options"][:category_id]
+    if category_id
+      dialog_field.category = category_id


Dunno if this needs the description as well.

@eclarizio - Does it need the description?

No, assigning the id is all that is needed

gmcculloug · 2018-02-07T21:09:08Z

app/models/dialog_field_importer.rb

@@ -32,12 +32,15 @@ def import_field(dialog_field_attributes)

  def set_category_for_tag_control(dialog_field, dialog_field_attributes)
    category_name = dialog_field_attributes["options"][:category_name]
-    if category_name
+    category_id = dialog_field_attributes["options"][:category_id]
+    if category_id


I can see how this would work in an edit scenario where you expect the tag category to already exist but doesn't this method also get used when importing dialogs between completely different environments?

The category ID may exist in the new environment but it could point to a completely different tag category. Also, this is just checking that the ID exists in the field data, but there is no check that the category exists when importing into a different environment.

cc @eclarizio

@gmcculloug You're right

@syncrou What I think you will need to do is if the category id exists, look up the category and then compare the name/description. The only problem is that in the edit scenario, the name/description that is getting passed in is incorrect as we determined when we were debugging and was the whole source of this issue. I think the best solution in this case would be to modify the ui-components here https://github.com/ManageIQ/ui-components/blob/master/src/dialog-editor/components/modal-field-template/tag-control.html#L26 and add an ng-change method that ends up setting the id, name, and description.

@eclarizio - Thanks! - I'll get those changes in.

eclarizio · 2018-02-08T15:35:27Z

app/models/dialog_field_importer.rb

-    if category_name
+    category_id = dialog_field_attributes["options"][:category_id]
+    if category_id
+      dialog_field.category = category_id


No, assigning the id is all that is needed

eclarizio · 2018-02-08T16:05:54Z

app/models/dialog_field_importer.rb

@@ -32,12 +32,15 @@ def import_field(dialog_field_attributes)

  def set_category_for_tag_control(dialog_field, dialog_field_attributes)
    category_name = dialog_field_attributes["options"][:category_name]
-    if category_name
+    category_id = dialog_field_attributes["options"][:category_id]
+    if category_id


@gmcculloug You're right

@syncrou What I think you will need to do is if the category id exists, look up the category and then compare the name/description. The only problem is that in the edit scenario, the name/description that is getting passed in is incorrect as we determined when we were debugging and was the whole source of this issue. I think the best solution in this case would be to modify the ui-components here https://github.com/ManageIQ/ui-components/blob/master/src/dialog-editor/components/modal-field-template/tag-control.html#L26 and add an ng-change method that ends up setting the id, name, and description.

syncrou · 2018-02-08T16:15:26Z

@miq-bot add_label blocker

syncrou · 2018-02-08T22:10:42Z

Rubocop complaint is due to this code where find_by_name is overridden.

gmcculloug · 2018-02-08T23:05:38Z

app/models/dialog_field_importer.rb

+  def adjust_category(opts)
+    return nil if opts[:category_description].nil?
+    description = opts[:category_description]
+    real_category = if opts[:category_id] && description


Thinking about the import operation and where these values come from the two variables description and real_category are not all that intuitive.

What do you think about: import_description and import_category?

I like import_category.

My thought on description was to use that name instead of category_description, to differentiate that we need a description as our control element. Without a description there is no category to adjust for. Though, if that logic were to follow through, then category should suffice as opposed to import_category and the circle of discussion continues.

Discussed further with @syncrou and agree that category (instead of real_category) is a better variable name here.

gmcculloug · 2018-02-09T17:52:55Z

@eclarizio Please review after changes were pushed. Update your approval status if you are good with the changes.

eclarizio

I'm guessing this will need the corresponding ui-components repo change that passes in the category_id, category_description, and category_name, otherwise it won't work because currently all that is passed in is the category_id, so the new line:

return nil if opts[:category_description].nil?

will always be hit and the category won't update properly, I think.

eclarizio · 2018-02-09T19:48:51Z

app/models/dialog_field_importer.rb

+                 Category.find(opts[:category_id])
+               elsif opts[:category_name] && description
+                 Category.find_by_name(opts[:category_name])
+               end


Do we need to do && description for the if and elsif conditions? We're already short circuiting on line 38 and that is ensuring that we have a description, right?

When editing a tag control for service dialogs, we were always looking up by the name coming in from the dialog. For a new record this sufficed, for an edit it caused edits to fail. This changes the order of lookups to first check the category_id if it comes in, and to fallback to the name if there is no category_id https://bugzilla.redhat.com/show_bug.cgi?id=1542590

eclarizio

👍

gmcculloug · 2018-02-09T20:08:16Z

Thanks @eclarizio, you've helped bring the cognitive complexity of that method to 5 or less! 🎉

miq-bot · 2018-02-09T20:08:22Z

Checked commit syncrou@571572e with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

app/models/dialog_field_importer.rb

❗ - Line 42, Col 18 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_name.

…ntrol Lookup a category_id if a tag control passes it in (cherry picked from commit 3153645) https://bugzilla.redhat.com/show_bug.cgi?id=1544849

simaishi · 2018-02-13T18:04:33Z

Gaprindashvili backport details:

$ git log -1
commit 16e829b168f5c1fb305efcafbb6a759f2c33a6a2
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Fri Feb 9 16:15:56 2018 -0500

    Merge pull request #16965 from syncrou/handle_category_id_in_a_tag_control
    
    Lookup a category_id if a tag control passes it in
    (cherry picked from commit 3153645674caa9fe76690d56212bdf0e2a74cf9f)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1544849

miq-bot assigned eclarizio Feb 7, 2018

miq-bot added bug gaprindashvili/yes lifecycle/services labels Feb 7, 2018

syncrou force-pushed the handle_category_id_in_a_tag_control branch 2 times, most recently from cd5afca to f2e35d8 Compare February 7, 2018 19:17

d-m-u reviewed Feb 7, 2018

View reviewed changes

syncrou force-pushed the handle_category_id_in_a_tag_control branch 2 times, most recently from ada950f to 41f9449 Compare February 7, 2018 20:01

d-m-u reviewed Feb 7, 2018

View reviewed changes

gmcculloug reviewed Feb 7, 2018

View reviewed changes

eclarizio suggested changes Feb 8, 2018

View reviewed changes

miq-bot added the blocker label Feb 8, 2018

syncrou force-pushed the handle_category_id_in_a_tag_control branch 2 times, most recently from 39ef21c to 54daa52 Compare February 8, 2018 21:50

syncrou changed the title ~~Lookup a category_id if a tag control passes it in~~ [DO NOT MERGE] Lookup a category_id if a tag control passes it in Feb 8, 2018

gmcculloug reviewed Feb 8, 2018

View reviewed changes

syncrou force-pushed the handle_category_id_in_a_tag_control branch from 54daa52 to 4b66fbe Compare February 9, 2018 14:36

syncrou changed the title ~~[DO NOT MERGE] Lookup a category_id if a tag control passes it in~~ Lookup a category_id if a tag control passes it in Feb 9, 2018

eclarizio suggested changes Feb 9, 2018

View reviewed changes

syncrou force-pushed the handle_category_id_in_a_tag_control branch from 4b66fbe to 571572e Compare February 9, 2018 20:01

eclarizio approved these changes Feb 9, 2018

View reviewed changes

gmcculloug merged commit 3153645 into ManageIQ:master Feb 9, 2018

gmcculloug added this to the Sprint 79 Ending Feb 12, 2018 milestone Feb 9, 2018

romanblanco mentioned this pull request Feb 13, 2018

Set ng-change for tag control options ManageIQ/ui-components#253

Merged

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Feb 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lookup a category_id if a tag control passes it in #16965

Lookup a category_id if a tag control passes it in #16965

syncrou commented Feb 7, 2018 •

edited by simaishi

Loading

syncrou commented Feb 7, 2018

d-m-u Feb 7, 2018

d-m-u Feb 7, 2018

syncrou Feb 7, 2018

eclarizio Feb 8, 2018

gmcculloug Feb 7, 2018

eclarizio Feb 8, 2018

syncrou Feb 8, 2018

eclarizio Feb 8, 2018

eclarizio Feb 8, 2018

syncrou commented Feb 8, 2018

syncrou commented Feb 8, 2018

gmcculloug Feb 8, 2018

syncrou Feb 8, 2018 •

edited

Loading

gmcculloug Feb 9, 2018

gmcculloug commented Feb 9, 2018

eclarizio left a comment

eclarizio Feb 9, 2018

eclarizio left a comment

gmcculloug commented Feb 9, 2018

miq-bot commented Feb 9, 2018

simaishi commented Feb 13, 2018

Lookup a category_id if a tag control passes it in #16965

Lookup a category_id if a tag control passes it in #16965

Conversation

syncrou commented Feb 7, 2018 • edited by simaishi Loading

syncrou commented Feb 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syncrou commented Feb 8, 2018

syncrou commented Feb 8, 2018

Choose a reason for hiding this comment

syncrou Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmcculloug commented Feb 9, 2018

eclarizio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eclarizio left a comment

Choose a reason for hiding this comment

gmcculloug commented Feb 9, 2018

miq-bot commented Feb 9, 2018

simaishi commented Feb 13, 2018

syncrou commented Feb 7, 2018 •

edited by simaishi

Loading

syncrou Feb 8, 2018 •

edited

Loading