Showing authentication status in Physical Provider list #3199
Conversation
douglasgabriel
changed the title
douglasgabriel
force-pushed
the
invalid_credentials_alert
branch
from
6f12ef8 to
a7ecfb9
Compare
douglasgabriel
changed the title
douglasgabriel
changed the title
douglasgabriel
force-pushed
the
invalid_credentials_alert
branch
4 times, most recently
from
a0b1522 to
c7e2321
Compare
douglasgabriel
changed the title
douglasgabriel
force-pushed
the
invalid_credentials_alert
branch
2 times, most recently
from
251f9ac to
1e3dbbd
Compare
|
Hi @martinpovolny, can you take a look on this? |
| def check_providers_credentials | ||
| model.find_each do |provider| | ||
| if provider.authentication_status.casecmp("invalid").zero? | ||
| add_flash(_("The provider \"%<name>s\" is with invalid credentials") % {:name => provider.name}, :error) |
There was a problem hiding this comment.
Can we change the string to
"The provider \"%{name}\" is with invalid credentials"so as not to confuse translators?
There was a problem hiding this comment.
Yes, I can, but I've get an issue of rubocop when I did so
douglasgabriel
force-pushed
the
invalid_credentials_alert
branch
from
1e3dbbd to
e0ba050
Compare
| def check_providers_credentials | ||
| model.find_each do |provider| | ||
| if provider.authentication_status.casecmp("invalid").zero? | ||
| add_flash(_("The provider \"%{name}\" is with invalid credentials") % {:name => provider.name}, :error) |
There was a problem hiding this comment.
is with invalid credentials should be has invalid credentials, for proper English.
|
|
||
| context "show provider with invalid credentials" do | ||
| it "shows an alert to user" do | ||
| expect(assigns(:flash_array).first[:message]).to include("The provider \"#{@provider.name}\" is with invalid credentials") |
There was a problem hiding this comment.
Same proper English change needed here. :)
douglasgabriel
force-pushed
the
invalid_credentials_alert
branch
from
e0ba050 to
285e1fb
Compare
|
it's done @dclarizio and @mzazrivec , please let me know if there are some more issues to address here 👍 |
|
I was discussing this with @martinpovolny and we concluded that this approach diverges from whatever we do in the rest of our application: we don't render flash messages informing the user of invalid credentials anywhere in our list views. I don't question that this information is useful to have, the question is where. Maybe a toolbar button which would trigger a check like this and render a flash message when needed? @Loicavenel what do you think? |
|
I agree with you, this feature doesn't exists yet. But the information of invalid credentials is already being save on database, just left to show this information by a util way for the user. I believe that this is a good feature that improves the user experience. A button that checks the credentials already exist, however none message about the result of the check is shown to user. Like we can see in the image: Well IMHO, this feature provides an approach a little more automated and a button is a more manual approach But yeah, I'm waiting the definitions from you guys |
|
Our thinking on this implementation is that we want to provide a way for the user to reset their password to the provider via MIQ. We needed a method to let the user when this is required. Is this better served as an MIQ Alert without the flash message? |
|
I understand that there's a value in having this information visible somewhere and I don't question the motivation behind this. Though rendering a flash message in the list view informing of invalid credentials would really be unique just to the physical providers page (we don't do anything like that in the other list views). So I'd rather have this decided by @Loicavenel |
dclarizio
changed the title
|
@douglasgabriel We are looking into this as I'm pretty sure we've solved it differently in other areas. Marking as WIP until we can respond. |
|
@douglasgabriel The Grid/Tile mode already shows the authentication status. Notice the green check mark in the image below - We could extend this feature for listviews as well - as in, add a column in the listview that shows To add the Auth Status column, the changes are quite simple. diff --git a/product/views/ManageIQ_Providers_PhysicalInfraManager.yaml b/product/views/ManageIQ_Providers_PhysicalInfraManager.yaml
index c8c6f6951..560ed9088 100644
--- a/product/views/ManageIQ_Providers_PhysicalInfraManager.yaml
+++ b/product/views/ManageIQ_Providers_PhysicalInfraManager.yaml
@@ -26,6 +26,7 @@ cols:
- total_vms
- total_miq_templates
- region_description
+- authentication_status
# Included tables (joined, has_one, has_many) and columns
include:
zone:
@@ -47,6 +48,7 @@ col_order:
- total_vms
- total_miq_templates
- region_description
+- authentication_status
# Column titles, in order
headers:
- Name
@@ -59,6 +61,7 @@ headers:
- VMs
- Templates
- Region
+- Authentication Status
# Condition(s) string for the SQL query
conditions:
# Order string for the SQL queryWith these changes, your list should look like this - Let us know what you think. |
|
Hmm, thanks @AparnaKarve . This way is pretty simpler, I like it 😄 |
douglasgabriel
force-pushed
the
invalid_credentials_alert
branch
from
285e1fb to
8b7f3e7
Compare
douglasgabriel
changed the title
|
Checked commit douglasgabriel@8b7f3e7 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
This PR is able to: