forgot_password: search by all fields instead of field selection

Rodrigo Souto
1 parent 25df24bd
Showing 7 changed files with 60 additions and 91 deletions Show diff stats
app/models/change_password.rb
app/views/account/forgot_password.rhtml
lib/noosfero/plugin.rb
plugins/stoa/lib/stoa_plugin.rb
plugins/stoa/test/functional/account_controller_test.rb
test/functional/account_controller_test.rb
test/unit/change_password_test.rb
 class ChangePassword < Task
-  settings_items :field, :value
+  settings_items :value
   attr_accessor :password, :password_confirmation, :environment_id
   include Noosfero::Plugin::HotSpot
   def self.human_attribute_name(attrib)
     case attrib.to_sym
-    when :field
-      _('Field')
     when :value
       _('Value')
     when :password
@@ -20,49 +18,54 @@ class ChangePassword &lt; Task
     end
   end
-  def plugins_fields
-    plugins.dispatch(:change_password_fields).inject({}) { |result, fields| result.merge!(fields)}
-  end
-
   def environment
     (requestor.environment if requestor) || Environment.find_by_id(environment_id)
   end
+  def plugins_options
+    plugins.dispatch(:change_password_fields)
+  end
+
+  def user_fields
+    %w[login email] + plugins_options.select {|options| options[:model].to_sym == :user }.map { |options| options[:field].to_s }
+  end
+
+  def person_fields
+    %w[] + plugins_options.select {|options| options[:model].to_sym == :person }.map { |options| options[:field].to_s }
+  end
+
   def fields
-    %w[login email] + plugins_fields.map { |field, name| field.to_s }
+    user_fields + person_fields
   end
-  def fields_choice
-    [
-      [_('Username'), 'login'],
-      [_('Email'), 'email'],
-    ] + plugins_fields.map { |field, name| [name, field] }
+  def fields_label
+    labels = [
+      _('Username'),
+      _('Email'),
+    ] + plugins_options.map { |options| options[:name] }
+
+    last = labels.pop
+    label = labels.join(', ')
+    "#{label} #{_('or')} #{last}"
   end
   ###################################################
   # validations for creating a ChangePassword task 
-  validates_presence_of :field, :value, :environment_id, :on => :create, :message => _('must be filled in')
-  # TODO Only on rails3
-  # validates_inclusion_of :field, :in => lambda { |data| data.fields }
-  validates_each :field do |data, attr, value|
-    unless data.fields.include?(value)
-      data.errors.add(attr, _('is not in the list of valid fields.'))
-    end
-  end
+  validates_presence_of :value, :environment_id, :on => :create, :message => _('must be filled in')
   validates_each :value, :on => :create do |data,attr,value|
-    unless data.field.blank? || data.value.blank?
-      user = data.user_find
-      if user.nil? 
-        data.errors.add(:value, _('"%s" is not a valid %s.') % [value.to_s, human_attribute_name(data.field)])
+    unless data.value.blank?
+      users = data.find_users
+      if users.blank?
+        data.errors.add(:value, _('"%s" is not valid.') % value.to_s)
       end
     end
   end
   before_validation do |change_password|
-    user = change_password.user_find
-    change_password.requestor = user.person if user
+    users = change_password.find_users
+    change_password.requestor ||= users.first.person if users.present?
   end
   ###################################################
@@ -73,15 +76,18 @@ class ChangePassword &lt; Task
   validates_presence_of :password_confirmation, :on => :update, :if => lambda { |change| change.status != Task::Status::CANCELLED }
   validates_confirmation_of :password, :if => lambda { |change| change.status != Task::Status::CANCELLED }
-  def user_find
-    begin
-      method = "find_by_#{field}_and_environment_id"
-      user = nil
-      user = User.send(method, value, environment_id) if User.respond_to?(method)
-      user = Person.send(method, value, environment_id).user if user.nil? && Person.respond_to?(method)
-    rescue
-    end
-    user
+  def build_query(fields)
+    fields.map {|field| "#{field} = '#{value}'"}.join(' OR ')
+  end
+
+  def find_users
+    results = []
+    person_query = build_query(person_fields)
+    user_query = build_query(user_fields)
+
+    results += Person.where(person_query).where(:environment_id => environment.id).map(&:user)
+    results += User.where(user_query).where(:environment_id => environment.id)
+    results
   end
   def title
@@ -4,8 +4,7 @@
 <% labelled_form_for :change_password, @change_password, :url => { :action => 'forgot_password' } do |f| %>
