Fixes #16530 - For active session - Redirect to :back or /hosts #3848

swapab · 2016-09-15T11:15:30Z

No description provided.

theforeman-bot · 2016-09-15T11:15:30Z

Can an existing organization member please verify this patch?

theforeman-bot · 2016-09-15T11:15:30Z

Can an existing organization member please verify this patch?

theforeman-bot · 2016-09-15T11:15:30Z

Can an existing organization member please verify this patch?

theforeman-bot · 2016-09-15T11:15:33Z

There were the following issues with the commit message:

length of the first commit message line for cb6423c exceeds 65 characters
commit message for cb6423c is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

houndci-bot · 2016-09-15T11:15:42Z

app/controllers/users_controller.rb

+  def verify_active_session
+    if params[:status].blank? && session[:user].present? && User.exists?(session[:user])
+      warning _("You have already Logged-In.")
+      redirect_to root_path and return


Use && instead of and.

theforeman-bot · 2016-09-15T11:26:14Z

There were the following issues with the commit message:

length of the first commit message line for cb6423c exceeds 65 characters
commit message for cb6423c is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

domcleal

Please address the comments from @theforeman-bot about the length and style of your commit messages, see the links provided for more details. Please also squash the commits to one.

domcleal · 2016-09-16T15:05:37Z

app/controllers/users_controller.rb

+
+  def verify_active_session
+    if params[:status].blank? && session[:user].present? && User.exists?(session[:user])
+      warning _("You have already Logged-In.")


"logged in"

domcleal · 2016-09-16T15:06:20Z

app/controllers/users_controller.rb

+  def verify_active_session
+    if params[:status].blank? && session[:user].present? && User.exists?(session[:user])
+      warning _("You have already Logged-In.")
+      redirect_to root_path


This should be the previous URL preferably, else hosts_path to match login behaviour, and to work for users without dashboard permissions.

Yes, that makes sense.
Previous url OR hosts_path in case no HTTP_REFERER

domcleal · 2016-09-16T15:08:39Z

test/functional/users_controller_test.rb

@@ -322,6 +322,7 @@ class UsersControllerTest < ActionController::TestCase
  end

  test "#login renders login page" do
+    session.clear


Instead of undoing the setup method, it might be better to try and move tests relying on this into a context that redefines setup_user to an empty method.

Sure @domcleal I am sorry I didn't quite get you. Could you please give an example?

Please disregard, I don't think it's worthwhile any more - there were more instances of it in the previous version of the PR.

domcleal · 2016-09-16T15:09:15Z

test/functional/users_controller_test.rb

+      assert_redirected_to root_path
+    end
+
+    test "for user :one -> #login redirects to root(dashboard)" do


What does this test that the block above doesn't?

I thought to also test it with different user(:one) along with default(:admin). Just to make sure users with different role behaves the same.

Sorry, I missed this point. It's worth leaving out unless there's a difference in the tested code for the two types of user, just to minimise the test time.

domcleal · 2016-09-16T15:09:45Z

test/functional/users_controller_test.rb

+  end
+
+  context "no active session present" do
+    test "#login renders login page" do


This looks identical to the "#login renders login page" test on line 324.

line 324 tests the response of :login
In above case the test is on rendered template. Maybe I can extend 324 case itself. Wont't that violate 1 assert per test standard ?

test "#login renders login page" do session.clear get :login assert_response :success end

Sorry, I missed this point. One assertion per test isn't something we try to implement, it's OK if it's asserting multiple things as the test environment/request is identical.

Consider adding messages or comments to the assertions if it's unclear why they're testing what they do, but generally it's clearer to have one test block testing different things about the same request rather than spreading it out.

(So if you'd like to add the assert_template :login back to the existing test, that'd be fine.)

domcleal · 2016-09-16T15:10:16Z

test/functional/users_controller_test.rb

+      assert_template :login
+    end
+
+    test "logout a logged-in user and visit /users/login should render login page" do


I don't think this tests anything that you haven't tested already.

This should make sense now, with the new change of redirecting to :back.

domcleal · 2016-09-16T15:10:49Z

app/controllers/users_controller.rb

@@ -7,6 +7,7 @@ class UsersController < ApplicationController
  skip_before_action :require_login, :authorize, :session_expiry, :update_activity_time, :set_taxonomy, :set_gettext_locale_db, :only => [:login, :logout, :extlogout]
  skip_before_action :authorize, :only => :extlogin
  after_action       :update_activity_time, :only => :login
+  before_action      :verify_active_session, only: :login, unless: -> { request.post? }


I'd suggest sticking with the same hash style as the previous line for readability.

Updated hash style.

theforeman-bot · 2016-09-16T16:58:15Z

There were the following issues with the commit message:

length of the first commit message line for cb6423c exceeds 65 characters
commit message for cb6423c is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2016-09-18T13:36:35Z

There were the following issues with the commit message:

length of the first commit message line for cb6423c exceeds 65 characters
commit message for cb6423c is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2016-09-18T17:14:11Z

There were the following issues with the commit message:

length of the first commit message line for cb6423c exceeds 65 characters
commit message for cb6423c is not wrapped at 72nd column
length of the first commit message line for f9917a2 exceeds 65 characters
commit message for f9917a2 is not wrapped at 72nd column
f954828 must be in the format fixes #redmine_number - brief description
length of the first commit message line for f954828 exceeds 65 characters
commit message for f954828 is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2016-09-18T18:10:49Z

There were the following issues with the commit message:

