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

Use base_model name instead of demodulize #5182

Merged
merged 1 commit into from
Feb 1, 2019
Merged

Use base_model name instead of demodulize #5182

merged 1 commit into from
Feb 1, 2019

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Jan 22, 2019 •
edited
Loading

CustomButtons run on Hosts return "HostEsx" with demodulize, and CloudNetworks when demodulized return "public" or "private", which means that for four or so different object types we're not running the custom buttons currently cause we can't find the right target_type for the API endpoint.

However, the check that was in was temporary and used to determine if the object could use the new dialogs. Erik confirmed that all objects that needed to do now, so this check can be removed. To anyone who doesn't like it, the alternatives are adding a buncha models to the NEW_DIALOG_USERS list, or moving all that logic to the respective models, neither of which is particularly palatable, so boooooya. (a64c119 and 65281ac and 0f4d24f)

It's been in since bcfee50.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1668023

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 22, 2019

@miq-bot add_label bug
@miq-bot add_label hammer/yes
@miq-bot add_reviewer @eclarizio

@miq-bot miq-bot requested a review from eclarizio January 22, 2019 18:46
@d-m-u d-m-u changed the title Use class base_name instead of demodulize Use class base_class name instead of demodulize Jan 22, 2019
@d-m-u d-m-u changed the title Use class base_class name instead of demodulize [WIP] Use class base_class name instead of demodulize Jan 22, 2019
@d-m-u d-m-u changed the title [WIP] Use class base_class name instead of demodulize Use class base_class name instead of demodulize Jan 22, 2019
@d-m-u d-m-u changed the title Use class base_class name instead of demodulize Use base_class name instead of demodulize Jan 22, 2019
@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 22, 2019

@miq-bot add_reviewer @gmcculloug

@miq-bot miq-bot requested a review from gmcculloug January 22, 2019 21:36
eclarizio
eclarizio suggested changes Jan 23, 2019
View reviewed changes
spec/services/dialog_local_service_spec.rb Outdated Show resolved Hide resolved
app/services/dialog_local_service.rb Outdated Show resolved Hide resolved
app/services/dialog_local_service.rb Outdated Show resolved Hide resolved
@d-m-u d-m-u force-pushed the bz1668023 branch 2 times, most recently from 19c7c1d to a1dafd1 Compare January 28, 2019 14:00
@d-m-u d-m-u changed the title Use base_class name instead of demodulize Use base_model name instead of demodulize Jan 28, 2019
@d-m-u d-m-u force-pushed the bz1668023 branch 2 times, most recently from b74d064 to 1f49d87 Compare January 28, 2019 20:11
@d-m-u d-m-u closed this Jan 29, 2019
@d-m-u d-m-u reopened this Jan 29, 2019
gmcculloug
gmcculloug reviewed Jan 30, 2019
View reviewed changes
app/services/dialog_local_service.rb Outdated Show resolved Hide resolved
app/services/dialog_local_service.rb Show resolved Hide resolved
app/services/dialog_local_service.rb
@@ -140,7 +101,7 @@ def determine_target_type(obj)
when /^Service/
"service"
else
obj.class.name.demodulize.underscore
obj.class.base_model.name.underscore.downcase
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a much different return value in some cases. Were you able to fully verify this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ones that are different from the old file with this change are:
cloud_network for what would have been private and
host for what would've returned host_esx

@d-m-u d-m-u force-pushed the bz1668023 branch 3 times, most recently from 76d14a9 to 8c105d8 Compare January 31, 2019 14:04
@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 31, 2019

@bdunne any chance you could 👀 for me please?

@miq-bot
Copy link
Member

miq-bot commented Jan 31, 2019

Checked commit d-m-u@e31688a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 1, 2019

@gmcculloug @bdunne please review?

bdunne
bdunne reviewed Feb 1, 2019
View reviewed changes
spec/services/dialog_local_service_spec.rb
let(:obj) { double(:class => ManageIQ::Providers::Vmware::CloudManager, :id => 123) }

include_examples "DialogLocalService#determine_dialog_locals_for_custom_button return value",
"ext_management_system", "providers", "/ems_cloud"
Copy link
Member

Choose a reason for hiding this comment

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

Is "/ems_cloud" right? Do you know how that is used?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, is that a UI route?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah but it'd be real spiffing if someone on the UI team could confirm that that's what is supposed to happen here.

bdunne
bdunne approved these changes Feb 1, 2019
View reviewed changes
Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

👍 Can't merge here, but LGTM

@h-kataria h-kataria self-assigned this Feb 1, 2019
@h-kataria h-kataria added this to the Sprint 104 Ending Feb 04, 2019 milestone Feb 1, 2019
@h-kataria h-kataria merged commit a510142 into ManageIQ:master Feb 1, 2019
simaishi pushed a commit that referenced this pull request Apr 23, 2019
@h-kataria @simaishi
Merge pull request #5182 from d-m-u/bz1668023
b227da4 
Use base_model name instead of demodulize

(cherry picked from commit a510142)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1702490
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit b227da42720395a8a09f815fadd1e61813bb5623
Author: Harpreet Kataria <[email protected]>
Date:   Fri Feb 1 14:08:08 2019 -0500

    Merge pull request #5182 from d-m-u/bz1668023
    
    Use base_model name instead of demodulize
    
    (cherry picked from commit a510142e47953ce93f933e112132c93ea0eadc5c)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1702490

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmcculloug gmcculloug gmcculloug left review comments

@eclarizio eclarizio eclarizio requested changes

@bdunne bdunne bdunne approved these changes

Assignees

@h-kataria h-kataria

Labels
automation/automate bug hammer/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@d-m-u @miq-bot @simaishi @eclarizio @bdunne @gmcculloug @h-kataria @mzazrivec