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

Make Automate Domains and Providers clickable from the list displayed through Tenant's Relationships #6144

Merged

Conversation

hstastna
Copy link
Contributor

@hstastna hstastna commented Sep 3, 2019

RFE: https://bugzilla.redhat.com/show_bug.cgi?id=1678124
Depends on: #5675 (merged)
Tracking issue: ManageIQ/manageiq#18734


This is a continuation of an RFE about converting Tenants details page to textual summary and adding Relationships with Services, Automate Domains and Providers. Specifically, this PR makes Automate Domains and Providers clickable from the list displayed thru Relationships table of Tenant's textual summary. After clicking on any Automate Domain or a Provider, we are redirected to the appropriate controller and details about the selected item are displayed.

For Automate Domains, I had to add new show method to miq_ae_class controller and to add a condition in application helper for choosing the right controller (instead of unnecessary creating miq_ae_domain controller). I also had to add missing template app/views/miq_ae_class/show.html.haml to make it work.


TODO:

  • make Automate Domains in the list (displayed thru Relationships table of Tenant's textual summary) clickable
  • make most of the common Providers clickable
  • make all the Satellite Providers clickable
    (but the whole rendered page does not work well - that's another bug, unrelated to this PR)
  • solve clicking on Ansible Tower Providers
  • solve clicking on Embedded Ansible
    (redirect to Automate -> Ansible -> Playbooks (Embedded Ansible))
  • make sure that clicking on all possible kinds of providers will work, with proper redirect,
    or at least, it will not 💣
  • specs

Automate Domains are finally clickable:
aedomain_click

Clicking on individual Satellite provider in the list of Providers related to the selected Tenant displays the provider and its Configuration Profiles:
foreman_provider_show

After clicking on some provider in the list, which is not common or is unknown, flash message is displayed and we are redirected to the Tenant's textual summary:
provider-no-click


Note:
Normally, we can access Automate Domains in Automation -> Automate -> Explorer.

@miq-bot miq-bot added the wip label Sep 3, 2019
@hstastna hstastna force-pushed the Tenant_summary_AeDomains_Providers branch 8 times, most recently from 779885a to 827e24b Compare September 10, 2019 16:27
@hstastna hstastna force-pushed the Tenant_summary_AeDomains_Providers branch 9 times, most recently from d92a175 to 34e2a33 Compare September 17, 2019 09:01
@hstastna hstastna force-pushed the Tenant_summary_AeDomains_Providers branch from 34e2a33 to 8b5e292 Compare September 18, 2019 08:45
@hstastna hstastna force-pushed the Tenant_summary_AeDomains_Providers branch 8 times, most recently from 4ac83ef to 3d56794 Compare September 19, 2019 16:05
@hstastna
Copy link
Contributor Author

hstastna commented Sep 23, 2019

Clicking on these providers redirects to appropriate controller and displays dashboard view/details page:

  • manageiq-providers-amazon
  • manageiq-providers-openstack
  • manageiq-providers-lenovo
  • manageiq-providers-vmware
  • manageiq-providers-ansible_tower
  • Embedded Ansible
  • manageiq-providers-kubernetes
  • manageiq-providers-azure
  • manageiq-providers-ovirt
  • manageiq-providers-hawkular
  • manageiq-providers-openshift
  • manageiq-providers-google
  • manageiq-providers-nuage (Network Manager)
  • manageiq-providers-foreman
  • manageiq-providers-kubevirt
  • manageiq-providers-redfish
  • manageiq-providers-scvmm
  • manageiq-providers-dummy_provider
  • manageiq-providers-azure_stack

@hstastna hstastna force-pushed the Tenant_summary_AeDomains_Providers branch 9 times, most recently from 96f62c2 to 7b9c1b5 Compare September 26, 2019 10:38
@hstastna hstastna changed the title [WIP] Make Automate Domains and Providers clickable from the list displayed thru Tenant's Relationships Make Automate Domains and Providers clickable from the list displayed thru Tenant's Relationships Sep 26, 2019
@miq-bot miq-bot removed the wip label Sep 26, 2019
@hstastna hstastna changed the title Make Automate Domains and Providers clickable from the list displayed thru Tenant's Relationships Make Automate Domains and Providers clickable from the list displayed through Tenant's Relationships Sep 26, 2019
@hstastna hstastna force-pushed the Tenant_summary_AeDomains_Providers branch 3 times, most recently from c479f61 to 4602c25 Compare September 26, 2019 13:09
redirect_to(polymorphic_path(record))
rescue NoMethodError
flash_to_session(_("Cannot redirect to \"%{record}\" provider.") % {:record => record.name}, :error)
redirect_to(:controller => 'ops', :action => 'explorer')
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to redirect user back to the tenant detail, but can stay in the screen displaying providers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussion with Martin P we decided to redirect user back to the Tenant textual summary.

Copy link
Member

@romanblanco romanblanco left a comment

Choose a reason for hiding this comment

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

Tested, works 👍

@hstastna hstastna force-pushed the Tenant_summary_AeDomains_Providers branch from 4602c25 to 49e16d6 Compare September 30, 2019 13:43
@miq-bot
Copy link
Member

miq-bot commented Sep 30, 2019

Checked commits hstastna/manageiq-ui-classic@df4d1e5~...49e16d6 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
15 files checked, 0 offenses detected
Everything looks fine. 👍

begin
redirect_to(polymorphic_path(record))
rescue NoMethodError
flash_to_session(_("Cannot redirect to \"%{record}\" provider.") % {:record => record.name}, :error)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the word redirect here is a bit unfortunate. I understand that we're in redirect controller and what we're doing here is rendering redirects, but the user reading the above flash message isn't expecting any redirects, the user is just clicking in the ui.

May I suggest a string like: Cannot display ... provider or something like that?

Copy link
Contributor Author

@hstastna hstastna Oct 3, 2019

Choose a reason for hiding this comment

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

"Cannot redirect..." flash message/string was suggested by @martinpovolny.
And in my opinion, "cannot redirect..." can be more accurate. We just have problems with redirecting to the proper place from list of Providers from Tenant's summary, not with displaying provider's info in general. Displaying still can be possible for example via different place/controller. I just cannot guarantee proper redirecting for all kinds of providers here, so I better implemented displaying this flash message. I am little bit worried that we would create a little misunderstanding by using "cannot display..". It would feel like we are not able to display the provider at all. @martinpovolny @mzazrivec What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

But we aren't able to display the provider in this case, are we? My point was that redirect is really a matter of internal implementation here, not necessarilly something a user would understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO, the flash message is ok. And yes, it could be confusing for some users because as you said, it's about internal implementation. I have no problem with changing the string in a flash message. But .. I think that this all is just about that we are trying to see into users' heads and assume what's understandable for them. Anyway, @martinpovolny what do you think about Milan's assumption?

@mzazrivec mzazrivec added this to the Sprint 122 Ending Oct 14, 2019 milestone Oct 10, 2019
@mzazrivec mzazrivec merged commit 3f7cb90 into ManageIQ:master Oct 10, 2019
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.

None yet

4 participants