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

Display custom buttons after comming from relationship table #4775

Merged
merged 9 commits into from
Oct 30, 2018

Conversation

romanblanco
Copy link
Member

@romanblanco romanblanco commented Oct 15, 2018

The code displays the custom buttons if the previous screen was a
provider screen (@record) and displayed item (@display) is from the list
of classes supported by custom buttons (APPLIES_TO_CLASS_BASE_MODELS)

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

Links

Steps for Testing/QA

  • Create a custom button for any entity
  • Navigate to the entity through the provider relationship screen
  • Custom button should be visible

CloudVolume ContainerGroup ContainerImage ContainerNode ContainerProject
ContainerTemplate ContainerVolume EmsCluster ExtManagementSystem
GenericObject GenericObjectDefinition Host LoadBalancer
MiqGroup MiqTemp MiqTemplate NetworkRouter OrchestrationStack SecurityGroup Service
Copy link
Contributor

Choose a reason for hiding this comment

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

you can remove MiqTemp, no such thing

@romanblanco
Copy link
Member Author

@miq-bot add_label blocker
@miq-bot add_label bug
@miq-bot add_label button
@miq-bot add_label actions toolbars
@miq-bot add_label hammer/yes

@miq-bot
Copy link
Member

miq-bot commented Oct 16, 2018

@romanblanco Cannot apply the following label because they are not recognized: button

@miq-bot miq-bot added the bug label Oct 16, 2018
@miq-bot
Copy link
Member

miq-bot commented Oct 16, 2018

@romanblanco Cannot apply the following label because they are not recognized: actions toolbars

@himdel
Copy link
Contributor

himdel commented Oct 16, 2018

@h-kataria
Copy link
Contributor

@romanblanco will this PR also show custom buttons when viewing list of Hosts for a Cluster or a Storage?

@ZitaNemeckova
Copy link
Contributor

ZitaNemeckova commented Oct 17, 2018

It's possible to execute custom button without anything selected from nested list view. Not consistent with list view.

From list view:
screen shot 2018-10-17 at 9 34 34 am
From nested list:
screen shot 2018-10-17 at 9 35 52 am

And it uses the id of the parent(Provider) instead of the id of selected item. When parent and child class don't have an instance with same id it throws an error:

Cloud Provider -> Cloud Tenants -> check one or more Cloud Tenants-> click custom button for list -> button uses id of Provider instead of Cloud Tenants
screen shot 2018-10-17 at 9 49 55 am

Container Provider -> Container Nodes -> check one or more Container Nodes-> click custom button for list -> button uses id of Provider instead of Container Nodes
screen shot 2018-10-17 at 10 15 26 am

@ZitaNemeckova
Copy link
Contributor

Just a check that Custom button is in toolbar in nested view for list and single entity:

  • Availability Zone
  • Cloud Tenant
  • Orchestration Stack
  • Security Group
  • Tenant - error on click
  • Cluster / Deployment Role
  • Host / Node
  • Datastore
  • VM Template and Image for Infra Provider
  • VM and Instance for Infra Provider
  • Container Project
  • Container Pod
  • Container Node
  • Container Template
  • Container Image

Missing custom button for nested list:

  • Provider - Cloud Provider summary page -> Storage Managers
  • Cloud Volume
  • VM Template and Image for Cloud Provider
  • VM and Instance for Cloud Provider
  • Cloud Object Store Container
  • Cloud Network
  • Load Balancer
  • Network Router
  • Container Volume
  • Cloud Subnet

@romanblanco romanblanco changed the title Display custom buttons after comming from relationship table [WIP]Display custom buttons after comming from relationship table Oct 17, 2018
@miq-bot miq-bot added the wip label Oct 17, 2018
@JPrause
Copy link
Member

JPrause commented Oct 18, 2018

@romanblanco if this will be able to be backported, can you also add the gaprindashvili/yes

@romanblanco romanblanco force-pushed the bz1635738-master branch 2 times, most recently from 61a13e8 to f2b0c18 Compare October 18, 2018 14:17
@romanblanco romanblanco changed the title [WIP]Display custom buttons after comming from relationship table Display custom buttons after comming from relationship table Oct 18, 2018
@miq-bot miq-bot removed the wip label Oct 18, 2018
@romanblanco
Copy link
Member Author

@miq-bot add_label hammer/yes
@miq-bot add_label gaprindashvili/yes

Closing #4772

@romanblanco romanblanco force-pushed the bz1635738-master branch 2 times, most recently from a3c7fe5 to ab87317 Compare October 19, 2018 09:52
@mzazrivec
Copy link
Contributor

@ZitaNemeckova @himdel are the current changes OK with you?

@mzazrivec
Copy link
Contributor

@romanblanco the CI failures seem related.

@miq-bot
Copy link
Member

miq-bot commented Oct 26, 2018

Checked commits romanblanco/manageiq-ui-classic@828a7de~...1623721 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
11 files checked, 0 offenses detected
Everything looks fine. 🍰

@JPrause
Copy link
Member

JPrause commented Oct 29, 2018

@romanblanco where does this PR stand.

@romanblanco
Copy link
Member Author

@JPrause I don't plan to do any more changes.

@himdel himdel self-assigned this Oct 30, 2018
@himdel
Copy link
Contributor

himdel commented Oct 30, 2018

Not seeing any breakage, LGTM :)

@himdel himdel merged commit 98b7dd0 into ManageIQ:master Oct 30, 2018
@himdel himdel added this to the Sprint 98 Ending Nov 5, 2018 milestone Oct 30, 2018
@romanblanco romanblanco deleted the bz1635738-master branch October 30, 2018 14:03
simaishi pushed a commit that referenced this pull request Oct 31, 2018
Display custom buttons after comming from relationship table

(cherry picked from commit 98b7dd0)

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

Hammer backport details:

$ git log -1
commit 19322dadedf1a99acf922c9e6de9e1674519ca67
Author: Martin Hradil <[email protected]>
Date:   Tue Oct 30 14:31:25 2018 +0100

    Merge pull request #4775 from romanblanco/bz1635738-master
    
    Display custom buttons after comming from relationship table
    
    (cherry picked from commit 98b7dd0e893120c7d40391644306c35304137d60)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1635738

@simaishi
Copy link
Contributor

simaishi commented Nov 5, 2018

@romanblanco Not sure how to resolve this conflict...

--- a/app/helpers/application_helper/toolbar_builder.rb
+++ b/app/helpers/application_helper/toolbar_builder.rb
@@@ -274,6 -234,11 +275,14 @@@ class ApplicationHelper::ToolbarBuilde
      }
      button[:text] = button_name if input[:text_display]
      button[:onwhen] = '1+' if cb_enabled_for_nested
++<<<<<<< HEAD
++=======
+     if record_id == 'LIST' ||
+        (@display.present? &&
+         custom_button_appliable_class?(@display.camelize.singularize))
+       button[:send_checked] = true
+     end
++>>>>>>> 98b7dd0e8... Merge pull request #4775 from romanblanco/bz1635738-master
      button
    end

button[:send_checked] = true if record_id == 'LIST' (the line this PR modified to the if above) doesn't exist in Gaprindashvili branch. Does that make this whole if unnecessary for G branch??

@himdel
Copy link
Contributor

himdel commented Nov 6, 2018

That line was introduced in #2398 (gaprindashvili/no), because we simplified the condition of when to send the list of checked items to a simple "is send_checked true?".

So, send_checked will not do anything in gaprindashvili and is therefore not needed, but we do need to make sure the old mechanism kicks in.

The previous logic was essentially "url_parms ends with _div" or "url_parms contains id=LIST".

@romanblanco can you verify that record_id is always LIST in that case? If so, we can just skip that check completely. But if we ever depend on the second condition (id is not LIST but @display.present? && custom_button_appliable_class?(@display.camelize.singularize) is true), we would need to adjust the code accordingly.
(Just note that we can't really rewrite url_parms in that case, so we would need to change miqToolbarOnClick to support that - in which case introducing some level of send_checked support doesn't seem like a bad idea.)

(I'm thinking it should be OK to remove, since you never need to send a list of checked items on a detail screen .. probably.)

@romanblanco
Copy link
Member Author

@himdel the condition is necessary.

The first part of condition (record_id == 'LIST') describes usual GTL list, where we want the custom buttons rendered (Infra -> Hosts)

The second part ((@display.present? && custom_button_appliable_class?(@display.camelize.singularize))) describes a case, when the list is accessed through relationship screen (Infra -> Provider -> Hosts (from relationship screen)), and therefore the record_id is not 'LIST' but an id of the provider

romanblanco added a commit to romanblanco/manageiq-ui-classic that referenced this pull request Nov 7, 2018
Response to ManageIQ#4775 (comment)

corrects the login in master, and should make it possible to backport to gaprindashvili
@romanblanco
Copy link
Member Author

@simaishi I've created a new PR that corrects the conflicting logic, taking it back to it's previous form (button[:send_checked] = true if record_id == 'LIST').

#4887

h-kataria pushed a commit to h-kataria/manageiq-ui-classic that referenced this pull request Nov 12, 2018
Response to ManageIQ#4775 (comment)

corrects the login in master, and should make it possible to backport to gaprindashvili

(cherry picked from commit a54bcc0)
h-kataria pushed a commit to h-kataria/manageiq-ui-classic that referenced this pull request Nov 14, 2018
Response to ManageIQ#4775 (comment)

corrects the login in master, and should make it possible to backport to gaprindashvili

(cherry picked from commit a54bcc0)
@simaishi
Copy link
Contributor

Backported to Gaprindashvili via #4893

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.

8 participants