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

Enable new dialog-user feature for more custom buttons #2671

Merged
merged 1 commit into from
Dec 5, 2017
Merged

Enable new dialog-user feature for more custom buttons #2671

merged 1 commit into from
Dec 5, 2017

Conversation

eclarizio
Copy link
Member

@eclarizio eclarizio commented Nov 8, 2017 •
edited
Loading

Related to #2562, it is a continuation for other objects. This one will make custom buttons for the following classes go through the new dialog-user ui-component:

CloudTenant
CloudVolume
ContainerNode
EmsCluster
Host
InfraManager (Provider)
MiqTemplate
Storage

https://www.pivotaltracker.com/story/show/152449649

/cc @gmcculloug
@d-m-u Can you review, please?
@himdel Can you also review or tag someone to review, please?

@miq-bot assign @h-kataria
@miq-bot add_label gaprindashvili/yes, enhancement, automation/automate

@eclarizio eclarizio force-pushed the add_top_priority_custom_buttons branch 2 times, most recently from 917684a to 6a7230f Compare November 8, 2017 18:06
@eclarizio eclarizio changed the title Enable new dialog-user feature for more custom buttons [WIP] Enable new dialog-user feature for more custom buttons Nov 8, 2017
@eclarizio
Copy link
Member Author

eclarizio commented Nov 8, 2017 •
edited
Loading

WIP-ing until the API code gets merged that supports custom actions for these objects.

@miq-bot miq-bot added the wip label Nov 8, 2017
@himdel
Copy link
Contributor

himdel commented Nov 9, 2017 •
edited
Loading

@eclarizio Perfect, nice to see this happening :) ..

@ZitaNemeckova is working on enabling custom buttons/custom buttons with dialogs in a few more screens - most notably in #1912 (continued in #2269, #2439, #2593 and #2679) .. so most likely you'll have a few more on the todo ;)

EDIT: also #3263

@eclarizio
Copy link
Member Author

@himdel Yeah, that's ok, those should by default use the old dialog-user code. This new class will just need to be updated to support all of the other possible classes that can utilize custom buttons.

@eclarizio eclarizio force-pushed the add_top_priority_custom_buttons branch from 6a7230f to 5b9bd53 Compare November 13, 2017 16:47
@miq-bot
Copy link
Member

miq-bot commented Nov 30, 2017

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

@eclarizio eclarizio force-pushed the add_top_priority_custom_buttons branch from 5b9bd53 to 5032ed2 Compare November 30, 2017 18:26
@eclarizio eclarizio changed the title [WIP] Enable new dialog-user feature for more custom buttons Enable new dialog-user feature for more custom buttons Nov 30, 2017
@eclarizio
Copy link
Member Author

Un WIP-ing now that the API code has been merged. The only potential issue here is the API performance concerns around custom buttons. @abellotti Do you have any input there?

@eclarizio eclarizio force-pushed the add_top_priority_custom_buttons branch from 5032ed2 to 8b20ad7 Compare November 30, 2017 18:38
@eclarizio eclarizio mentioned this pull request Nov 30, 2017
Merged
syncrou
syncrou reviewed Nov 30, 2017
View reviewed changes
Copy link
Contributor

@syncrou syncrou left a comment

Choose a reason for hiding this comment

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

This really looks good to me - though - I don't have context on the performance issues question brought up by @eclarizio - so other than that - 👍

syncrou
syncrou reviewed Nov 30, 2017
View reviewed changes
app/services/custom_button_service.rb Outdated

return dialog_locals unless NEW_DIALOG_USERS.include?(obj.class.name.demodulize)

submit_endpoint, cancel_endpoint = determine_api_endpoints(obj)
Copy link
Contributor

Choose a reason for hiding this comment

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

I like how this is much easier to read.

@eclarizio eclarizio force-pushed the add_top_priority_custom_buttons branch from 8b20ad7 to 9619b9a Compare December 1, 2017 17:23
@h-kataria
Copy link
Contributor

@dclarizio changes look good to me, it can be merged if API Performance is not an issue here, @abellotti any thoughts?

abellotti
abellotti reviewed Dec 1, 2017
View reviewed changes
app/services/dialog_local_service.rb
def determine_api_endpoints(obj)
case obj.class.name.demodulize
when /CloudTenant/
api_collection_name = "cloud_tenants"
Copy link
Member

Choose a reason for hiding this comment

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

Optionally, the api_collection_name could be obtained via:

api_collection_name = Api::CollectionConfig.new.name_for_klass(obj.class)

@abellotti
Copy link
Member

perf wise should be good in api. The fixes went into g/yes so that collection searches do not expand custom actions, and this PR seems to act upon resources so I'm ok here.

@miq-bot
Copy link
Member

miq-bot commented Dec 4, 2017

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

@eclarizio eclarizio force-pushed the add_top_priority_custom_buttons branch from 9619b9a to 3f35e39 Compare December 4, 2017 22:42
@eclarizio
Add dialog locals for many objects and refactor to a service
0f4d24f 
https://www.pivotaltracker.com/story/show/152449649

Custom buttons supported in this commit:
CloudTenant
CloudVolume
ContainerNode
EmsCluster
Host
InfraManager (Provider)
MiqTemplate
Storage
@eclarizio eclarizio force-pushed the add_top_priority_custom_buttons branch from 3f35e39 to 0f4d24f Compare December 5, 2017 00:07
@miq-bot
Copy link
Member

miq-bot commented Dec 5, 2017

Checked commit eclarizio@0f4d24f with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@h-kataria h-kataria added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 5, 2017
@h-kataria h-kataria merged commit 30fa192 into ManageIQ:master Dec 5, 2017
simaishi pushed a commit that referenced this pull request Dec 5, 2017
@h-kataria @simaishi
Merge pull request #2671 from eclarizio/add_top_priority_custom_buttons
b4144c4 
Enable new dialog-user feature for more custom buttons
(cherry picked from commit 30fa192)
@simaishi
Copy link
Contributor

simaishi commented Dec 5, 2017

Gaprindashvili backport details:

$ git log -1
commit b4144c4c9506b80444a0dff3c09753a29cad5177
Author: Harpreet Kataria <[email protected]>
Date:   Mon Dec 4 19:38:48 2017 -0500

    Merge pull request #2671 from eclarizio/add_top_priority_custom_buttons
    
    Enable new dialog-user feature for more custom buttons
    (cherry picked from commit 30fa192980bc7db9a9f3656393e11f944c7e6fd0)

@eclarizio eclarizio mentioned this pull request Feb 20, 2018
Merged
@ZitaNemeckova ZitaNemeckova mentioned this pull request May 3, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abellotti abellotti abellotti left review comments

@syncrou syncrou syncrou left review comments

Assignees

@h-kataria h-kataria

Labels
automation/automate enhancement gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@eclarizio @himdel @miq-bot @h-kataria @abellotti @simaishi @syncrou