Merge pull request #109 from azul/feature/clear-all-data-of-deleted-user

Feature/clear data of deleted user

15 files changed, 189 insertions, 57 deletions

diff --git a/help/app/models/account_extension/tickets.rb b/help/app/models/account_extension/tickets.rb
new file mode 100644
index 0000000..f898b56
--- /dev/null
+++ b/help/app/models/account_extension/tickets.rb
@@ -0,0 +1,13 @@
+module AccountExtension::Tickets
+  extend ActiveSupport::Concern
+
+  def destroy_with_tickets
+    Ticket.destroy_all_from(self.user)
+    destroy_without_tickets
+  end
+
+  included do
+    alias_method_chain :destroy, :tickets
+  end
+
+end
diff --git a/help/app/models/ticket.rb b/help/app/models/ticket.rb
index 8066d0d..74f2050 100644
--- a/help/app/models/ticket.rb
+++ b/help/app/models/ticket.rb
@@ -24,6 +24,7 @@ class Ticket < CouchRest::Model::Base
   design do
     view :by_updated_at
     view :by_created_at
+    view :by_created_by
 
     view :by_is_open_and_created_at
     view :by_is_open_and_updated_at
@@ -40,6 +41,12 @@ class Ticket < CouchRest::Model::Base
     @selection.tickets
   end
 
+  def self.destroy_all_from(user)
+    self.by_created_by.key(user.id).each do |ticket|
+      ticket.destroy
+    end
+  end
+
   def is_creator_validated?
     !!created_by
   end
diff --git a/help/config/initializers/account_lifecycle.rb b/help/config/initializers/account_lifecycle.rb
new file mode 100644
index 0000000..d9f04c1
--- /dev/null
+++ b/help/config/initializers/account_lifecycle.rb
@@ -0,0 +1,3 @@
+ActiveSupport.on_load(:account) do
+  include AccountExtension::Tickets
+end
diff --git a/help/test/factories.rb b/help/test/factories.rb
index 5b38952..bce3af1 100644
--- a/help/test/factories.rb
+++ b/help/test/factories.rb
@@ -2,8 +2,16 @@ FactoryGirl.define do
 
   factory :ticket do
     title { Faker::Lorem.sentence }
-    comments_attributes do
-      { "0" => { "body" => Faker::Lorem.sentences.join(" ") } }
+    email { Faker::Internet.email }
+
+    factory :ticket_with_comment do
+      comments_attributes do
+        { "0" => { "body" => Faker::Lorem.sentences.join(" ") } }
+      end
+    end
+
+    factory :ticket_with_creator do
+      created_by { FactoryGirl.create(:user).id }
     end
   end
 
diff --git a/help/test/unit/account_extension_test.rb b/help/test/unit/account_extension_test.rb
new file mode 100644
index 0000000..aba162c
--- /dev/null
+++ b/help/test/unit/account_extension_test.rb
@@ -0,0 +1,12 @@
+require 'test_helper'
+
+class AccountExtensionTest < ActiveSupport::TestCase
+
+  test "destroying an account triggers ticket destruction" do
+    t = FactoryGirl.create :ticket_with_creator
+    u = t.created_by_user
+    Account.new(u).destroy
+    assert_equal nil, Ticket.find(t.id)
+  end
+
+end
diff --git a/help/test/unit/ticket_test.rb b/help/test/unit/ticket_test.rb
index ce35e1d..751fe70 100644
--- a/help/test/unit/ticket_test.rb
+++ b/help/test/unit/ticket_test.rb
@@ -1,47 +1,45 @@
 require 'test_helper'
 
 class TicketTest < ActiveSupport::TestCase
-  #test "the truth" do
-  #  assert true
-  #end
 
-  setup do
-    @sample = Ticket.new
+  test "ticket with default attribs is valid" do
+    t = FactoryGirl.build :ticket
+    assert t.valid?
   end
 
-  test "validity" do
-    t = Ticket.create :title => 'test title', :email => 'blah@blah.com'
+  test "ticket without email is valid" do
+    t = FactoryGirl.build :ticket, email: ""
     assert t.valid?
-    assert_equal t.title, 'test title'
+  end
 
+  test "ticket validates email format" do
+    t = FactoryGirl.build :ticket, email: "aswerssfd"
+    assert !t.valid?
+  end
+
+  test "ticket open states" do
+    t = FactoryGirl.build :ticket
     assert t.is_open
     t.close
     assert !t.is_open
     t.reopen
     assert t.is_open
-    #user = LeapWebHelp::User.new(User.valid_attributes_hash)
-    #user = LeapWebUsers::User.create
-
-    #t.user = user
-
-    #t.email = '' #invalid
-    #assert !t.valid?
-    #t.email = 'blah@blah.com, bb@jjj.org'
-    #assert t.valid?
-    t.email = 'bdlfjlkasfjklasjf' #invalid
-    #p t.email_address
-    #p t.email_address.strip =~ RFC822::EmailAddress
-    assert !t.valid?
-    t.reload.destroy
   end
 
   test "creation validated" do
+    @sample = Ticket.new
     assert !@sample.is_creator_validated?
     #p current_user
     @sample.created_by = 22 #current_user
     assert @sample.is_creator_validated?
   end
 
+  test "destroy all tickets from a user" do
+    t = FactoryGirl.create :ticket_with_creator
+    u = t.created_by_user
+    Ticket.destroy_all_from(u)
+    assert_equal nil, Ticket.find(t.id)
+  end
 =begin
 # TODO: do once have current_user stuff in order
   test "code if & only if not creator-validated" do
diff --git a/users/app/controllers/users_controller.rb b/users/app/controllers/users_controller.rb
index de21983..3cbb6dc 100644
--- a/users/app/controllers/users_controller.rb
+++ b/users/app/controllers/users_controller.rb
@@ -47,7 +47,7 @@ class UsersController < UsersBaseController
   end
 
   def destroy
-    @user.destroy
+    @user.account.destroy
     flash[:notice] = I18n.t(:account_destroyed)
     # admins can destroy other users
     if @user != current_user
diff --git a/users/app/controllers/v1/users_controller.rb b/users/app/controllers/v1/users_controller.rb
index 03a5a62..0903888 100644
--- a/users/app/controllers/v1/users_controller.rb
+++ b/users/app/controllers/v1/users_controller.rb
@@ -24,15 +24,9 @@ module V1
     end
 
     def update
-      account.update params[:user]
+      @user.account.update params[:user]
       respond_with @user
     end
 
-    protected
-
-    def account
-      @user.account
-    end
-
   end
 end
diff --git a/users/app/models/account.rb b/users/app/models/account.rb
index 5368a1b..5c943bb 100644
--- a/users/app/models/account.rb
+++ b/users/app/models/account.rb
@@ -1,5 +1,10 @@
 #
-# A Composition of a User record and it's identity records.
+# The Account model takes care of the livecycle of a user.
+# It composes a User record and it's identity records.
+# It also allows for other engines to hook into the livecycle by
+# monkeypatching the create, update and destroy methods.
+# There's an ActiveSupport load_hook at the end of this file to
+# make this more easy.
 #
 class Account
 
@@ -29,9 +34,7 @@ class Account
 
   def destroy
     return unless @user
-    Identity.by_user_id.key(@user.id).each do |identity|
-      identity.destroy
-    end
+    Identity.disable_all_for(@user)
     @user.destroy
   end
 
@@ -54,4 +57,7 @@ class Account
     @new_identity.try(:save) && @old_identity.try(:save)
   end
 
+  # You can hook into the account lifecycle from different engines using
+  #   ActiveSupport.on_load(:account) do ...
+  ActiveSupport.run_load_hooks(:account, self)
 end
diff --git a/users/app/models/identity.rb b/users/app/models/identity.rb
index e0a24e9..97966d0 100644
--- a/users/app/models/identity.rb
+++ b/users/app/models/identity.rb
@@ -27,6 +27,17 @@ class Identity < CouchRest::Model::Base
         emit(doc.address, doc.keys["pgp"]);
       }
     EOJS
+    view :disabled,
+      map: <<-EOJS
+      function(doc) {
+        if (doc.type != 'Identity') {
+          return;
+        }
+        if (typeof doc.user_id === "undefined") {
+          emit(doc._id, 1);
+        }
+      }
+    EOJS
 
   end
 
@@ -50,6 +61,19 @@ class Identity < CouchRest::Model::Base
     identity
   end
 
+  def self.disable_all_for(user)
+    Identity.by_user_id.key(user.id).each do |identity|
+      identity.disable
+      identity.save
+    end
+  end
+
+  def self.destroy_all_disabled
+    Identity.disabled.each do |identity|
+      identity.destroy
+    end
+  end
+
   def self.attributes_from_user(user)
     { user_id: user.id,
       address: user.email_address,
@@ -57,6 +81,15 @@ class Identity < CouchRest::Model::Base
     }
   end
 
+  def enabled?
+    self.destination && self.user_id
+  end
+
+  def disable
+    self.destination = nil
+    self.user_id = nil
+  end
+
   def keys
     read_attribute('keys') || HashWithIndifferentAccess.new
   end
@@ -93,7 +126,8 @@ class Identity < CouchRest::Model::Base
   end
 
   def destination_email
-    return if destination.valid? #this ensures it is Email
+    return if destination.nil?   # this identity is disabled
+    return if destination.valid? # this ensures it is Email
     self.errors.add(:destination, destination.errors.messages[:email].first) #assumes only one error #TODO
   end
 
diff --git a/users/test/functional/users_controller_test.rb b/users/test/functional/users_controller_test.rb
index 75d900f..9c5f8d9 100644
--- a/users/test/functional/users_controller_test.rb
+++ b/users/test/functional/users_controller_test.rb
@@ -77,7 +77,11 @@ class UsersControllerTest < ActionController::TestCase
 
   test "admin can destroy user" do
     user = find_record :user
+
+    # we destroy the user record and the associated data...
     user.expects(:destroy)
+    Identity.expects(:disable_all_for).with(user)
+    Ticket.expects(:destroy_all_from).with(user)
 
     login :is_admin? => true
     delete :destroy, :id => user.id
@@ -88,7 +92,11 @@ class UsersControllerTest < ActionController::TestCase
 
   test "user can cancel account" do
     user = find_record :user
+
+    # we destroy the user record and the associated data...
     user.expects(:destroy)
+    Identity.expects(:disable_all_for).with(user)
+    Ticket.expects(:destroy_all_from).with(user)
 
     login user
     expect_logout
diff --git a/users/test/integration/browser/account_test.rb b/users/test/integration/browser/account_test.rb
index b712c95..b349489 100644
--- a/users/test/integration/browser/account_test.rb
+++ b/users/test/integration/browser/account_test.rb
@@ -6,6 +6,10 @@ class AccountTest < BrowserIntegrationTest
     Capybara.current_driver = Capybara.javascript_driver
   end
 
+  teardown do
+    Identity.destroy_all_disabled
+  end
+
   test "normal account workflow" do
     username, password = submit_signup
     assert page.has_content?("Welcome #{username}")
@@ -19,31 +23,32 @@ class AccountTest < BrowserIntegrationTest
   test "successful login" do
     username, password = submit_signup
     click_on 'Logout'
-    click_on 'Log In'
-    fill_in 'Username', with: username
-    fill_in 'Password', with: password
-    click_on 'Log In'
+    attempt_login(username, password)
     assert page.has_content?("Welcome #{username}")
     User.find_by_login(username).account.destroy
   end
 
   test "failed login" do
     visit '/'
-    click_on 'Log In'
-    fill_in 'Username', with: "username"
-    fill_in 'Password', with: "wrong password"
-    click_on 'Log In'
-    assert page.has_selector? 'input.btn-primary.disabled'
-    assert page.has_content? I18n.t(:invalid_user_pass)
-    assert page.has_no_selector? 'input.btn-primary.disabled'
+    attempt_login("username", "wrong password")
+    assert_invalid_login(page)
   end
 
   test "account destruction" do
     username, password = submit_signup
     click_on I18n.t('account_settings')
     click_on I18n.t('destroy_my_account')
-    page.save_screenshot('/tmp/destroy.png')
     assert page.has_content?(I18n.t('account_destroyed'))
+    attempt_login(username, password)
+    assert_invalid_login(page)
+  end
+
+  test "handle blocked after account destruction" do
+    username, password = submit_signup
+    click_on I18n.t('account_settings')
+    click_on I18n.t('destroy_my_account')
+    submit_signup(username)
+    assert page.has_content?('has already been taken')
   end
 
   test "change password" do
@@ -55,10 +60,7 @@ class AccountTest < BrowserIntegrationTest
       click_on 'Save'
     end
     click_on 'Logout'
-    click_on 'Log In'
-    fill_in 'Username', with: username
-    fill_in 'Password', with: "other password"
-    click_on 'Log In'
+    attempt_login(username, "other password")
     assert page.has_content?("Welcome #{username}")
     User.find_by_login(username).account.destroy
   end
@@ -100,6 +102,19 @@ class AccountTest < BrowserIntegrationTest
     assert page.has_content?("server failed")
   end
 
+  def attempt_login(username, password)
+    click_on 'Log In'
+    fill_in 'Username', with: username
+    fill_in 'Password', with: password
+    click_on 'Log In'
+  end
+
+  def assert_invalid_login(page)
+    assert page.has_selector? 'input.btn-primary.disabled'
+    assert page.has_content? I18n.t(:invalid_user_pass)
+    assert page.has_no_selector? 'input.btn-primary.disabled'
+  end
+
   def inject_malicious_js
     page.execute_script <<-EOJS
       var calc = new srp.Calculate();
diff --git a/users/test/support/integration_test_helper.rb b/users/test/support/integration_test_helper.rb
index cfe72cf..51e47c6 100644
--- a/users/test/support/integration_test_helper.rb
+++ b/users/test/support/integration_test_helper.rb
@@ -1,7 +1,7 @@
 module IntegrationTestHelper
-  def submit_signup
-    username = "test_#{SecureRandom.urlsafe_base64}".downcase
-    password = SecureRandom.base64
+  def submit_signup(username = nil, password = nil)
+    username ||= "test_#{SecureRandom.urlsafe_base64}".downcase
+    password ||= SecureRandom.base64
     visit '/users/new'
     fill_in 'Username', with: username
     fill_in 'Password', with: password
diff --git a/users/test/unit/account_test.rb b/users/test/unit/account_test.rb
index 94a9980..4fb3c3d 100644
--- a/users/test/unit/account_test.rb
+++ b/users/test/unit/account_test.rb
@@ -2,6 +2,10 @@ require 'test_helper'
 
 class AccountTest < ActiveSupport::TestCase
 
+  teardown do
+    Identity.destroy_all_disabled
+  end
+
   test "create a new account" do
     user = Account.create(FactoryGirl.attributes_for(:user))
     assert user.valid?
@@ -13,7 +17,8 @@ class AccountTest < ActiveSupport::TestCase
   end
 
   test "create and remove a user account" do
-    assert_no_difference "Identity.count" do
+    # We keep an identity that will block the handle from being reused.
+    assert_difference "Identity.count" do
       assert_no_difference "User.count" do
         user = Account.create(FactoryGirl.attributes_for(:user))
         user.account.destroy
diff --git a/users/test/unit/identity_test.rb b/users/test/unit/identity_test.rb
index 0842a77..eca104f 100644
--- a/users/test/unit/identity_test.rb
+++ b/users/test/unit/identity_test.rb
@@ -90,6 +90,35 @@ class IdentityTest < ActiveSupport::TestCase
     assert id.errors.messages[:destination].include? "needs to be a valid email address"
   end
 
+  test "disabled identity" do
+    id = Identity.for(@user)
+    id.disable
+    assert_equal @user.email_address, id.address
+    assert_equal nil, id.destination
+    assert_equal nil, id.user
+    assert !id.enabled?
+    assert id.valid?
+  end
+
+  test "disabled identity blocks handle" do
+    id = Identity.for(@user)
+    id.disable
+    id.save
+    other_user = find_record :user
+    taken = Identity.build_for other_user, address: id.address
+    assert !taken.valid?
+    Identity.destroy_all_disabled
+  end
+
+  test "destroy all disabled identities" do
+    id = Identity.for(@user)
+    id.disable
+    id.save
+    assert Identity.count > 0
+    Identity.destroy_all_disabled
+    assert_equal 0, Identity.disabled.count
+  end
+
   def alias_name
     @alias_name ||= Faker::Internet.user_name
   end