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.

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

Classification.yml don't use parent_id=0 #18418

Merged
merged 1 commit into from
Jun 13, 2019

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Jan 31, 2019

For seeds and import/exports, the levels explain the nesting, so
there is no need to pass parent_id: 0

This is part of the Classification.parent_id initiative #18301

@kbrock kbrock requested a review from cben January 31, 2019 00:17
@kbrock kbrock mentioned this pull request Jan 31, 2019
6 tasks
@kbrock kbrock force-pushed the classification_parent_yml branch 2 times, most recently from e582c63 to b226d60 Compare January 31, 2019 00:44
Copy link
Contributor

@cben cben left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@kbrock
Copy link
Member Author

kbrock commented Feb 1, 2019

@cben I truncated classifications, ran seeds and we had no parent_id => nil records.
I ran a second time and it didn't seem to update anything (this is good)

Any other tests you think would be good?

app/models/classification.rb Outdated Show resolved Hide resolved
@Fryguy
Copy link
Member

Fryguy commented Feb 5, 2019

Will customers have exported data with non-zero parent_id values, and if so, will this de-parent those on re-import?

@cben
Copy link
Contributor

cben commented Feb 7, 2019

Will customers have exported data with non-zero parent_id values

I'm aware of 3 export paths, all converge to export_to_array which always omitted parent_id on non-categories (since Initial commit). Also, they're always structured under entries:.

For seeds and import/exports, the levels explain the nesting, so
there is no need to pass parent_id

This is part of the Classification.parent_id initiative

more factory methods that do not need to specify parent_id

Remove seed update_all

The categories are now properly with parent_id
no need to update_all after seeding
@kbrock
Copy link
Member Author

kbrock commented Feb 7, 2019

@Fryguy ok - removed the hash delete. Anything else?

Thanks for the followup @cben

@miq-bot
Copy link
Member

miq-bot commented Feb 12, 2019

Checked commit kbrock@6c8d9ba with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 1 offense detected

app/models/classification.rb

@kbrock
Copy link
Member Author

kbrock commented Feb 25, 2019

@Fryguy Think this is good to go. Anything else?

@kbrock
Copy link
Member Author

kbrock commented Mar 20, 2019

@Fryguy If this is too much, I can pull out one or two small nuggets and try again.

@kbrock
Copy link
Member Author

kbrock commented Apr 10, 2019

@Fryguy if you would prefer, I can remove the yaml file changes and just put in the code changes.

These changes need to be made for removing parent_id=0 stuff.
But regardless of those changes, these simplify the code and seem to make more sense to me.

We'll probably never put into place the change from parent_id=0 => parent_id=nil. So if you also want to punt on this PR just give the word.

Otherwise, let me know what I can do here to make this easier to grok

@cben
Copy link
Contributor

cben commented May 21, 2019

@Fryguy These are not very important but they do simplify a subtle corner and would be nice to land :shipit:

@bdunne bdunne merged commit d1f1db7 into ManageIQ:master Jun 13, 2019
@bdunne bdunne self-assigned this Jun 13, 2019
@bdunne bdunne added this to the Sprint 114 Ending Jun 24, 2019 milestone Jun 13, 2019
@kbrock kbrock deleted the classification_parent_yml branch June 13, 2019 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants