Addressing peer review feedback.

Replacing inline JS with ES 2015 functions included in pages/sessions/new. Also applying suggested server-side syntax improvements to OmniAuthCallbacksController.

3 files changed, 108 insertions, 0 deletions

diff --git a/spec/javascripts/fixtures/signin_forms_and_buttons.html.haml b/spec/javascripts/fixtures/signin_forms_and_buttons.html.haml
new file mode 100644
index 00000000000..32a9becb636
--- /dev/null
+++ b/spec/javascripts/fixtures/signin_forms_and_buttons.html.haml
@@ -0,0 +1,21 @@
+#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/common_utils_spec.js b/spec/javascripts/lib/utils/common_utils_spec.js
index f320f232687..3a25be766cb 100644
--- a/spec/javascripts/lib/utils/common_utils_spec.js
+++ b/spec/javascripts/lib/utils/common_utils_spec.js
@@ -65,6 +65,67 @@ describe('common_utils', () => {
     });
   });
 
+  describe('setUrlParam', () => {
+    it('should append param when url has no other params', () => {
+      const url = commonUtils.setUrlParam('/feature/home', 'newParam', 'yes');
+      expect(url).toBe('/feature/home?newParam=yes');
+    });
+
+    it('should append param when url has other params', () => {
+      const url = commonUtils.setUrlParam('/feature/home?showAll=true', 'newParam', 'yes');
+      expect(url).toBe('/feature/home?showAll=true&newParam=yes');
+    });
+
+    it('should replace param when url contains the param', () => {
+      const url = commonUtils.setUrlParam('/feature/home?showAll=true&limit=5', 'limit', '100');
+      expect(url).toBe('/feature/home?showAll=true&limit=100');
+    });
+
+    it('should update param and preserve fragment', () => {
+      const url = commonUtils.setUrlParam('/home?q=no&limit=5&showAll=true#H1', 'limit', '100');
+      expect(url).toBe('/home?q=no&limit=100&showAll=true#H1');
+    });
+  });
+
+  describe('removeUrlParam', () => {
+    it('should remove param when url has no other params', () => {
+      const url = commonUtils.removeUrlParam('/feature/home?size=5', 'size');
+      expect(url).toBe('/feature/home');
+    });
+
+    it('should remove param when url has other params', () => {
+      const url = commonUtils.removeUrlParam('/feature/home?q=1&size=5&f=html', 'size');
+      expect(url).toBe('/feature/home?q=1&f=html');
+    });
+
+    it('should remove param and preserve fragment', () => {
+      const url = commonUtils.removeUrlParam('/feature/home?size=5#H2', 'size');
+      expect(url).toBe('/feature/home#H2');
+    });
+
+    it('should not modify url if param does not exist', () => {
+      const url = commonUtils.removeUrlParam('/feature/home?q=1&size=5&f=html', 'locale');
+      expect(url).toBe('/feature/home?q=1&size=5&f=html');
+    });
+  });
+
+  describe('setUrlFragment', () => {
+    it('should set fragment when url has no fragment', () => {
+      const url = commonUtils.setUrlFragment('/home/feature', 'usage');
+      expect(url).toBe('/home/feature#usage');
+    });
+
+    it('should set fragment when url has existing fragment', () => {
+      const url = commonUtils.setUrlFragment('/home/feature#overview', 'usage');
+      expect(url).toBe('/home/feature#usage');
+    });
+
+    it('should set fragment when given fragment includes #', () => {
+      const url = commonUtils.setUrlFragment('/home/feature#overview', '#install');
+      expect(url).toBe('/home/feature#install');
+    });
+  });
+
   describe('handleLocationHash', () => {
     beforeEach(() => {
       spyOn(window.document, 'getElementById').and.callThrough();
diff --git a/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js b/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js
new file mode 100644
index 00000000000..c3be06ce6f9
--- /dev/null
+++ b/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js
@@ -0,0 +1,26 @@
+import $ from 'jquery';
+import preserveUrlFragment from '~/pages/sessions/new/preserve_url_fragment';
+
+describe('preserve_url_fragment', () => {
+  preloadFixtures('static/signin_forms_and_buttons.html.raw');
+
+  beforeEach(() => {
+    loadFixtures('static/signin_forms_and_buttons.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');
+  });
+
+  it('adds the "redirect_fragment" query parameter to all OAuth and SAML login buttons', () => {
+    preserveUrlFragment('#L65');
+
+    expect($('.omniauth-container #oauth-login-auth0').attr('href')).toBe('/users/auth/auth0?redirect_fragment=L65');
+    expect($('.omniauth-container #oauth-login-facebook').attr('href')).toBe('/users/auth/facebook?remember_me=1&redirect_fragment=L65');
+    expect($('.omniauth-container #oauth-login-saml').attr('href')).toBe('/users/auth/saml?redirect_fragment=L65');
+  });
+});