convert tag.category to a regular association #17418

kbrock · 2018-05-14T15:31:57Z

Use relationships instead of virtual relationships.

relationships are native to rails

pulled out of #17210
(probably retiring that PR, but want this change)

kbrock · 2018-05-15T20:49:20Z

/cc @Fryguy you asked me to pull this out - sound good to you?

Fryguy · 2018-05-21T19:10:38Z

spec/models/ems_refresh/save_inventory/save_tags_inventory_spec.rb

@@ -6,7 +6,7 @@ def mapped_tag(category_name, tag_name)
    mapping = FactoryGirl.create(:tag_mapping_with_category,
                                 :category_name        => category_name,
                                 :category_description => category_name)
-    category = mapping.tag.category
+    category = mapping.tag.classification


Why does this have to change? The code is looking for the category, so it's surprising to ask for the classification

Spoke with @cben about this (author of the test)

There is currently a bug in #category
name_path.split('/').first means it will return a category or classification.

This test was relying upon a bug in #category. Changing this line s/category/classification still passes the tests.

In this test, there is category/tag (which really translates to classification/category)

cben · 2018-05-22T08:52:09Z

Nice. However manageiq-providers-kubernetes & -openshift are failing with this, it seems there are tests there relying on same bug. Let me see if I can change them...

cben · 2018-05-22T10:17:49Z

A more detailed explanation, because the terminology in above discussion is IMHO overloaded:
There are category/entry Tags, and category/entry Classifications:

                                   .parent
   category Classification "cat"   <----------   entry Classification "ent"
             |^                ^                        |^
??       .tag||.classification  \_______________    .tag||.classification
  \          v|                     .category   \       v|
   category Tag /managed/cat                     entry Tag /managed/cat/ent

Tag#category is supposed to get the "uncle", the category Classification from an entry Tag.

The question is what it does / should do given a category Tag?
@kbrock can you add a Tag#category unit test for this case?
The current behavior was returning the category Classification anyway; I think new behavior will be nil?

Currently .category on a category Tag ("/managed/cat") happens to work same as .classification, but it's ill-defined and will stop working after ManageIQ/manageiq#17418. .parent category Classification "cat" <---------- entry Classification "ent" |^ ^ |^ .tag||.classification \_______________ .tag||.classification v| .category \ v| category Tag /managed/cat entry Tag /managed/cat/ent

cben · 2018-05-22T13:01:17Z

mapping.tag is a category Tag, calling .category is ill-defined manageiq-providers-kubernetes#255 should be merged before this.
mapping.tag is a category Tag, calling .category is ill-defined manageiq-providers-openshift#98 should be merged before this.
mapping.tag is a category Tag, calling .category is ill-defined manageiq-ui-classic#3973 should be merged before this.
manageiq-providers-amazon tests unaffected
manageiq-providers-azure tests unaffected
manageiq-api tests unaffected (spec/requests/tags_spec.rb calls .category, but only deals in entry Tags)

Currently .category on a category Tag ("/managed/cat") happens to work same as .classification, but it's ill-defined and will stop working after ManageIQ/manageiq#17418. .parent category Classification "cat" <---------- entry Classification "ent" |^ ^ |^ .tag||.classification \_______________ .tag||.classification v| .category \ v| category Tag /managed/cat entry Tag /managed/cat/ent

For a regular tag (before and after): node.classification.parent == node.category node.classification != node.category There is an edge case for category tags that has changed: This edge case is used in tests. Note: category_tag = node.category.tag before: category_tag behaves differently from a category_tag category_tag.classification.parent != category_tag.category category_tag.classification == category_tag.category after: category_tag behaves the same as a category_tag category_tag.classification.parent == category_tag.category == nil category_tag.classification != category_tag.category

kbrock · 2018-05-22T16:38:32Z

Thanks @cben for both your graph and the PRs for specs. Both help a lot

Yes, when looking at the category node (dotted border) it gets a little confusing.
I have been going under a few base assumptions:

node.classification.parent == node.category
node.classification != node.category

So I've come to the conclusion that node.category.tag.category = nil

I've added a test to show the expected behavior.

thnx to https://graphviz.glitch.me/

digraph G {
node [shape=Mrecord]
nil [label="nil"]
c_class [label="category classification 'cat'"]
e_class [label="entry classification 'ent'"]
c_tag   [label="category tag '/managed/cat'" style="dashed"]
e_tag   [label="entry tag '/managed/cat/ent'" style="bold"]
c_class -> e_tag   [label="category" dir="back" constraint="false"]
nil -> c_class [label="parent" dir="back" constraint="false"]
c_class -> e_class [label="parent" dir="back" constraint="false"]
c_class -> c_tag   [label="tag" constraint="false"]
c_class -> c_tag   [label="classification" dir="back"]
e_class -> e_tag   [label="tag"]
e_class -> e_tag   [label="classification" dir="back"]
nil     -> c_tag   [label="category" dir="back" constraint="false"]
}

miq-bot · 2018-05-22T16:42:48Z

Checked commit kbrock@fe708cd with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 👍

cben · 2018-05-22T21:13:32Z

assumptions make sense 👍. LGTM.

cben · 2018-05-28T09:49:16Z

dependencies all landed

kbrock added the refactoring label May 14, 2018

Fryguy reviewed May 21, 2018

View reviewed changes

cben mentioned this pull request May 22, 2018

mapping.tag is a category Tag, calling .category is ill-defined ManageIQ/manageiq-providers-kubernetes#255

Merged

cben mentioned this pull request May 22, 2018

mapping.tag is a category Tag, calling .category is ill-defined ManageIQ/manageiq-providers-openshift#98

Merged

cben mentioned this pull request May 22, 2018

mapping.tag is a category Tag, calling .category is ill-defined ManageIQ/manageiq-ui-classic#3973

Merged

kbrock force-pushed the tag_category branch from c905138 to fe708cd Compare May 22, 2018 16:37

Fryguy merged commit f1ef359 into ManageIQ:master Jun 19, 2018

Fryguy added this to the Sprint 89 Ending Jul 2, 2018 milestone Jun 19, 2018

Fryguy self-assigned this Jun 19, 2018

kbrock deleted the tag_category branch August 21, 2018 21:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

convert tag.category to a regular association #17418

convert tag.category to a regular association #17418

kbrock commented May 14, 2018

kbrock commented May 15, 2018

Fryguy May 21, 2018

kbrock May 21, 2018 •

edited

Loading

cben commented May 22, 2018 •

edited

Loading

cben commented May 22, 2018 •

edited

Loading

cben commented May 22, 2018 •

edited

Loading

kbrock commented May 22, 2018 •

edited

Loading

miq-bot commented May 22, 2018

cben commented May 22, 2018

cben commented May 28, 2018

convert tag.category to a regular association #17418

convert tag.category to a regular association #17418

Conversation

kbrock commented May 14, 2018

kbrock commented May 15, 2018

Fryguy May 21, 2018

Choose a reason for hiding this comment

kbrock May 21, 2018 • edited Loading

Choose a reason for hiding this comment

cben commented May 22, 2018 • edited Loading

cben commented May 22, 2018 • edited Loading

cben commented May 22, 2018 • edited Loading

kbrock commented May 22, 2018 • edited Loading

miq-bot commented May 22, 2018

cben commented May 22, 2018

cben commented May 28, 2018

kbrock May 21, 2018 •

edited

Loading

cben commented May 22, 2018 •

edited

Loading

cben commented May 22, 2018 •

edited

Loading

cben commented May 22, 2018 •

edited

Loading

kbrock commented May 22, 2018 •

edited

Loading