-  <%= labelled_form_field(_('What do you remember?'), f.select(:field, @change_password.fields_choice)) %>
-  <%= text_field_tag('change_password[value]') %>
+  <%= labelled_form_field @change_password.fields_label, text_field_tag('change_password[value]') %>
 <div>
   <% button_bar do %>
@@ -461,7 +461,7 @@ class Noosfero::Plugin
   end
   # -> Adds aditional fields for change_password
-  # returns = {:field1 => 'field1 name', :field2 => 'field2 name' ... }
+  # returns = [{:field => 'field1', :name => 'field 1 name', :model => 'person'}, {...}]
   def change_password_fields
     nil
   end
@@ -121,7 +121,7 @@ class StoaPlugin &lt; Noosfero::Plugin
   end
   def change_password_fields
-    {:usp_id => _('USP Number')}
+    {:field => :usp_id, :name => _('USP Number'), :model => 'person'}
   end
 end
@@ -67,7 +67,7 @@ class AccountControllerTest &lt; ActionController::TestCase
   end
   should 'be able to recover password with usp_id' do
-    post :forgot_password, :change_password => { :field => 'usp_id', :value => '87654321' }
+    post :forgot_password, :change_password => { :value => '87654321' }
     assert_template 'password_recovery_sent'
   end
 end
@@ -217,19 +217,19 @@ class AccountControllerTest &lt; ActionController::TestCase
   should 'respond to forgotten password change request with login' do
     change = ChangePassword.new
-    ChangePassword.expects(:new).with('field' => 'login', 'value' => 'test').returns(change)
+    ChangePassword.expects(:new).with('value' => 'test').returns(change)
     change.expects(:save!).returns(true)
-    post :forgot_password, :change_password => { :field => 'login', :value => 'test' }
+    post :forgot_password, :change_password => { :value => 'test' }
     assert_template 'password_recovery_sent'
   end
   should 'respond to forgotten password change request with email' do
     change = ChangePassword.new
-    ChangePassword.expects(:new).with('field' => 'email', 'value' => 'test@localhost.localdomain').returns(change)
+    ChangePassword.expects(:new).with('value' => 'test@localhost.localdomain').returns(change)
     change.expects(:save!).returns(true)
-    post :forgot_password, :change_password => { :field => 'email', :value => 'test@localhost.localdomain' }
+    post :forgot_password, :change_password => { :value => 'test@localhost.localdomain' }
     assert_template 'password_recovery_sent'
   end
@@ -276,7 +276,7 @@ class AccountControllerTest &lt; ActionController::TestCase
   should 'require password confirmation correctly to enter new pasword' do
     user = create_user('testuser', :email => 'testuser@example.com', :password => 'test', :password_confirmation => 'test')
-    change = ChangePassword.create!(:field => 'login', :value => 'testuser', :environment_id => Environment.default.id)
+    change = ChangePassword.create!(:value => 'testuser', :environment_id => Environment.default.id)
     post :new_password, :code => change.code, :change_password => { :password => 'onepass', :password_confirmation => 'another_pass' }
     assert_response :success
@@ -862,7 +862,7 @@ class AccountControllerTest &lt; ActionController::TestCase
     assert_response :redirect
     #Redirect on post action
-    post :forgot_password, :change_password => { :field => 'login', :value => 'test' }
+    post :forgot_password, :change_password => { :value => 'test' }
     assert_response :redirect
   end
@@ -9,39 +9,7 @@ class ChangePasswordTest &lt; ActiveSupport::TestCase
     assert !data.valid?
   end
-  should 'validate field is login or email' do
-    data = ChangePassword.new(:environment_id => Environment.default)
-    data.field = 'anything'
-    data.valid?
-    assert data.errors.invalid?(:field)
-
-    data.field = 'login'
-    data.valid?
-    assert !data.errors.invalid?(:field)
-
-    data.field = 'email'
-    data.valid?
-    assert !data.errors.invalid?(:field)
-  end
-
-  should 'refuse invalid field' do
-    User.destroy_all
-
-    data = ChangePassword.new
-    data.environment_id = Environment.default.id
-
-    data.field = 'login'
-    data.value = 'unexisting'
-    data.valid?
-    assert data.errors.invalid?(:value)
-
-    data.field = 'email'
-    data.value = 'example@example.com'
-    data.valid?
-    assert data.errors.invalid?(:value)
-  end
-
-  should 'require only a valid field-value' do
+  should 'require only a valid value' do
     User.destroy_all
     create_user('testuser', :email => 'test@example.com')
@@ -50,12 +18,10 @@ class ChangePasswordTest &lt; ActiveSupport::TestCase
     assert !data.valid?
     assert data.errors.invalid?(:value)
-    data.field = 'login'
     data.value = 'testuser'
     data.valid?
     assert data.valid?
-    data.field = 'email'
     data.value = 'test@example.com'
     assert data.valid?
   end
@@ -65,7 +31,6 @@ class ChangePasswordTest &lt; ActiveSupport::TestCase
     change = ChangePassword.new
     change.environment_id = Environment.default.id
-    change.field = 'login'
     change.value = 'testuser'
     change.save!
@@ -86,7 +51,6 @@ class ChangePasswordTest &lt; ActiveSupport::TestCase
     change = ChangePassword.new
     change.environment_id = Environment.default.id
-    change.field = 'login'
     change.value = 'testuser'
     change.save!
@@ -105,7 +69,6 @@ class ChangePasswordTest &lt; ActiveSupport::TestCase
     change = ChangePassword.new
     change.environment_id = Environment.default.id
-    change.field = 'login'
     change.value = 'testuser'
     change.save!
@@ -127,8 +90,8 @@ class ChangePasswordTest &lt; ActiveSupport::TestCase
     p1 = create_user('sample-user', :password => 'test', :password_confirmation => 'test', :email => 'sample-user@test.com', :environment => e1).person
     p2 = create_user('sample-user', :password => 'test', :password_confirmation => 'test', :email => 'sample-user@test.com', :environment => e2).person
-    c1 = ChangePassword.create!(:field => 'login', :value => 'sample-user', :environment_id => e1.id)
-    c2 = ChangePassword.create!(:field => 'login', :value => 'sample-user', :environment_id => e2.id)
+    c1 = ChangePassword.create!(:value => 'sample-user', :environment_id => e1.id)
+    c2 = ChangePassword.create!(:value => 'sample-user', :environment_id => e2.id)
     assert_equal c1.requestor, p1
     assert_equal c2.requestor, p2
@@ -137,7 +100,7 @@ class ChangePasswordTest &lt; ActiveSupport::TestCase
   should 'have target notification description' do
     person = create_user('testuser').person
