Addressing feedback from most recent reviews.

7 files changed, 72 insertions, 64 deletions

diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js
index afdca012127..fc34d243dd7 100644
--- a/app/assets/javascripts/lib/utils/common_utils.js
+++ b/app/assets/javascripts/lib/utils/common_utils.js
@@ -4,14 +4,6 @@ import { getLocationHash } from './url_utility';
 import { convertToCamelCase } from './text_utility';
 import { isObject } from './type_utility';
 
-/**
- * Simply returns `window.location`. This function exists to provide a means to spy
- * `window.location` in unit tests.
- *
- * @returns {Location | string | any} The browser's `window.location`
- */
-export const windowLocation = () => window.location;
-
 export const getPagePath = (index = 0) => {
   const page = $('body').attr('data-page') || '';
 
diff --git a/app/assets/javascripts/lib/utils/url_utility.js b/app/assets/javascripts/lib/utils/url_utility.js
index 61f53a632b8..4ba84589705 100644
--- a/app/assets/javascripts/lib/utils/url_utility.js
+++ b/app/assets/javascripts/lib/utils/url_utility.js
@@ -1,5 +1,3 @@
-import { windowLocation } from './common_utils';
-
 // Returns an array containing the value(s) of the
 // of the key passed as an argument
 export function getParameterValues(sParam) {
@@ -53,11 +51,11 @@ export function mergeUrlParams(params, url) {
  * @param {string} [url=windowLocation().href] - url from which the query param will be removed
  * @returns {string} A copy of the original url but without the query param
  */
-export function removeParams(params, url = windowLocation().href) {
+export function removeParams(params, url = window.location.href) {
   const [rootAndQuery, fragment] = url.split('#');
   const [root, query] = rootAndQuery.split('?');
 
-  if (query === undefined) {
+  if (!query) {
     return url;
   }
 
diff --git a/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js
index 56ea39f1259..e617fecaa0f 100644
--- a/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js
+++ b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js
@@ -6,8 +6,8 @@ import { mergeUrlParams, setUrlFragment } from '~/lib/utils/url_utility';
  *
  * @param fragment {string} - url fragment to be preserved
  */
-export default function preserveUrlFragment(fragment) {
-  if (fragment && fragment !== '') {
+export default function preserveUrlFragment(fragment = '') {
+  if (fragment) {
     const normalFragment = fragment.replace(/^#/, '');
 
     // Append the fragment to all sign-in/sign-up form actions so it is preserved when the user is
diff --git a/spec/javascripts/fixtures/sessions.rb b/spec/javascripts/fixtures/sessions.rb
new file mode 100644
index 00000000000..e90a58e8c54
--- /dev/null
+++ b/spec/javascripts/fixtures/sessions.rb
@@ -0,0 +1,26 @@
+require 'spec_helper'
+
+describe 'Sessions (JavaScript fixtures)' do
+  include JavaScriptFixturesHelpers
+
+  before(:all) do
+    clean_frontend_fixtures('sessions/')
+  end
+
+  describe SessionsController, '(JavaScript fixtures)', type: :controller do
+    include DeviseHelpers
+
+    render_views
+
+    before do
+      set_devise_mapping(context: @request)
+    end
+
+    it 'sessions/new.html.raw' do |example|
+      get :new
+
+      expect(response).to be_success
+      store_frontend_fixture(response, example.description)
+    end
+  end
+end
diff --git a/spec/javascripts/fixtures/signin_forms_and_buttons.html.haml b/spec/javascripts/fixtures/signin_forms_and_buttons.html.haml
deleted file mode 100644
index 32a9becb636..00000000000
--- a/spec/javascripts/fixtures/signin_forms_and_buttons.html.haml
+++ /dev/null
@@ -1,21 +0,0 @@
-#signin-container
-  .tab-content
-    .active.login-box.tab-pane
-      .login-body
-        %form#new_ldap_user{ action: '/users/auth/ldapmain/callback', method: 'post' }
-
-    .login-box.tab-pane
-      .login-body
-        %form#new_user.new_user{ action: '/users/sign_in', method: 'post' }
-
-    #register-pane.login-box.tab-pane
-      .login-body
-        %form#new_new_user.new_new_user{ action: '/users', method: 'post' }
-
-  .omniauth-container
-    %span.light
-      %a#oauth-login-auth0.oauth-login.btn{ href: '/users/auth/auth0' } Auth0
-    %span.light
-      %a#oauth-login-facebook.oauth-login.btn{ href:'/users/auth/facebook?remember_me=1' } Facebook
-    %span.light
-      %a#oauth-login-saml.oauth-login.btn{ href:'/users/auth/saml' } SAML
diff --git a/spec/javascripts/lib/utils/url_utility_spec.js b/spec/javascripts/lib/utils/url_utility_spec.js
index 1ac1c414df7..381c7b2d0a6 100644
--- a/spec/javascripts/lib/utils/url_utility_spec.js
+++ b/spec/javascripts/lib/utils/url_utility_spec.js
@@ -1,4 +1,4 @@
-import UrlUtility, * as urlUtils from '~/lib/utils/url_utility';
+import * as urlUtils from '~/lib/utils/url_utility';
 
 describe('URL utility', () => {
   describe('webIDEUrl', () => {
@@ -86,20 +86,6 @@ describe('URL utility', () => {
         expect(url).toBe('/home?l=en_US#H2');
       });
     });
-
-    describe('when no url is passed', () => {
-      it('should remove params from window.location.href', () => {
-        spyOnDependency(UrlUtility, 'windowLocation').and.callFake(() => {
-          const anchor = document.createElement('a');
-          anchor.href = 'https://mysite.com/?zip=11111&locale=en_US&ads=false#privacy';
-          return anchor;
-        });
-
-        const url = urlUtils.removeParams(['locale']);
-
-        expect(url).toBe('https://mysite.com/?zip=11111&ads=false#privacy');
-      });
-    });
   });
 
   describe('setUrlFragment', () => {
diff --git a/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js b/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js
index de308127a0f..7a8227479d4 100644
--- a/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js
+++ b/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js
@@ -2,33 +2,60 @@ import $ from 'jquery';
 import preserveUrlFragment from '~/pages/sessions/new/preserve_url_fragment';
 
 describe('preserve_url_fragment', () => {
-  preloadFixtures('static/signin_forms_and_buttons.html.raw');
+  preloadFixtures('sessions/new.html.raw');
 
   beforeEach(() => {
-    loadFixtures('static/signin_forms_and_buttons.html.raw');
+    loadFixtures('sessions/new.html.raw');
   });
 
   it('adds the url fragment to all login and sign up form actions', () => {
     preserveUrlFragment('#L65');
 
-    expect($('#new_ldap_user').attr('action')).toBe('/users/auth/ldapmain/callback#L65');
-    expect($('#new_user').attr('action')).toBe('/users/sign_in#L65');
-    expect($('#new_new_user').attr('action')).toBe('/users#L65');
+    expect($('#new_user').attr('action')).toBe('http://test.host/users/sign_in#L65');
+    expect($('#new_new_user').attr('action')).toBe('http://test.host/users#L65');
   });
 
-  it('adds the "redirect_fragment" query parameter to all OAuth and SAML login buttons', () => {
-    preserveUrlFragment('#L65');
+  it('does not add an empty url fragment to login and sign up form actions', () => {
+    preserveUrlFragment();
+
+    expect($('#new_user').attr('action')).toBe('http://test.host/users/sign_in');
+    expect($('#new_new_user').attr('action')).toBe('http://test.host/users');
+  });
+
+  it('does not add an empty query parameter to OmniAuth login buttons', () => {
+    preserveUrlFragment();
+
+    expect($('#oauth-login-cas3').attr('href')).toBe('http://test.host/users/auth/cas3');
 
     expect($('.omniauth-container #oauth-login-auth0').attr('href')).toBe(
-      '/users/auth/auth0?redirect_fragment=L65',
+      'http://test.host/users/auth/auth0',
     );
+  });
 
-    expect($('.omniauth-container #oauth-login-facebook').attr('href')).toBe(
-      '/users/auth/facebook?remember_me=1&redirect_fragment=L65',
-    );
+  describe('adds "redirect_fragment" query parameter to OmniAuth login buttons', () => {
+    it('when "remember_me" is not present', () => {
+      preserveUrlFragment('#L65');
 
-    expect($('.omniauth-container #oauth-login-saml').attr('href')).toBe(
-      '/users/auth/saml?redirect_fragment=L65',
-    );
+      expect($('#oauth-login-cas3').attr('href')).toBe(
+        'http://test.host/users/auth/cas3?redirect_fragment=L65',
+      );
+
+      expect($('.omniauth-container #oauth-login-auth0').attr('href')).toBe(
+        'http://test.host/users/auth/auth0?redirect_fragment=L65',
+      );
+    });
+
+    it('when "remember-me" is present', () => {
+      $('a.omniauth-btn').attr('href', (i, href) => `${href}?remember_me=1`);
+      preserveUrlFragment('#L65');
+
+      expect($('#oauth-login-cas3').attr('href')).toBe(
+        'http://test.host/users/auth/cas3?remember_me=1&redirect_fragment=L65',
+      );
+
+      expect($('#oauth-login-auth0').attr('href')).toBe(
+        'http://test.host/users/auth/auth0?remember_me=1&redirect_fragment=L65',
+      );
+    });
   });
 });