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

host-controller use GenericSessionMixin #472

Merged
merged 6 commits into from
Apr 10, 2017
Merged

host-controller use GenericSessionMixin #472

merged 6 commits into from
Apr 10, 2017

Conversation

petoS6
Copy link
Contributor

@petoS6 petoS6 commented Feb 24, 2017

Delete useless lines in hostController, using GenericSessionMixin now.

@miq-bot miq-bot added the wip label Feb 24, 2017
@petoS6
Copy link
Contributor Author

petoS6 commented Mar 10, 2017

@miq-bot assign himdel
@miq-bot add_label refactoring, euwe/no

@petoS6
Copy link
Contributor Author

petoS6 commented Mar 10, 2017

@miq-bot remove_label wip

@petoS6 petoS6 changed the title [WIP] host-controller use GenericSessionMixin host-controller use GenericSessionMixin Mar 10, 2017
@miq-bot miq-bot removed the wip label Mar 10, 2017
@@ -646,20 +647,18 @@ def get_hosts
end

def get_session_data
super
@title = _("Hosts")
Copy link
Contributor

Choose a reason for hiding this comment

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

@title should live in the title method now, which is being used by the mixin so you should no longer have to set it here..

@@ -646,20 +647,18 @@ def get_hosts
end

def get_session_data
super
@title = _("Hosts")
@layout = "host"
Copy link
Contributor

Choose a reason for hiding this comment

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

@layout will probably be set automagically from model..

@@ -646,20 +647,18 @@ def get_hosts
end

def get_session_data
super
@title = _("Hosts")
@layout = "host"
@drift_db = "Host"
@lastaction = session[:host_lastaction]
Copy link
Contributor

Choose a reason for hiding this comment

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

@lastaction is already handled by GenericSessionMixin .. and so is @catinfo.

@title = _("Hosts")
@layout = "host"
@drift_db = "Host"
@lastaction = session[:host_lastaction]
@display = session[:host_display]
@filters = session[:host_filters]
@catinfo = session[:host_catinfo]
@base = session[:vm_compare_base]
Copy link
Contributor

Choose a reason for hiding this comment

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

@base you can ignore/delete, it should be completely gone after #665

@himdel
Copy link
Contributor

himdel commented Mar 13, 2017

Also missing PR description ;)

@himdel
Copy link
Contributor

himdel commented Mar 13, 2017

(The travis failures are valid, but not caused by changes in your PR - rebasing should make travis green again :).)

@miq-bot
Copy link
Member

miq-bot commented Mar 17, 2017

This pull request is not mergeable. Please rebase and repush.

@himdel
Copy link
Contributor

himdel commented Mar 27, 2017

@petoS6 This looks good, I'd merge, but I'm going to ask you for one more change (sorry :))...

Please add a title method to the controller, it should just return _("Host"). You're right that GenericSessionMixin will set the right value anyway, but a) consistency with the others, and b) we're trying to deprecate ui_lookup (which GenericSessionMixin uses to do that).

Thanks

end

def set_session_data
super
session[:host_lastaction] = @lastaction
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, one more thing, can you also remove this (lastaction) line? That one is handled by the mixin too.

Also, the same for catinfo.

@miq-bot
Copy link
Member

miq-bot commented Apr 7, 2017

Checked commits https://github.com/petoS6/manageiq-ui-classic/compare/4653c8500c80d986ad47cb96e6de482199d332de~...1267c3a319bbde92160b4c3db03251c3c5f95556 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🍪

@himdel himdel added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 10, 2017
@himdel himdel merged commit b86fb7c into ManageIQ:master Apr 10, 2017
@himdel
Copy link
Contributor

himdel commented Apr 10, 2017

Thanks @petoS6, merged :)

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