diff options
| -rw-r--r-- | README.md | 2 | ||||
| -rw-r--r-- | help/app/controllers/tickets_controller.rb | 11 | ||||
| -rw-r--r-- | help/app/models/account_extension/tickets.rb | 13 | ||||
| -rw-r--r-- | help/app/models/ticket.rb | 7 | ||||
| -rw-r--r-- | help/config/initializers/account_lifecycle.rb | 3 | ||||
| -rw-r--r-- | help/test/factories.rb | 12 | ||||
| -rw-r--r-- | help/test/unit/account_extension_test.rb | 12 | ||||
| -rw-r--r-- | help/test/unit/ticket_test.rb | 42 | ||||
| -rw-r--r-- | users/app/controllers/users_controller.rb | 2 | ||||
| -rw-r--r-- | users/app/controllers/v1/users_controller.rb | 8 | ||||
| -rw-r--r-- | users/app/models/account.rb | 14 | ||||
| -rw-r--r-- | users/app/models/identity.rb | 36 | ||||
| -rw-r--r-- | users/test/functional/users_controller_test.rb | 8 | ||||
| -rw-r--r-- | users/test/integration/browser/account_test.rb | 47 | ||||
| -rw-r--r-- | users/test/support/integration_test_helper.rb | 6 | ||||
| -rw-r--r-- | users/test/unit/account_test.rb | 7 | ||||
| -rw-r--r-- | users/test/unit/identity_test.rb | 29 |
17 files changed, 194 insertions, 65 deletions
@@ -41,7 +41,7 @@ Typically, this application is installed automatically as part of the LEAP Platf ### Install system requirements - sudo apt-get install git ruby1.8 rubygems1.8 couchdb + sudo apt-get install git ruby1.9.3 rubygems couchdb sudo gem install bundler On Debian Wheezy or later, there is a Debian package for bundler, so you can alternately run ``sudo apt-get install bundler``. diff --git a/help/app/controllers/tickets_controller.rb b/help/app/controllers/tickets_controller.rb index a669e19..c193ff4 100644 --- a/help/app/controllers/tickets_controller.rb +++ b/help/app/controllers/tickets_controller.rb @@ -62,14 +62,11 @@ class TicketsController < ApplicationController @ticket.comments.last.private = false unless admin? end - if @ticket.changed? - if @ticket.save - flash[:notice] = t(:changes_saved) - redirect_to_tickets - else - respond_with @ticket - end + if @ticket.changed? and @ticket.save + flash[:notice] = t(:changes_saved) + redirect_to_tickets else + flash[:error] = @ticket.errors.full_messages.join(". ") if @ticket.changed? redirect_to auto_ticket_path(@ticket) end end 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 |