-    change = ChangePassword.create(:field => 'login', :value => 'testuser', :environment_id => Environment.default.id)
+    change = ChangePassword.create(:value => 'testuser', :environment_id => Environment.default.id)
     assert_match(/#{change.requestor.name} wants to change its password/, change.target_notification_description)
   end
@@ -145,7 +108,7 @@ class ChangePasswordTest &lt; ActiveSupport::TestCase
   should 'deliver task created message' do
     person = create_user('testuser').person
-    task = ChangePassword.create(:field => 'login', :value => 'testuser', :environment_id => Environment.default.id)
+    task = ChangePassword.create(:value => 'testuser', :environment_id => Environment.default.id)
     email = TaskMailer.deliver_task_created(task)
     assert_match(/#{task.requestor.name} wants to change its password/, email.subject)
@@ -154,12 +117,13 @@ class ChangePasswordTest &lt; ActiveSupport::TestCase
   should 'allow extra fields provided by plugins' do
     class Plugin1 < Noosfero::Plugin
       def change_password_fields
-        {:f1 => 'F1'}
+        {:field => 'f1', :name => 'F1', :model => 'person'}
       end
     end
     class Plugin2 < Noosfero::Plugin
       def change_password_fields
-        {:f2 => 'F2', :f3 => 'F3'}
+        [{:field => 'f2', :name => 'F2', :model => 'person'},
+         {:field => 'f3', :name => 'F3', :model => 'person'}]
       end
     end
	@@ -4,8 +4,7 @@		@@ -4,8 +4,7 @@
4		4
5	<% labelled_form_for :change_password, @change_password, :url => { :action => 'forgot_password' } do \|f\| %>	5	<% labelled_form_for :change_password, @change_password, :url => { :action => 'forgot_password' } do \|f\| %>
6		6
7	- <%= labelled_form_field(_('What do you remember?'), f.select(:field, @change_password.fields_choice)) %>
8	- <%= text_field_tag('change_password[value]') %>	7	+ <%= labelled_form_field @change_password.fields_label, text_field_tag('change_password[value]') %>
9		8
10	<div>	9	<div>
11	<% button_bar do %>	10	<% button_bar do %>
	@@ -461,7 +461,7 @@ class Noosfero::Plugin		@@ -461,7 +461,7 @@ class Noosfero::Plugin
461	end	461	end
462		462
463	# -> Adds aditional fields for change_password	463	# -> Adds aditional fields for change_password
464	- # returns = {:field1 => 'field1 name', :field2 => 'field2 name' ... }	464	+ # returns = [{:field => 'field1', :name => 'field 1 name', :model => 'person'}, {...}]
465	def change_password_fields	465	def change_password_fields
466	nil	466	nil
467	end	467	end
	@@ -121,7 +121,7 @@ class StoaPlugin < Noosfero::Plugin		@@ -121,7 +121,7 @@ class StoaPlugin < Noosfero::Plugin
121	end	121	end
122		122
123	def change_password_fields	123	def change_password_fields
124	- {:usp_id => _('USP Number')}	124	+ {:field => :usp_id, :name => _('USP Number'), :model => 'person'}
125	end	125	end
126		126
127	end	127	end
	@@ -67,7 +67,7 @@ class AccountControllerTest < ActionController::TestCase		@@ -67,7 +67,7 @@ class AccountControllerTest < ActionController::TestCase
67	end	67	end
68		68
69	should 'be able to recover password with usp_id' do	69	should 'be able to recover password with usp_id' do
70	- post :forgot_password, :change_password => { :field => 'usp_id', :value => '87654321' }	70	+ post :forgot_password, :change_password => { :value => '87654321' }
71	assert_template 'password_recovery_sent'	71	assert_template 'password_recovery_sent'
72	end	72	end
73	end	73	end
	@@ -217,19 +217,19 @@ class AccountControllerTest < ActionController::TestCase		@@ -217,19 +217,19 @@ class AccountControllerTest < ActionController::TestCase
217		217
218	should 'respond to forgotten password change request with login' do	218	should 'respond to forgotten password change request with login' do
219	change = ChangePassword.new	219	change = ChangePassword.new
220	- ChangePassword.expects(:new).with('field' => 'login', 'value' => 'test').returns(change)	220	+ ChangePassword.expects(:new).with('value' => 'test').returns(change)
221	change.expects(:save!).returns(true)	221	change.expects(:save!).returns(true)
222		222
223	- post :forgot_password, :change_password => { :field => 'login', :value => 'test' }	223	+ post :forgot_password, :change_password => { :value => 'test' }
224	assert_template 'password_recovery_sent'	224	assert_template 'password_recovery_sent'
225	end	225	end
226		226
227	should 'respond to forgotten password change request with email' do	227	should 'respond to forgotten password change request with email' do
228	change = ChangePassword.new	228	change = ChangePassword.new
229	- ChangePassword.expects(:new).with('field' => 'email', 'value' => 'test@localhost.localdomain').returns(change)	229	+ ChangePassword.expects(:new).with('value' => 'test@localhost.localdomain').returns(change)
230	change.expects(:save!).returns(true)	230	change.expects(:save!).returns(true)
231		231
232	- post :forgot_password, :change_password => { :field => 'email', :value => 'test@localhost.localdomain' }	232	+ post :forgot_password, :change_password => { :value => 'test@localhost.localdomain' }
233	assert_template 'password_recovery_sent'	233	assert_template 'password_recovery_sent'
234	end	234	end
235		235
	@@ -276,7 +276,7 @@ class AccountControllerTest < ActionController::TestCase		@@ -276,7 +276,7 @@ class AccountControllerTest < ActionController::TestCase
276		276
277	should 'require password confirmation correctly to enter new pasword' do	277	should 'require password confirmation correctly to enter new pasword' do
278	user = create_user('testuser', :email => 'testuser@example.com', :password => 'test', :password_confirmation => 'test')	278	user = create_user('testuser', :email => 'testuser@example.com', :password => 'test', :password_confirmation => 'test')
279	- change = ChangePassword.create!(:field => 'login', :value => 'testuser', :environment_id => Environment.default.id)	279	+ change = ChangePassword.create!(:value => 'testuser', :environment_id => Environment.default.id)
280		280
281	post :new_password, :code => change.code, :change_password => { :password => 'onepass', :password_confirmation => 'another_pass' }	281	post :new_password, :code => change.code, :change_password => { :password => 'onepass', :password_confirmation => 'another_pass' }
282	assert_response :success	282	assert_response :success
	@@ -862,7 +862,7 @@ class AccountControllerTest < ActionController::TestCase		@@ -862,7 +862,7 @@ class AccountControllerTest < ActionController::TestCase
862	assert_response :redirect	862	assert_response :redirect
863		863
864	#Redirect on post action	864	#Redirect on post action
865	- post :forgot_password, :change_password => { :field => 'login', :value => 'test' }	865	+ post :forgot_password, :change_password => { :value => 'test' }
866	assert_response :redirect	866	assert_response :redirect
867	end	867	end
868		868