Fix the navigation to Services maintab after viewing a Generic Object item via Services Explorer

[AparnaKarve]

Fixed the navigation to Services maintab issue reproduced with the steps below -

1. Navigate to Services -> My Services -> Select the Service with the GO instance
2. Click on the GO Instances count in the Summary screen
3. Pick a GO Instance to view in detail
4. Click on the Services maintab on the left

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

[AparnaKarve]

@miq-bot add_label bug,gaprindashvili/yes

[AparnaKarve]

@lgalis Can you review/test this?
Thanks.

[lgalis]

@AparnaKarve - looks good, tested in the UI

[miq-bot]

Checked commits AparnaKarve/manageiq-ui-classic@a967317~...644ad84 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. ⭐

[AparnaKarve]

@lgalis Thanks for testing this.

@dclarizio I have addressed 1 of the 2 code climate issues.
The current code climate issue above would need a more involved refactoring and hence not considering resolving it now.

[simaishi]

Gaprindashvili backport details:

$ git log -1
commit 5e560f59a2e6ce8cd6af8d8a177fa8ff61f59564
Author: Dan Clarizio <[email protected]>
Date:   Mon Dec 11 11:13:07 2017 -0800

    Merge pull request #2992 from AparnaKarve/bz1523396_fix_go_service_navigate_away
    
    Fix the navigation to Services maintab after viewing a Generic Object item via Services Explorer
    (cherry picked from commit f4d4d4388a0b9c072e7fdcc7d3919c17a8764a40)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1524715

[martinpovolny]

@AparnaKarve, @dclarizio : don't do/merge such things, please.

Problems with this:

1. the show methods have certain structure. I and other people are working to refactor them all in the same way. If we refactor one of them in a different way, the situation is not getting any better. On the contrary. Anyone grepping the sources for patterns will miss this one. Also the automated tools looking for similar code will miss this one.
2. This change replaces one issue with another. The 2 extracted lines are moved into method named set_display. That immediately creates an issue because methods should not be named set_* or get_*.

Thank you!

[AparnaKarve]

The change was done to address cyclomatic complexity in the method.

Usually Rubocop explicitly mentions about set_ or get_ usage in method names -- but as you can see, in this case, Rubocop is all clean.
Has the set_ /get_ method name restriction been removed? -- we are not getting those warnings anymore.

[martinpovolny]

The change was done to address cyclomatic complexity in the method.

I can see that in the commit message. However I don't find that information relevant in the context of my comment.

Usually Rubocop explicitly mentions about set_ or get_ usage in method names -- but as you can see, in this case, Rubocop is all clean.

I am not aware of such change.

[dclarizio]

@martinpovolny can you recommend the right way to fix this so we can change it upstream with a follow up PR? Thx, Dan

[AparnaKarve]

For this particular case, where the choice is between keeping an existing pattern that exists in the other controllers Vs fixing the cyclomatic complexity, I would favor fixing the cyclomatic complexity.

(content edited)
I think it is a good idea to have a separate method that sets @display, which has a special significance in the code, rather than blending it up with rest of the show method

Maybe the other controllers should be having a dedicated method for setting @display too.

[martinpovolny]

For this particular case, where the choice is between keeping an existing pattern that exists in the other controllers Vs fixing the cyclomatic complexity, I would favor fixing the cyclomatic complexity.

I would not.

It all boils down to a simple fact: If we refactor similar stuff differently in different places, we increase the cost of maintenance from that point in time until a point where someone actually unifies the similar places back.

I hope that it's clear that maintaining 2 different routines that do the same thing is more expensive than maintaining one.

Also the cost of refactoring that piece increases. Someone will have to find the places (that look less similar now), reason about why these are different: Is there some functional difference? Is there a bug? Then probably unify the code back and then replace it with refactored code.

I can try to reword the reasons behind that or I can show you a number of examples.

If you compare the cost of the above vs the cost of keeping the "cyclomatic complexity" warning in place would you still prefer "fixing" the problem?

I urge everyone to think in the context of the whole app. Not in the context of a single method or controller. If the controllers or methods in them that do the very same thing differ too much due to people refactoring each piece in a different way the project will not be maintainable at all.

[AparnaKarve]

Ok, noted the above.

To remove this cyclomatic complexity fix in upstream, we could simply revert 644ad84