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.

Sign up for GitHub

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

Jump to bottom

[WIP] Remove Category - use Classification instead #17210

Closed
wants to merge 1 commit into from
Closed

[WIP] Remove Category - use Classification instead #17210

wants to merge 1 commit into from

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Mar 27, 2018 •
edited
Loading

The high level goal is to have tags that are hierarchy aware.
So we will merge Classification and Category into Tag.

Since Category < Classification, this PR takes an easy step and removes Category.

/cc @jrafanie @Fryguy any opinions?

@kbrock
Copy link
Member Author

kbrock commented Mar 27, 2018

rubocop: currently find_by_name is overridden - so we need to use that version for now. thanks for the advice though

@kbrock kbrock force-pushed the tags branch 2 times, most recently from 3902ca6 to 5d83ba0 Compare March 28, 2018 22:53
@Fryguy
Copy link
Member

Fryguy commented Mar 29, 2018

I didn't think we had a category model, so that's surprising. If it's phased out can we just delete the Category model in this PR...or is that a follow up once you verify other repos aren't using it?

@kbrock
Copy link
Member Author

kbrock commented Mar 29, 2018

@Fryguy it is referenced in 3 other repos and factories. Will put in requests over there. Debating if it makes sense to transition Category -> Classification -> Tag. Or just do one fell swoop.

@kbrock kbrock force-pushed the tags branch 2 times, most recently from 3a974ad to a5a7466 Compare April 2, 2018 16:27
Fryguy
Fryguy reviewed Apr 3, 2018
View reviewed changes
app/models/classification.rb Outdated Show resolved Hide resolved
@Fryguy
Copy link
Member

Fryguy commented Apr 3, 2018

I think Category -> Classification -> Tag makes sense. I think if this PR removed Category that would be a good step 1.

@Fryguy
Copy link
Member

Fryguy commented Apr 6, 2018

This LGTM, but have you tested the other repos, particularly manageiq-ui-classic to see that there are no failures?

@miq-bot
Copy link
Member

miq-bot commented Apr 16, 2018

This pull request is not mergeable. Please rebase and repush.

@kbrock kbrock mentioned this pull request May 14, 2018
Merged
@cben
Copy link
Contributor

cben commented May 22, 2018

LOL, never knew we had a Category model.

But but... this table has no STI type column, how did it even work?! What does it mean to have a model subclass without type?
You get Category instances only if you explicitly do something like Category.where(...), and usually same records are represented as Classification instances? very confusing imho..

Let me know if I can help 🔥 ✂️ 😄

@kbrock
Copy link
Member Author

kbrock commented Sep 4, 2018

What does it mean to have a model subclass without type?
-- @cben

type is a default scope. In this case, we made our own default scope

@cben I'm thinking of just abandoning it. This seems like a bunch of change when we would like to stabilize stuff

jrafanie
jrafanie reviewed Sep 4, 2018
View reviewed changes
spec/models/dialog_field_serializer_spec.rb
let(:dynamic) { false }
let(:category) do
double("Category", :name => "best category ever", :description => "best category ever", :single_value => true)
Copy link
Member

Choose a reason for hiding this comment

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

This makes me laugh

@jrafanie
Copy link
Member

jrafanie commented Sep 4, 2018

I didn't think we had a category model, so that's surprising. If it's phased out can we just delete the Category model in this PR...or is that a follow up once you verify other repos aren't using it?

Agreed.

LOL, never knew we had a Category model.

Same here.

@kbrock kbrock mentioned this pull request Nov 30, 2018
Merged
@Fryguy
Copy link
Member

Fryguy commented Dec 14, 2018

@kbrock Is this still in progress or should it be closed? Wasn't sure what you meant by

I'm thinking of just abandoning it. This seems like a bunch of change when we would like to stabilize stuff

This was referenced Dec 19, 2018
Merged
Merged
Merged
@kbrock
Copy link
Member Author

kbrock commented Jan 2, 2019

@Fryguy yea. so this has been pared back so much.

This is as far as I feel comfortable going with this PR. Not ready to drop Category model yet.
If it looks good to you, lets merge.

If you don't like, then we can just close/toss.

@kbrock kbrock mentioned this pull request Jan 30, 2019
Closed
@kbrock kbrock changed the title Phase out Category - use Classification instead [WIP] Phase out Category - use Classification instead Jan 31, 2019
@kbrock kbrock changed the title [WIP] Phase out Category - use Classification instead [WIP] Remove Category - use Classification instead Jan 31, 2019
@kbrock
Copy link
Member Author

kbrock commented Jan 31, 2019 •
edited
Loading

WIP status: Looks like we have a few strings passed around with Category objects. the href methods are blowing up

@miq-bot miq-bot added the wip label Jan 31, 2019
@miq-bot
Copy link
Member

miq-bot commented Feb 13, 2019

This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Feb 26, 2019

Checked commit kbrock@4d4b680 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/dialog_field_importer.rb

@kbrock
Copy link
Member Author

kbrock commented Apr 10, 2019

Category was added to simplify the api.
And without it, the api is much more complex. so I'm going with live and let live.

It really isn't hurting anyone, especially since we can use a named scope in here and not reference parent_id=0

closing

@kbrock kbrock closed this Apr 10, 2019
@kbrock kbrock deleted the tags branch April 10, 2019 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cben cben cben left review comments

@Fryguy Fryguy Fryguy left review comments

@jrafanie jrafanie jrafanie left review comments

Assignees

@jrafanie jrafanie

Labels
core refactoring wip
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kbrock @Fryguy @miq-bot @cben @jrafanie