diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 0bdd2a801..81eaa08cb 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -41,9 +41,7 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController end def save_user(user) - @user.save || - @user.save_requiring_finish_signup || - @user.save_requiring_finish_signup_without_email + @user.save || @user.save_requiring_finish_signup end end diff --git a/app/models/user.rb b/app/models/user.rb index f7980c796..ea889ac57 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -26,7 +26,7 @@ class User < ActiveRecord::Base belongs_to :geozone validates :username, presence: true, if: :username_required? - validates :username, uniqueness: true, if: :username_required? + validates :username, uniqueness: { scope: :registering_with_oauth }, if: :username_required? validates :document_number, uniqueness: { scope: :document_type }, allow_nil: true validate :validate_username_length @@ -154,6 +154,7 @@ class User < ActiveRecord::Base confirmed_phone: nil, unconfirmed_phone: nil ) + self.identities.destroy_all end def erased? @@ -182,11 +183,11 @@ class User < ActiveRecord::Base end def username_required? - !organization? && !erased? && !registering_with_oauth + !organization? && !erased? end def email_required? - !erased? && !registering_with_oauth + !erased? end def has_official_email? @@ -215,11 +216,15 @@ class User < ActiveRecord::Base end def save_requiring_finish_signup - self.update(registering_with_oauth: true) - end - - def save_requiring_finish_signup_without_email - self.update(registering_with_oauth: true, email: nil) + begin + self.registering_with_oauth = true + self.save(validate: false) + # Devise puts unique constraints for the email the db, so we must detect & handle that + rescue ActiveRecord::RecordNotUnique + self.email = nil + self.save(validate: false) + end + true end def ability diff --git a/app/views/users/registrations/finish_signup.html.erb b/app/views/users/registrations/finish_signup.html.erb index fa7bb621e..79f160a51 100644 --- a/app/views/users/registrations/finish_signup.html.erb +++ b/app/views/users/registrations/finish_signup.html.erb @@ -3,6 +3,10 @@ <%= form_for current_user, as: :user, url: do_finish_signup_path, html: { role: 'form'} do |f| %> <%= render 'shared/errors', resource: current_user %> +
+ <%= t("omniauth.finish_signup.username_warning") %> +
+ <% if current_user.errors.include? :username %> <%= f.text_field :username, placeholder: t("devise_views.users.registrations.new.username_label") %> <% else %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 8609c079c..b5899e63a 100755 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -542,6 +542,7 @@ en: omniauth: finish_signup: title: "Additional details" + username_warning: "Due to a change in the way we interact with social networks, it is possible that your username now appears as 'already in use'. If that is your case, please choose a different username." twitter: sign_in: "Sign in with Twitter" sign_up: "Sign up with Twitter" diff --git a/config/locales/es.yml b/config/locales/es.yml index a15528d5a..f1241bb15 100755 --- a/config/locales/es.yml +++ b/config/locales/es.yml @@ -542,6 +542,7 @@ es: omniauth: finish_signup: title: "Detalles adicionales de tu cuenta" + username_warning: "Debido a que hemos cambiado la forma en la que nos conectamos con redes sociales y es posible que tu nombre de usuario aparezca como 'ya en uso', incluso si antes podías acceder con él. Si es tu caso, por favor elige un nombre de usuario distinto." twitter: sign_in: "Entra con Twitter" sign_up: "Regístrate con Twitter" diff --git a/db/migrate/20160418144013_add_index_to_username.rb b/db/migrate/20160418144013_add_index_to_username.rb new file mode 100644 index 000000000..191f24ac6 --- /dev/null +++ b/db/migrate/20160418144013_add_index_to_username.rb @@ -0,0 +1,5 @@ +class AddIndexToUsername < ActiveRecord::Migration + def change + add_index :users, :username + end +end diff --git a/db/schema.rb b/db/schema.rb index 223a14d47..49f0011f2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -432,6 +432,7 @@ ActiveRecord::Schema.define(version: 20160418172919) do add_index "users", ["geozone_id"], name: "index_users_on_geozone_id", using: :btree add_index "users", ["hidden_at"], name: "index_users_on_hidden_at", using: :btree add_index "users", ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true, using: :btree + add_index "users", ["username"], name: "index_users_on_username", using: :btree create_table "valuation_assignments", force: :cascade do |t| t.integer "valuator_id" diff --git a/lib/tasks/users.rake b/lib/tasks/users.rake index c56df2dd0..efbeeaa94 100644 --- a/lib/tasks/users.rake +++ b/lib/tasks/users.rake @@ -59,4 +59,21 @@ namespace :users do end end + desc "Makes duplicate username users change their username" + task social_network_reset: :environment do + duplicated_usernames = User.all.select(:username).group(:username).having('count(username) > 1').pluck(:username) + + duplicated_usernames.each do |username| + print "." + user_ids = User.where(username: username).order(created_at: :asc).pluck(:id) + user_ids_to_review = Identity.where(user_id: user_ids).pluck(:user_id) + user_ids_to_review.shift if user_ids.size == user_ids_to_review.size + user_ids_to_review.each { |id| User.find(id).update(registering_with_oauth: true) } + end + end + + desc "Removes identities associated to erased users" + task remove_erased_identities: :environment do + Identity.joins(:user).where('users.erased_at IS NOT NULL').destroy_all + end end diff --git a/spec/features/users_auth_spec.rb b/spec/features/users_auth_spec.rb index a1c9374be..f9b92bb12 100644 --- a/spec/features/users_auth_spec.rb +++ b/spec/features/users_auth_spec.rb @@ -169,6 +169,12 @@ feature 'Users' do expect(current_path).to eq(finish_signup_path) + expect(page).to have_field('user_username', with: 'manuela') + + click_button 'Register' + + expect(current_path).to eq(do_finish_signup_path) + fill_in 'user_username', with: 'manuela2' click_button 'Register' diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 4d1b269ec..b49e3cc6b 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -333,6 +333,7 @@ describe User do email_verification_token: "token3", confirmed_phone:"5678", unconfirmed_phone:"5678") + user.erase('a test') user.reload @@ -353,6 +354,14 @@ describe User do expect(user.confirmation_token).to be_nil expect(user.reset_password_token).to be_nil expect(user.email_verification_token).to be_nil + + end + + it "destroys associated identities" do + user = create(:user) + identity = create(:identity, user: user) + user.erase('an identity test') + expect(Identity.exists?(identity.id)).to_not be end end