Fix nickname uniqueness validation & generate when inviting (#2783) (#…

…2789)

* Create nickname before saving and fix migrations

* Fix nicknamizable

CHANGELOG.md

@@ -19,6 +19,7 @@
 * **decidim-core**: Fix categories select when missing translations. [\#2742](https://github.com/decidim/decidim/pull/2742)
 * **decidim-core**: Fix mention parsing to only search users in current organization. [\2711](https://github.com/decidim/decidim/pull/2711)
 * **decidim-meetings**: Fix meeting update and creation when its participatory space has a scope without subscopes. [\#2720](https://github.com/decidim/decidim/pull/2720)
+- **decidim-core**: Fix user invitations by generating their nickname. [\#2783](https://github.com/decidim/decidim/pull/2789)
 
 ## [v0.9.2](https://github.com/decidim/decidim/tree/v0.9.2) (2018-2-9)
 

decidim-core/app/commands/decidim/invite_user.rb

@@ -41,6 +41,7 @@ def invite_user
       @user = Decidim::User.new(
         name: form.name,
         email: form.email.downcase,
+        nickname: User.nicknamize(form.name),
         organization: form.organization,
         admin: form.role == "admin",
         roles: form.role == "admin" ? [] : [form.role].compact

decidim-core/app/models/decidim/user.rb

@@ -28,7 +28,7 @@ class User < ApplicationRecord
     validates :locale, inclusion: { in: :available_locales }, allow_blank: true
     validates :tos_agreement, acceptance: true, allow_nil: false, on: :create
     validates :avatar, file_size: { less_than_or_equal_to: ->(_record) { Decidim.maximum_avatar_size } }
-    validates :email, :nickname, uniqueness: { scope: :organization }, unless: -> { deleted? || managed? }
+    validates :email, :nickname, uniqueness: { scope: :organization }, unless: -> { deleted? || managed? || nickname.blank? }
     validates :email, 'valid_email_2/email': { disposable: true }
 
     validate :all_roles_are_valid

decidim-core/db/migrate/20180221101934_fix_nickname_index.rb

@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+class FixNicknameIndex < ActiveRecord::Migration[5.1]
+  class User < ApplicationRecord
+    self.table_name = :decidim_users
+  end
+
+  def change
+    Decidim::User.where(nickname: nil)
+                 .where(deleted_at: nil)
+                 .where(managed: false)
+                 .find_each { |u| u.update_attributes(nickname: Decidim::User.nicknamize(u.name)) }
+
+    # rubocop:disable Rails/SkipsModelValidations
+    Decidim::User.where(nickname: nil)
+                 .update_all("nickname = ''")
+    # rubocop:enable Rails/SkipsModelValidations
+
+    change_column_default :decidim_users, :nickname, ""
+    change_column_null(:decidim_users, :nickname, false)
+  end
+end

decidim-core/lib/decidim/nicknamizable.rb

@@ -42,7 +42,7 @@ def disambiguate(name)
         2.step do |n|
           return candidate unless exists?(nickname: candidate)
 
-          candidate = numbered_variation_of(candidate, n)
+          candidate = numbered_variation_of(name, n)
         end
       end
 

decidim-core/spec/lib/nicknamizable_spec.rb

@@ -43,6 +43,14 @@ class DummyTestUser < ApplicationRecord
 
         expect(subject.nicknamize("Felipe Rocks So Much")).to eq("felipe_rocks_so_mu_2")
       end
+
+      it "resolves conflicts with other existing nicknames" do
+        create(:user, nickname: "existing")
+        create(:user, nickname: "existing_1")
+        create(:user, nickname: "existing_2")
+
+        expect(subject.nicknamize("existing")).to eq("existing_3")
+      end
     end
   end
 end

decidim-core/spec/models/decidim/user_spec.rb

@@ -60,6 +60,77 @@ module Decidim
           expect(user).not_to be_valid
           expect(user.errors[:nickname].length).to eq(1)
         end
+
+        it "can't be empty backed by an index" do
+          expect { user.save(validate: false) }.not_to raise_error
+        end
+
+        context "when managed" do
+          before do
+            user.managed = true
+          end
+
+          it "is valid" do
+            expect(user).to be_valid
+          end
+
+          it "can be saved" do
+            expect(user.save).to be true
+          end
+
+          it "can have duplicates" do
+            user.save!
+
+            expect do
+              create(:user, organization: user.organization,
+                            nickname: user.nickname,
+                            managed: true)
+            end.not_to raise_error
+          end
+        end
+
+        context "when deleted" do
+          before do
+            user.deleted_at = Time.zone.now
+          end
+
+          it "is valid" do
+            expect(user).to be_valid
+          end
+
+          it "can be saved" do
+            expect(user.save).to be true
+          end
+
+          it "can have duplicates" do
+            user.save!
+
+            expect do
+              create(:user, organization: user.organization,
+                            nickname: user.nickname,
+                            deleted_at: Time.zone.now)
+            end.not_to raise_error
+          end
+        end
+      end
+
+      context "when the nickname is not empty" do
+        before do
+          user.nickname = "a-nickname"
+        end
+
+        it "can be created" do
+          expect(user.save).to eq(true)
+        end
+
+        it "can't have duplicates even when skipping validations" do
+          user.save!
+
+          expect do
+            build(:user, organization: user.organization,
+                         nickname: user.nickname).save(validate: false)
+          end.to raise_error(ActiveRecord::RecordNotUnique)
+        end
       end
 
       context "when the file is too big" do

decidim-meetings/app/commands/decidim/meetings/admin/invite_user_to_join_meeting.rb

@@ -40,6 +40,7 @@ def invite_user
             InviteJoinMeetingMailer.invite(user, meeting, invited_by).deliver_later
           else
             user.name = form.name
+            user.nickname = User.nicknamize(user.name)
             user.skip_reconfirmation!
             user.invite!(invited_by, invitation_instructions: "join_meeting", meeting: meeting)
           end

decidim-system/spec/commands/decidim/system/register_organization_spec.rb

@@ -47,6 +47,7 @@ module System
             expect(admin.organization.name).to eq("Gotham City")
             expect(admin).to be_admin
             expect(admin).to be_created_by_invite
+            expect(admin).to be_valid
           end
 
           it "sends a custom email" do

lib/generators/decidim/install_generator.rb

@@ -108,13 +108,8 @@ def letter_opener_web
       def recreate_db
         soft_rails "db:environment:set", "db:drop"
         rails "db:create"
-
-        if options[:seed_db]
-          rails "db:migrate", "db:seed"
-        else
-          rails "db:migrate"
-        end
-
+        rails "db:migrate"
+        rails "db:seed" if options[:seed_db]
         rails "db:test:prepare"
       end