length of the first commit message line for cb6423c exceeds 65 characters
commit message for cb6423c is not wrapped at 72nd column
length of the first commit message line for f9917a2 exceeds 65 characters
commit message for f9917a2 is not wrapped at 72nd column
f954828 must be in the format fixes #redmine_number - brief description
length of the first commit message line for f954828 exceeds 65 characters
commit message for f954828 is not wrapped at 72nd column
289f756 must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

swapab · 2016-09-18T18:24:08Z

@domcleal Thank you for reviewing my PR.

I have made the requested changes. Could you please have a look ?

I have also rebased the commits to make it look cleaner.

domcleal · 2016-09-19T08:33:18Z

test/functional/users_controller_test.rb

+  context "when user is logged in" do
+    setup do
+      @previous_url = "/bookmarks"
+      get :login, set_session_user


I think this can be removed, it doesn't appear to be testing anything. The main requests are in the body of the test.

Test refactored.

domcleal · 2016-09-19T08:33:51Z

test/functional/users_controller_test.rb

+    end
+  end
+
+  context "when user has logged out" do


I think you can remove this block, it tests the same thing as "#login renders login page" (rendering with an empty session).

domcleal · 2016-09-19T08:37:22Z

app/controllers/users_controller.rb

+  def verify_active_session
+    if !request.post? && params[:status].blank? && User.exists?(session[:user].presence)
+      warning _("You have already logged in")
+      redirect_to back_or_hosts


There is already a redirect_back_or_to helper in the application controller, you should be able to call redirect_back_or_to hosts_path.

Nice. I used the helper method.
Thanks!

domcleal · 2016-09-19T08:37:36Z

[test foreman]

Fixes #16530 - For active session - Redirect to :back or /hosts

swapab · 2016-09-19T10:50:32Z

@domcleal http://ci.theforeman.org/job/test_develop_pr_core/10484/#showFailuresLink
640 failures ? Are these usual ?
I am sure it is getting addressed somewhere. I would be happy to fix them.

domcleal · 2016-09-19T10:57:12Z

Thanks for the update. They will be fixed under ticket http://projects.theforeman.org/issues/16599, I don't know if anybody is working on it right now.

I'll retest once the develop branch has stabilised again.

domcleal · 2016-09-21T14:18:50Z

ok to test

domcleal · 2016-09-22T07:22:13Z

Merged as be36ce9, thanks @swapnilabnave.

theforeman-bot added Waiting on contributor Needs testing Not yet reviewed labels Sep 15, 2016

houndci-bot reviewed Sep 15, 2016

View reviewed changes

theforeman-bot added Needs testing Waiting on contributor and removed Waiting on contributor labels Sep 15, 2016

domcleal suggested changes Sep 16, 2016

View reviewed changes

domcleal removed the Not yet reviewed label Sep 16, 2016

swapab changed the title ~~fixes #16530 - Logged-In user should be redirected to dashboard on re-visiting /users/login path~~ fixes #16530 - Logged-In user should be redirected to dashboard Sep 16, 2016

theforeman-bot added the Waiting on contributor label Sep 16, 2016

theforeman-bot added Needs re-review Needs testing Waiting on contributor and removed Waiting on contributor labels Sep 18, 2016

swapab force-pushed the 16530-if_active_session_then_no_login_page branch from 289f756 to 84d63d3 Compare September 18, 2016 18:19

theforeman-bot removed the Waiting on contributor label Sep 18, 2016

theforeman-bot added Needs re-review Needs testing labels Sep 18, 2016

swapab changed the title ~~fixes #16530 - Logged-In user should be redirected to dashboard~~ Fixes #16530 - For active session - Redirect to :back or /hosts Sep 18, 2016

domcleal suggested changes Sep 19, 2016

View reviewed changes

domcleal added Waiting on contributor and removed Needs re-review labels Sep 19, 2016

Fixes #16530 - If active session - redirect_back_or_to /hosts

de2624d

Fixes #16530 - For active session - Redirect to :back or /hosts

swapab force-pushed the 16530-if_active_session_then_no_login_page branch from 84d63d3 to de2624d Compare September 19, 2016 10:44

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Sep 19, 2016

domcleal added Ready to merge and removed Needs re-review Needs testing labels Sep 21, 2016

domcleal approved these changes Sep 21, 2016

View reviewed changes

domcleal closed this Sep 22, 2016

swapab deleted the 16530-if_active_session_then_no_login_page branch December 14, 2016 14:43

This pull request was closed.

Fixes #16530 - For active session - Redirect to :back or /hosts #3848

Fixes #16530 - For active session - Redirect to :back or /hosts #3848

Conversation

swapab commented Sep 15, 2016

theforeman-bot commented Sep 15, 2016

theforeman-bot commented Sep 15, 2016

theforeman-bot commented Sep 15, 2016

theforeman-bot commented Sep 15, 2016

Choose a reason for hiding this comment

theforeman-bot commented Sep 15, 2016

domcleal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swapab Sep 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theforeman-bot commented Sep 16, 2016

theforeman-bot commented Sep 18, 2016

theforeman-bot commented Sep 18, 2016

theforeman-bot commented Sep 18, 2016

swapab commented Sep 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domcleal Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domcleal commented Sep 19, 2016

swapab commented Sep 19, 2016

domcleal commented Sep 19, 2016

domcleal commented Sep 21, 2016

domcleal commented Sep 22, 2016

swapab Sep 18, 2016 •

edited

Loading

domcleal Sep 19, 2016 •

edited

Loading