tests for error cases on user creation, tidy up validations and fix b…

…roken specs

app/assets/stylesheets/global.scss

@@ -75,10 +75,14 @@ body > .container {
     margin-top: 0;
     margin-bottom: 0;
   }
+  &.is-invalid {
+    border-color: var(--bs-form-invalid-color);
+  }
 }
 
 input:checked[type="checkbox"] {
   --bs-form-check-bg-image: url("data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 20 20'%3e%3cpath fill='none' stroke='%231C1C1C' stroke-linecap='round' stroke-linejoin='round' stroke-width='3' d='m6 10 3 3 6-6'/%3e%3c/svg%3e") !important;
+    background-color: $white !important;
 }
 
 .form-label {
@@ -92,3 +96,7 @@ input:checked[type="checkbox"] {
 .width-fit-content {
   width: fit-content !important;
 }
+
+.invalid-feedback {
+  margin-left: 1rem;
+}

app/controllers/ui/sessions_controller.rb

@@ -66,7 +66,7 @@ def change_password
       @user = User.find_by(password_reset_token: @token)
       if @user
         authorize @user, :update_password?
-        @user.update({ password: params.require(:password), password_reset_token: nil })
+        @user.update(params.permit(:password, :password_confirmation).merge(password_reset_token: nil))
         flash[:success] = I18n.t(:password_reset_success, username: @user.username)
         redirect_to new_ui_session_path
       else

app/controllers/ui/users_controller.rb

@@ -11,16 +11,22 @@ def new
     end
 
     def create
-      @user = User.create(params.permit(
+      @user = User.new(params.require(:user).permit(
         :username,
         :email,
         :password,
         :password_confirmation,
         :ts_and_cs,
       ))
-      session[:user_id] = @user.id
-      flash[:success] = I18n.t(:new_user_success)
-      redirect_to ui_users_path
+      if @user.valid?
+        @user.save
+        session[:user_id] = @user.id
+        flash[:success] = I18n.t(:new_user_success)
+        redirect_to ui_users_path
+      else
+        flash[:alert] = I18n.t(:new_user_failure)
+        render :new, status: :unprocessable_entity
+      end
     end
   end
 end

app/controllers/v0/password_resets_controller.rb

@@ -36,7 +36,11 @@ def show
     def update
       @user = User.find_by!(password_reset_token: params[:id])
       authorize @user, :update_password?
-      if @user.update({ password: params.require(:password), password_reset_token: nil })
+      ActiveSupport::Deprecation.warn(
+        """Creating and updating user passwords in the API without providing a password confirmation
+        is deprecated, and will be removed in an upcomming API release"""
+      )
+      if @user.update({ password: params.require(:password), password_confirmation: params.require(:password), password_reset_token: nil })
         render 'users/show', status: :ok
       else
         raise Smartcitizen::UnprocessableEntity.new @user.errors

app/controllers/v0/users_controller.rb

@@ -55,7 +55,7 @@ def destroy
 private
 
     def user_params
-      params.permit(*[
+      user_params = params.permit(*[
         :email,
         :username,
         :password,
@@ -64,6 +64,11 @@ def user_params
         :url,
         (:role_mask if current_user&.is_admin?)
       ].compact)
+      ActiveSupport::Deprecation.warn(
+        """Creating and updating user passwords in the API without providing a password confirmation
+        is deprecated, and will be removed in an upcomming API release"""
+      )
+      user_params.merge(password_confirmation: user_params[:password])
     end
 
   end

app/models/user.rb

@@ -24,11 +24,10 @@ class User < ActiveRecord::Base
 
   has_secure_password validations: false
 
-  validates :password, presence: { on: :create }, length: { minimum: 5, allow_blank: true }
-  validates :username, :email, presence: true
-  validates :username, uniqueness: true, if: :username?
-  validates :username, length: { in: 3..30 }, allow_nil: true
-  validates :email, format: { with: /@/ }, uniqueness: true
+  validates_acceptance_of :ts_and_cs, on: :create
+  validates :password, presence: { on: :create }, length: { minimum: 5 }, confirmation: true, if: :password
+  validates :username, format: { with: /\A@?[^@]+\z/ , if: :username }, length: { in: 3..30 }, presence: true, uniqueness: true
+  validates :email, format: { with: /@/ }, uniqueness: true, presence: true
   validates :url, format: URI::regexp(%w(http https)), allow_nil: true, allow_blank: true, on: :create
 
   has_many :devices, foreign_key: 'owner_id', after_add: :update_cached_device_ids!, after_remove: :update_cached_device_ids!

config/locales/views/users/en.yml

@@ -1,6 +1,7 @@
 en:
   new_user_submit: "Sign up"
-  new_user_ts_and_cs_label_html: "I accept the <a target=\"_blank\" href=\"https://smartcitizen.me/policy\">Terms and Conditions</a>"
+  new_user_ts_and_cs_label_html: "I accept the <a target=\"_blank\" href=\"https://smartcitizen.me/policy\">terms and conditions</a>"
   new_user_success: "Thanks for signing up! You are now logged in."
+  new_user_failure: "Some errors prevented us from creating your account. Please check below and try again!"
   new_user_login_heading: "Already have an account?"
   new_user_login_link: "Log in"

spec/factories/users.rb

@@ -4,6 +4,8 @@
     sequence(:username) { |n| "user#{n}" }
     sequence(:email) { |n| "user#{n}@bitsushi.com" }
     password { "password1" }
+    password_confirmation { "password1"}
+    ts_and_cs { true }
     url { "http://www.yahoo.com" }
     role_mask { 0 }
 

spec/features/user_management_spec.rb

@@ -2,15 +2,120 @@
 
 feature "User signs up for an account" do
   scenario "User signs up for an account with correct details" do
+    user_count_before = User.count
     visit "/login"
     click_on "Sign up"
     expect(page).to have_current_path(new_ui_user_path)
     fill_in "Email", with: "user@example.com"
     fill_in "Username", with: "myusername"
     fill_in "Password", with: "password123"
     fill_in "Password confirmation", with: "password123"
-    check "I accept the Terms and Conditions"
+    check "I accept the terms and conditions"
     click_on "Sign up"
     expect(page).to have_content("Thanks for signing up! You are now logged in.")
+    expect(User.count).to eq(user_count_before + 1)
+  end
+
+  scenario "User uses invalid email" do
+    user_count_before = User.count
+    visit "/login"
+    click_on "Sign up"
+    expect(page).to have_current_path(new_ui_user_path)
+    fill_in "Email", with: "user_example.com"
+    fill_in "Username", with: "myusername"
+    fill_in "Password", with: "password123"
+    fill_in "Password confirmation", with: "password123"
+    check "I accept the terms and conditions"
+    click_on "Sign up"
+    expect(page).to have_current_path(ui_users_path)
+    expect(page).to have_content("Some errors prevented us from creating your account. Please check below and try again!")
+    expect(page).to have_content("Email\nis invalid")
+    expect(User.count).to eq(user_count_before)
+  end
+
+  scenario "User uses email that has already been taken" do
+    FactoryBot.create(:user, email: "user@example.com")
+    user_count_before = User.count
+    visit "/login"
+    click_on "Sign up"
+    expect(page).to have_current_path(new_ui_user_path)
+    fill_in "Email", with: "user@example.com"
+    fill_in "Username", with: "myusername"
+    fill_in "Password", with: "password123"
+    fill_in "Password confirmation", with: "password123"
+    check "I accept the terms and conditions"
+    click_on "Sign up"
+    expect(page).to have_current_path(ui_users_path)
+    expect(page).to have_content("Some errors prevented us from creating your account. Please check below and try again!")
+    expect(page).to have_content("Email\nhas already been taken")
+    expect(User.count).to eq(user_count_before)
+  end
+
+  scenario "User uses invalid username" do
+    user_count_before = User.count
+    visit "/login"
+    click_on "Sign up"
+    expect(page).to have_current_path(new_ui_user_path)
+    fill_in "Email", with: "user@example.com"
+    fill_in "Username", with: "my@usernameisnotanemail.com"
+    fill_in "Password", with: "password123"
+    fill_in "Password confirmation", with: "password123"
+    check "I accept the terms and conditions"
+    click_on "Sign up"
+    expect(page).to have_current_path(ui_users_path)
+    expect(page).to have_content("Some errors prevented us from creating your account. Please check below and try again!")
+    expect(page).to have_content("Username\nis invalid")
+    expect(User.count).to eq(user_count_before)
+  end
+
+  scenario "User uses username that has already been taken" do
+    FactoryBot.create(:user, username: "myusername")
+    user_count_before = User.count
+    visit "/login"
+    click_on "Sign up"
+    expect(page).to have_current_path(new_ui_user_path)
+    fill_in "Email", with: "user@example.com"
+    fill_in "Username", with: "myusername"
+    fill_in "Password", with: "password123"
+    fill_in "Password confirmation", with: "password123"
+    check "I accept the terms and conditions"
+    click_on "Sign up"
+    expect(page).to have_current_path(ui_users_path)
+    expect(page).to have_content("Some errors prevented us from creating your account. Please check below and try again!")
+    expect(page).to have_content("Username\nhas already been taken")
+    expect(User.count).to eq(user_count_before)
+  end
+
+  scenario "User provides passwords that dont match" do
+    user_count_before = User.count
+    visit "/login"
+    click_on "Sign up"
+    expect(page).to have_current_path(new_ui_user_path)
+    fill_in "Email", with: "user@example.com"
+    fill_in "Username", with: "myusername"
+    fill_in "Password", with: "password123"
+    fill_in "Password confirmation", with: "password246"
+    check "I accept the terms and conditions"
+    click_on "Sign up"
+    expect(page).to have_current_path(ui_users_path)
+    expect(page).to have_content("Some errors prevented us from creating your account. Please check below and try again!")
+    expect(page).to have_content("Password confirmation\ndoesn't match Password")
+    expect(User.count).to eq(user_count_before)
+  end
+
+  scenario "User does not confirm terms and conditions" do
+    user_count_before = User.count
+    visit "/login"
+    click_on "Sign up"
+    expect(page).to have_current_path(new_ui_user_path)
+    fill_in "Email", with: "user@example.com"
+    fill_in "Username", with: "myusername"
+    fill_in "Password", with: "password123"
+    fill_in "Password confirmation", with: "password123"
+    click_on "Sign up"
+    expect(page).to have_current_path(ui_users_path)
+    expect(page).to have_content("Some errors prevented us from creating your account. Please check below and try again!")
+    expect(page).to have_css("#user_ts_and_cs.is-invalid")
+    expect(User.count).to eq(user_count_before)
   end
 end

spec/models/user_spec.rb

@@ -11,6 +11,7 @@
   it { is_expected.to validate_uniqueness_of(:username) }
   it { is_expected.to validate_length_of(:username).is_at_least(3).is_at_most(30) }
 
+  it { is_expected.to validate_confirmation_of(:password) }
   it { is_expected.to have_many(:oauth_applications) }
 
   it { is_expected.to have_many(:devices) }
@@ -29,6 +30,10 @@
     expect(build(:user, email: "not an email")).to be_invalid
   end
 
+  it "is invalid with a username which is like an email adddress" do
+    expect(build(:user, username: "anemail@email.com")).to be_invalid
+  end
+
   it "is invalid without an email" do
     expect(build(:user, email: nil)).to be_invalid
   end

spec/requests/v0/me_spec.rb

@@ -3,7 +3,7 @@
 describe V0::MeController, type: :request do
 
   let(:application) { create :application }
-  let(:user) { create :user, username: 'barney', password: '1234567' }
+  let(:user) { create :user, username: 'barney', password: '1234567', password_confirmation: '1234567' }
   let(:token) { create :access_token, application: application, resource_owner_id: user.id }
 
   before(:each) do

spec/requests/v0/rack_attack_spec.rb

@@ -55,7 +55,7 @@
 
   context "a user with role 'citizen' is logged in" do
     let(:user) {
-      FactoryBot.create(:user, username: username, password: password)
+      FactoryBot.create(:user, username: username, password: password, password_confirmation: password)
     }
 
     context "number of requests is lower than the limit" do
@@ -71,7 +71,7 @@
 
   context "a user with role 'researcher' is logged in" do
     let(:user) {
-      FactoryBot.create(:researcher, username: username, password: password)
+      FactoryBot.create(:researcher, username: username, password: password, password_confirmation: password)
     }
 
     context "number of requests is lower than the limit" do
@@ -87,7 +87,7 @@
 
   context "a user with role 'admin' is logged in" do
     let(:user) {
-      FactoryBot.create(:admin, username: username, password: password)
+      FactoryBot.create(:admin, username: username, password: password, password_confirmation: password)
     }
 
     context "number of requests is lower than the limit" do

spec/requests/v0/sessions_spec.rb

@@ -2,7 +2,7 @@
 
 describe V0::SessionsController do
 
-  let!(:user) { create(:user, username: 'milhouse', password: 'greatpass', email: 'mil@house.com') }
+  let!(:user) { create(:user, username: 'milhouse', password: 'greatpass', password_confirmation: 'greatpass', email: 'mil@house.com') }
 
   it "gets access_token with valid parameters" do
     j = api_post 'sessions', { username: 'milhouse', password: 'greatpass'}