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

vm_controller and session mixin #865

Closed
wants to merge 6 commits into from

Conversation

LANeo64
Copy link

@LANeo64 LANeo64 commented Mar 31, 2017

@miq-bot assign himdel
@miq-bot add_label refactoring,euwe/no
Added changes to vm_controller to be compatible with GenereicSessionMixin

session[:polArr] = @polArr unless @polArr.nil?
session[:policy_options] = @policy_options unless @policy_options.nil?
end

def title
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move the title method above private please?

(The respond_to?(:title) bit in GenericSessionMixin won't see it otherwise.)

@himdel
Copy link
Contributor

himdel commented Apr 6, 2017

@LANeo64 thanks, this looks good, adding miq_exists_mode and miq_compressed makes sense to me since they are used in at least 7 controllers 👍

Except for the title thing, LGTM :).

LANeo64 added 2 commits April 7, 2017 08:22
removed some cosmetic things as spaces and empty lines
@himdel
Copy link
Contributor

himdel commented Apr 13, 2017

Uh, @LANeo64 just noticed a bit of a problem .. you're not using the mixin ;). No super anywhere, and the methods are still defined so .. you kinda need super :).

@himdel himdel added wip and removed wip labels May 19, 2017
@himdel
Copy link
Contributor

himdel commented May 19, 2017

Ping @LANeo64 will you be able to add that super or should I take care of it?

@LANeo64
Copy link
Author

LANeo64 commented May 19, 2017

@himdel Oh, sorry i totally forgot about it. It seems that i have no clue where the word super should go. Can you please put it there?
Thanks a lot.

himdel added 2 commits May 22, 2017 16:52
too much indent because of an earlier (removed) change
Actually call super when using the mixin..
@himdel
Copy link
Contributor

himdel commented May 22, 2017

@LANeo64 OK, added :).

@ZitaNemeckova can you please double-check?

@miq-bot
Copy link
Member

miq-bot commented May 22, 2017

Checked commits LANeo64/manageiq-ui-classic@26c087b~...3990cfd with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

@himdel himdel closed this Jun 10, 2017
@himdel himdel reopened this Jun 10, 2017
@himdel himdel closed this Jun 12, 2017
@himdel himdel reopened this Jun 12, 2017
@LANeo64
Copy link
Author

LANeo64 commented Jun 12, 2017

@himdel I see you are trying to pass it through the automatic tests, may i ask what is causing the errors? I looked up the errors from Travis and saw that something was missing. Can't recall the name tho...

@himdel
Copy link
Contributor

himdel commented Jun 13, 2017

Ah, sorry @LANeo64, looks like that controller has since changed and no longer actually uses the session in any way... I'm still investigating, but we may have to close your PR, sorry :(.

(But, for the purpose of grading, I'm marking this as green 👍 )

@himdel himdel closed this Jun 23, 2017
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.

3 participants