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

Allow performing SSA on Datastores in Datastore Clusters accordion #6298

Merged

Conversation

hstastna
Copy link

@hstastna hstastna commented Oct 14, 2019

Issue: #6241

There was an issue with Datastores and SSA: The selected Datastore was not accessible while performing SSA from the list of Datastores of selected Datastore Cluster.

Details:
In generic_button_operation method, called by scanstorage after clicking on Perform SmartState Analysis toolbar button, screen_redirection is called. However, in this method, logic for @lastaction different than "show_list" is missing (see). The result is that it calls show instead of show_list here (it does not 'go' into the proper condition and does not call return when it should so we fall into the next condition with show call). And this is simply wrong because we are in the list of Datastores, we want to call show_list.
I changed the condition and added some more possibilities for @lastaction, the same as it already is in a different place (for deleting Datastores, where it already fixes the same problem), see this. This all is just because @lastaction is very specific, it is set to "storage_pod_list". Not sure how important is to set @lastaction to such value but I am not going to change it.
For other methods, it simply works just because @lastaction is set to "show_list" or there is options[:redirect] present (in screen_redirection). Or, simply generic_button_operation is not called at all.


Before:
ssa_before

After:
ssa_after

@hstastna hstastna force-pushed the SSA_Datastore_under_Datastore_Clusters branch from bed03c4 to b5c2040 Compare October 14, 2019 12:05
@miq-bot
Copy link
Member

miq-bot commented Oct 14, 2019

Checked commit hstastna@b5c2040 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@ZitaNemeckova
Copy link
Contributor

@hstastna The breadcrumb changes after clicking on SSA.
From:
image
To:
image

It's not happening for Delete Datastores.

Otherwise it works perfectly :)

@himdel
Copy link
Contributor

himdel commented Oct 29, 2019

SSA = toolbar Configuration / Perform SmartState Analysis

@hstastna
Copy link
Author

hstastna commented Oct 29, 2019

@ZitaNemeckova The issue here is that it is not happening for Delete Datastores. Right? Because I think that, originally, 'Datastores in .. Cluster' was missing in breadcrumbs.

@himdel
Copy link
Contributor

himdel commented Oct 29, 2019

@hstastna I think it's the other way - using a toolbar button that doesn't change the screen we're on should not change the breadcrumbs either.

So, on a list screen, neither Delete nor Perform SSA should change the breadcrumbs.

@ZitaNemeckova
Copy link
Contributor

ZitaNemeckova commented Oct 29, 2019

@hstastna @himdel It seems it happens for SSA for VMs too. So I would say it's out of scope for this PR. WDYT?

image

Copy link
Contributor

@ZitaNemeckova ZitaNemeckova left a comment

Choose a reason for hiding this comment

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

The breadcrumb issue is out of scope for this PR.

LGTM 👍

@himdel himdel merged commit 28a2178 into ManageIQ:master Oct 29, 2019
@himdel himdel self-assigned this Oct 29, 2019
@himdel himdel added this to the Sprint 124 Ending Nov 11, 2019 milestone Oct 29, 2019
@hstastna
Copy link
Author

hstastna commented Oct 29, 2019

I agree, partially with you, @himdel . Yes, breadcrumbs shouldn't be changed because of some action but I think that here, after the SSA, the breadcrumbs are finally right, and before the action they weren't - 'Datastores in .. Cluster' was missing there. Because, everywhere across MIQ, I can see that the title of the page is also in breadcrumbs. I don't see any reason to have it differently in this page.
So I think we need to fix the breadcrumbs how they 'look' before SSA here. But I can open a separate issue for this and we can discuss what's the proper solution.

@himdel
Copy link
Contributor

himdel commented Oct 29, 2019

Aah, agreed, the breadcrumbs should reflect the current screen.
So, they shouldn't change after the toolbar action, but should be that way even before that action.

Create the issue please and maybe @rvsia can point you to the right way of fixing this situation.

@himdel
Copy link
Contributor

himdel commented Oct 29, 2019

Ah, @ZitaNemeckova already created #6347 :)

EDIT: but #6348 is more correct re #6298 (comment)

@ZitaNemeckova
Copy link
Contributor

Created an issue for it
#6347

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.

4 participants