Merge pull request #1072 from consul/oauth-signup-fix

Oauth signup fix

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 || @user.save_requiring_finish_signup
-      @user.save_requiring_finish_signup ||
-      @user.save_requiring_finish_signup_without_email
     end
 
 end

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)
+    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
-  def save_requiring_finish_signup_without_email
-    self.update(registering_with_oauth: true, email: nil)
   end
 
   def ability

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 %>
 
+  <div class='callout primary'>
+    <%= t("omniauth.finish_signup.username_warning") %>
+  </div>
+
   <% if current_user.errors.include? :username %>
     <%= f.text_field :username, placeholder: t("devise_views.users.registrations.new.username_label") %>
   <% else %>

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"

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"

db/migrate/20160418144013_add_index_to_username.rb

@@ -0,0 +1,5 @@
+class AddIndexToUsername < ActiveRecord::Migration
+  def change
+    add_index :users, :username
+  end
+end

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"

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

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'
 

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