diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index b09d58c..3f53c49 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -41,8 +41,10 @@ # POST /resource def create - if !sign_up_params[:accept_terms] then + if !sign_up_params[:accept_terms] redirect_to after_sign_up_error_path_for(resource), alert: _('You must accept the terms and conditions to register.') + elsif params[:user][:org_id].blank? && params[:user][:other_organisation].blank? + redirect_to after_sign_up_error_path_for(resource), alert: _('Please select an organisation from the list, or enter your organisation\'s name.') else existing_user = User.where_case_insensitive('email', sign_up_params[:email]).first if existing_user.present? @@ -55,6 +57,13 @@ # https://github.com/DMPRoadmap/roadmap/issues/322 end end + if params[:user][:org_id].blank? + other_org = Org.find_by(is_other: true) + if other_org.nil? + redirect_to(after_sign_up_error_path_for(resource), alert: _('You cannot be assigned to other organisation since that option does not exist in the system. Please contact your system administrators.')) and return + end + params[:user][:org_id] = other_org.id + end build_resource(sign_up_params) if resource.save if resource.active_for_authentication? diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 51b7535..4737072 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -106,7 +106,11 @@ def org_swap # Allows the user to swap their org affiliation on the fly authorize current_user - org = Org.find(org_swap_params[:org_id]) + begin + org = Org.find(org_swap_params[:org_id]) + rescue ActiveRecord::RecordNotFound + redirect_to(request.referer, alert: _('Please select an organisation from the list')) and return + end if org.present? current_user.org = org if current_user.save! diff --git a/app/models/user.rb b/app/models/user.rb index 76f0f5f..e0eedf1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -125,13 +125,14 @@ # @param new_organisation_id [Integer] the id for an organisation # @return [String] the empty string as a causality of setting api_token def org_id=(new_org_id) + puts "$$$$ new_org_id: #{new_org_id}" unless self.can_change_org? || new_org_id.nil? || self.org.nil? || (new_org_id.to_s == self.org.id.to_s) # rip all permissions from the user self.perms.delete_all end # set the user's new organisation super(new_org_id) - self.save! + # self.save! # rip api permissions from the user self.remove_token! end diff --git a/app/views/shared/_create_account_form.html.erb b/app/views/shared/_create_account_form.html.erb index 55e7e7a..c2ab40d 100644 --- a/app/views/shared/_create_account_form.html.erb +++ b/app/views/shared/_create_account_form.html.erb @@ -12,7 +12,6 @@ <%= f.label(:email, _('Email'), class: "control-label") %> <%= f.email_field(:email, class: "form-control", "aria-required": true) %> -
<%= render partial: "shared/my_org", locals: {f: f, default_org: @default_org, diff --git a/app/views/shared/_my_org.html.erb b/app/views/shared/_my_org.html.erb index 7f9b506..11a7051 100644 --- a/app/views/shared/_my_org.html.erb +++ b/app/views/shared/_my_org.html.erb @@ -13,4 +13,7 @@
<%= f.text_field :other_organisation, autocomplete: "off", class: "form-control hide", placeholder: _('Please enter the name of your organisation') %> +
+ +
<% end %> diff --git a/lib/assets/javascripts/views/shared/create_account_form.js b/lib/assets/javascripts/views/shared/create_account_form.js index 4c4c3bb..0b11ef8 100644 --- a/lib/assets/javascripts/views/shared/create_account_form.js +++ b/lib/assets/javascripts/views/shared/create_account_form.js @@ -1,25 +1,22 @@ import initAutoComplete from '../../utils/autoComplete'; import ariatiseForm from '../../utils/ariatiseForm'; import { togglisePasswords } from '../../utils/passwordHelper'; -import { SHOW_SELECT_ORG_MESSAGE, SHOW_OTHER_ORG_MESSAGE } from '../../constants'; +import { isValidText } from '../../utils/isValidInputType'; $(() => { initAutoComplete(); ariatiseForm({ selector: '#create_account_form' }); togglisePasswords({ selector: '#create_account_form' }); - $('#other_org_link').click((e) => { - e.preventDefault(); - const other = $('#user_other_organisation'); - const selector = $('.combobox-container'); - if ($(other).css('display') === 'none') { - $('#other_org_link').text(SHOW_SELECT_ORG_MESSAGE); - $(selector).hide(); - $(other).show(); + $('#create_account_form').on('submit', (e) => { + // Additional validation to force the user to choose an org or type something for other + const orgId = $('[name="user[org_id]"]'); + const otherOrg = $('[name="user[other_organisation]"]'); + if (isValidText(orgId.val()) || isValidText(otherOrg.val())) { + $('#help-org').hide(); } else { - $('#other_org_link').text(SHOW_OTHER_ORG_MESSAGE); - $(selector).show(); - $(other).val('').hide(); + e.preventDefault(); + $('#help-org').show(); } }); }); diff --git a/test/functional/registrations_controller_test.rb b/test/functional/registrations_controller_test.rb index b9c2a5b..771f498 100644 --- a/test/functional/registrations_controller_test.rb +++ b/test/functional/registrations_controller_test.rb @@ -7,7 +7,7 @@ @user = User.first end - # ------------------------------------------------------------- + # # ------------------------------------------------------------- test "sign up form loads" do get new_user_registration_path @@ -26,17 +26,27 @@ assert_equal _('You must accept the terms and conditions to register.'), flash[:alert] end + test "user receives proper error messaging if they have not select an org from the list or entered their organisation name" do + post user_registration_path, {user: {accept_terms: "on"}} + assert_response :redirect + follow_redirect! + + assert_response :success + assert_equal _('Please select an organisation from the list, or enter your organisation\'s name.'), flash[:alert] + end + # ------------------------------------------------------------- test "user receives proper error messaging if they have not provided a valid email and/or password" do + org_id = Org.first.id [ {}, - {email: 'foo.bar@test.org'}, # No Password or Confirmation - {password: 'test12345'}, # No Email + {email: 'foo.bar@test.org' }, # No Password or Confirmation + {password: 'test12345' }, # No Email {password: 'test12345', password_confirmation: 'test12345'}, # No Email - {email: 'foo.bar@test.org', password: 'test'}, # Password is too short - {email: 'foo.bar$test.org', password: 'test12345'} # invalid email + {email: 'foo.bar@test.org', password: 'test' }, # Password is too short + {email: 'foo.bar$test.org', password: 'test12345' } # invalid email ].each do |params| - post user_registration_path, {user: {accept_terms: "on"}.merge(params)} - + post user_registration_path, {user: { accept_terms: "on", org_id: org_id }.merge(params)} + assert_response :redirect follow_redirect! @@ -49,23 +59,16 @@ test "user is able to register and is auto-logged in and brought to profile page" do form = {accept_terms: "on", email: 'foo.bar@test.org', - password: 'Test12345'} + password: 'Test12345', + org_id: Org.first.id } + post user_registration_path, {user: form} - cntr = 1 - # Test the bare minimum requirements and then all options - [form, form.merge({email: "foo.bar#{cntr}@test.org", - organisation_id: Org.first.id})].each do |params| - post user_registration_path, {user: params} + assert_response :redirect + assert_redirected_to root_url - assert_response :redirect - assert_redirected_to root_url - - follow_redirect! - assert_response :redirect - assert_redirected_to plans_path - - cntr += 1 - end + follow_redirect! + assert_response :redirect + assert_redirected_to plans_path end # -------------------------------------------------------------