Classification parent

[kbrock]

blocked:

• Use scope instead of parent_id => 0 #18304
• Tag#find_by_classification_name #18305
• update this PR to not be 0 and nil compatible (just stick with nil)
• Classification: use category scope #18361
• Classification.yml don't use parent_id=0 #18418 don't use parent_id=0 in classification files (and ignore it if it is in there)
• classification#parent_id = nil when no parents manageiq-schema#309 (need to be merged simultaneously)

Extracted from #17210
Thanks for pinging me on this one @Fryguy

Before:

Currently we use Classification#parent_id == 0 to mean there is a Category and it has no parent.

After

ActiveRecord has the convention of having parent_id == nil to mean there is no parent.
So we follow that convention: Classification#parent_id == nil when this is a Category and it has no parent.
We now pass around nil when there is no parent (we were passing 0). But we still handle passing a 0 in case some code is not properly updated across the various repos.

This PR:

• cleans up our use of name2tag.
• cleans up our interface for Tag.find_by_classification_name.
• relies less upon parent_id value, using more scopes.
• more lenient code handling parent_id of 0 or nil.

[Fryguy]

@kbrock There's a lot going on here, but I can't tell if this is all drive-by cleanups or required to get parent_id => nil working.

Also as I mentioned in the migration PR, I'd prefer we just cut off parent_id 0 altogether, with the migration of 0 to nil being the cutover point.

[kbrock]

Re: Rubocop

The rubocop comments don't make sense for this PR.

The .zero? cause null object errors and the find_by_name was introduced a long time ago and is not a simple find_by(:name => like the comment suggests

[kbrock]

WIP: splitting this up

[cben]

I did see your WIP message but finally found some time so reviewed first commit ("Tag#find_by_classification_name"). LGTM on that, with couple of optional suggestions.

[The root cause of many things that annoy me around namespaces is that Classification pretends it knows its namespace but actually gets it from slicing the name of the associated tag (unless it's a new classification that first knows its ns, then uses that to compute save_tag).
In an ideal world, categories would have a parent — the namespace — and a lot of code passing ns around could be unnecessary. That'd be a hugely invasive change, out of scope possibly forever.]

[cben]

So the hash contains (or might contain?) a "parent_id" key, and we get a parent arg as well?
Checking parent looks fine, but are the two always in sync? Below (L411) we create(classification) which would use classification["parent_id"], ignoring parent.

Looking at db/fixtures/classifications.yml, categories come with :parent_id: 0, and entries without :parent_id.
OK, I think I see. The recursive call goes into the other branch, which sets "parent_id" => parent.id.

=> Assuming the imported yaml is always rooted at categories, LGTM.
Consider also forcing "parent_id" => parent.id in the categories branch too.

P.S. db/fixtures/classifications.yml is not really relevant. That file is loaded by .seed which uses completely separate code 🤦‍♂️

[cben]

This import_from_yaml->import_from_hash code path is only used by tools/import_tags.rb. I don't think we have tests or examples what input looks like, although it's pretty obvious it should be symmetric to tools/export_tags.rb -> Classification.export_to_yaml. That one is always rooted at categories, omitting "parent_id" from entries => satisfies assumptions here 👍

There is also a 3rd completely separate importing code in lib/task_helpers/imports/tags.rb.
And a 4th completely separate https://github.com/rhtconsulting/cfme-rhconsulting-scripts/blob/master/rhconsulting_tags.rake
😕 🤦‍♂️ 😆
oh well, consolidating is totally not the goal here, I'm just amused.
EDIT: 3rd was based on 4th (#16983)

[cben]

• However you also need to update lib/task_helpers/exports/tags.rb, which has :parent_id => 0.
• lib/task_helpers/imports/tags.rb looks unaffected to me?

[cben]

orthogonal to this PR but valid optimization, avoids unnecessary find 👍

[kbrock]

yes, this was unnecessary

(but I can't bring myself to remove it :) )

[cben]

Looks great! 💯

Didn't look deep into Travis failures. Must be tested locally with schema change.
But one thing there stood out — Classification#ns method contains following logic before this change:

if category?
  @ns = tag2ns(tag.name)
else
  @ns = tag2ns(parent.tag.name) unless parent_id.nil?
end

Huh. So there are presently situations when parent_id is nil, which is distinct from parent_id==0 meaning a category?! 😕

[kbrock]

Huh. So there are presently situations when parent_id is nil, which is distinct from parent_id==0 meaning a category?! 😕
-- @cben

Yea, think that is present when creating records.
I seem to remember removing that check and a few specs blowing up.

Merging nil and 0 will make this case disappear.

re: travis failures

From what I can tell, I think this is failing travis because the database itself has a category_id default of 0. So they really need to be applied together.

But this assumption is also why I want to get as much code out of here.
So stuff doesn't sneak through.

[cben]

[I've gone silent partly because I hoped to look deeper into "present situations when parent_id is nil" but never found the time, but also because I wasn't sure if this is finished. Please ping me explicitly if you want another look.]

[miq-bot]

Checked commit kbrock@7c5f3d1 with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 3 offenses detected

app/models/classification.rb

• ❗ - Line 338, Col 61 - Naming/UncommunicativeMethodParamName - Method parameter must be at least 3 characters long.
• ❗ - Line 343, Col 63 - Naming/UncommunicativeMethodParamName - Method parameter must be at least 3 characters long.
• ❗ - Line 508, Col 44 - Naming/UncommunicativeMethodParamName - Method parameter must be at least 3 characters long.