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#parent_id = nil when no parents #309

Merged
merged 1 commit into from
Jun 13, 2019

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Nov 30, 2018

We used to have parent_id=0 when there was no parent.
Rails prefers to use a nil

This is in response to @Fryguy comment in ManageIQ/manageiq#17210

We used to have parent_id=0 when there was no parent.
Rails prefers to use a nil
@miq-bot
Copy link
Member

miq-bot commented Nov 30, 2018

Checked commit kbrock@2f21d18 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 2 offenses detected

db/migrate/20181130203334_classification_parent_null.rb

@cben
Copy link
Contributor

cben commented Dec 2, 2018

Kudos for not giving up on simplifying classifications! 🙇‍♂️

LGTM. Needs matching change in core of course.

@Fryguy
Copy link
Member

Fryguy commented Dec 14, 2018

@carbonin @bdunne Please review.

@kbrock Is there a correspnding change to core that sets the value to nil as well so new 0 values aren't introduced after this is merged?

@kbrock
Copy link
Member Author

kbrock commented Dec 18, 2018

@Fryguy do you want the code to handle the parent_id = 0 case, or do we assume the people have run the migrations? (I'm thinking we should handle parent_id = 0)

e.g.:

class Classification
  scope :is_category, -> { where(:parent_id => [0, nil]) }

  def category?
    parent_id == 0 || parent_id.nil?
  end
end

Currently it only supports :parent_id => 0. The alternative would be to change this to :parent_id => nil.

@Fryguy
Copy link
Member

Fryguy commented Dec 18, 2018

Once the migration is merged we can safely assume that the user has run through it. They can't use a release of the code without migrating first.

@bdunne bdunne merged commit efbba46 into ManageIQ:master Jun 13, 2019
@bdunne bdunne self-assigned this Jun 13, 2019
@bdunne bdunne added the data label 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 branch June 13, 2019 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants