diff --git a/lib/admin_actions/number_change.rb b/lib/admin_actions/number_change.rb index 13dd810f262809b328c74d8381e6f60cfb9cfb3c..f58ae7f8e449c3d2896a42a07f19d563b23d9111 100644 --- a/lib/admin_actions/number_change.rb +++ b/lib/admin_actions/number_change.rb @@ -12,15 +12,11 @@ class AdminAction using FormToH def self.for(target_customer, reply:) - unless (registration = target_customer.registered?) - return EMPromise.reject("Customer not registered") - end - EMPromise.resolve( new( customer_id: target_customer.customer_id, - old_backend: target_customer.sgx.strip!.to_s, - old_tel: registration.phone + old_backend: target_customer.sgx&.strip!&.to_s, + old_tel: (reg = target_customer.registered?) ? reg.phone : nil ) ).then { |x| reply.call(x.form).then(&x.method(:change)) } end @@ -47,17 +43,22 @@ class AdminAction end end - NilKey = Struct.new(:key) { + class Orphan < StandardError def to_s - "Expected a key with a value, but #{key} has no value." + "Can't register nil tel to any backend" + end + end + + class UnknownBackend < StandardError + def initialize(backend:, expected:) + @backend = backend + @expected = expected end - } - UnknownBackend = Struct.new(:backend, :expected) { def to_s - "Got unknown backend: #{backend}, expected one of #{expected}" + "Got unknown backend: #{@backend}, expected one of #{@expected}" end - } + end def customer_id @attributes[:customer_id] @@ -84,37 +85,49 @@ class AdminAction end def check_forward - EMPromise.all([check_noop, check_cat_jid, check_backend]) + # Without this ordering, it's possible for + # check_noop and check_orphan to race, + # which creates ambiguity around whether + # an actual error will raise or not. + check_orphan.then { + EMPromise.all([check_noop, check_backend]) + } end def forward - repo = TrivialBackendSgxRepo.new(redis: REDIS) EMPromise.all([ - new_tel != old_tel && change_catapult_fwd!, - repo.put(customer_id, new_backend).then { |sgx| - sgx.register!(new_tel) - }, + old_tel && new_tel != old_tel && change_catapult_fwd!, + update_tel_and_backend, should_disconnect_old_number? && disconnect_bandwidth_number ]).then { self } end + def update_tel_and_backend + TrivialBackendSgxRepo.new(redis: REDIS).put( + customer_id, new_backend, + ).then { |sgx| sgx.register!(new_tel) } + end + + # If old_tel exists, it's still possible no catapult_fwd has been set. + # However, if one exists, we should rename it. + # If old_tel does not exist, it's not possible for catapult_fwd + # to exist, and it's not possible for there to be value. + # to set catapult_fwd to. That's out-of-scope for this command. def change_catapult_fwd! REDIS.rename("catapult_fwd-#{old_tel}", "catapult_fwd-#{new_tel}") end def to_reverse with( - old_tel: new_tel, - new_tel: old_tel, - old_backend: new_backend, - new_backend: old_backend + old_tel: new_tel, new_tel: old_tel, + old_backend: new_backend, new_backend: old_backend ) end def to_s base = "Change Number\n" base += " [move backend?]: #{old_backend} -> #{new_backend}\n" - base += " [change number?]: #{old_tel} -> #{new_tel}" + base += " [change number?]: #{old_tel || 'nil'} -> #{new_tel}" base + delete_warning end @@ -144,11 +157,8 @@ class AdminAction end end - def check_cat_jid - cat_jid = "catapult_jid-#{old_tel}" - REDIS.exists(cat_jid).then { |v| - EMPromise.reject(NilKey.new(cat_jid)) unless v == 1 - } + def check_orphan + EMPromise.reject(Orphan.new) unless new_tel end def check_backend diff --git a/test/test_admin_command.rb b/test/test_admin_command.rb index 6d8ec8110287fe9193a9a77e045be1e800b77153..8cfb031aac872c6bc966ee7e43e95589a6d30be5 100644 --- a/test/test_admin_command.rb +++ b/test/test_admin_command.rb @@ -431,26 +431,181 @@ class AdminCommandTest < Minitest::Test end em :test_action_cancel_account_keep_number - def test_change_num_for_unregistered_customer - execute_command { |exe| - sgx = Minitest::Mock.new(OpenStruct.new( - registered?: nil, - jid: Blather::JID.new(CONFIG[:sgx]) # Uses Bandwidth backend - )) - target_customer = customer(sgx: sgx) - admin = AdminCommand.for(target_customer, exe.customer_repo) + def test_change_num_for_unregistered_customer + bandwidth_tn_repo_mock = setup_bandwidth_tn_repo_mock( + should_disconnect: false + ) - error = assert_raises( - "number change for unregistered customer should raise" - ) { - admin.action_number_change.sync - } + BandwidthTnRepo.stub :new, bandwidth_tn_repo_mock do + execute_command { |exe| + sgx = Minitest::Mock.new(OpenStruct.new( + registered?: false, + jid: Blather::JID.new(CONFIG[:sgx]) # Uses Bandwidth backend + )) + target_customer = customer(sgx: sgx) - assert_equal error.to_s, "Customer not registered" - } + exe.customer_repo.expect( + :find_by_jid, + EMPromise.resolve(target_customer), + [Matching.new do |jid| + assert jid.is_a? Blather::JID + assert_equal jid.stripped.to_s, "test@example.com" + end] + ) + + AdminAction::NumberChange::REDIS.expect( + :get, + EMPromise.resolve(nil), + ["jmp_customer_backend_sgx-test"] + ) + + AdminAction::NumberChange::REDIS.expect( + :get, + EMPromise.resolve(nil), + ["jmp_customer_backend_sgx-test"] + ) + + AdminAction::NumberChange::REDIS.expect( + :del, + EMPromise.resolve(nil), + ["jmp_customer_backend_sgx-#{target_customer.customer_id}"] + ) + + BackendSgx::IQ_MANAGER.expect( + :write, + EMPromise.resolve(nil) + ) do |iq| + assert_ibr_deregister_form(iq) + end + + BackendSgx::IQ_MANAGER.expect( + :write, + EMPromise.resolve(nil) + ) do |iq| + assert_ibr_register_form( + iq, + "+12222222222" + ) + end + + BackendSgx::REDIS.expect( + :set, + EMPromise.resolve(nil), + ["catapult_jid-+12222222222", "customer_test@component"] + ) + + expected_xadd_args = { + customer_id: "test", + old_tel: nil, + new_tel: "+12222222222", + should_delete: nil, + actor_id: "test", + class: "NumberChange", + direction: :forward + }.freeze + + AdminActionRepo::REDIS.expect( + :xadd, + EMPromise.resolve(nil) + ) do |admin_actions, star, **xadd_args| + assert_equal admin_actions, "admin_actions" + assert_equal star, "*" + + xadd_args.each do |k, v| + assert_equal v, expected_xadd_args[k] + end + end + + Command::COMMAND_MANAGER.expect( + :write, + EMPromise.resolve(change_number_form( + new_tel: "+12222222222" + )) + ) do |iq| + assert_undoable_form(iq) + assert_change_number_form(iq) + end + + result = AdminCommand.for( + target_customer, + exe.customer_repo + ).action_number_change.sync + + slug, backend_report, number_report = result.lines + + assert_equal slug.strip, "Action : Change Number" + assert_equal( + backend_report.strip, + "[move backend?]: sgx -> sgx" + ) + assert_equal( + number_report.strip, + "[change number?]: nil -> +12222222222" + ) + + assert_mock Command::COMMAND_MANAGER + assert_mock exe.customer_repo + assert_mock AdminAction::NumberChange::REDIS + assert_mock AdminActionRepo::REDIS + assert_mock BackendSgx::REDIS + assert_mock BackendSgx::IQ_MANAGER + assert_mock bandwidth_tn_repo_mock + assert_mock sgx + } + end end em :test_change_num_for_unregistered_customer + def test_change_num_for_unregistered_customer_blank_new_tel + bandwidth_tn_repo_mock = setup_bandwidth_tn_repo_mock( + should_disconnect: false + ) + + BandwidthTnRepo.stub :new, bandwidth_tn_repo_mock do + execute_command { |exe| + sgx = Minitest::Mock.new(OpenStruct.new( + registered?: false, + jid: Blather::JID.new(CONFIG[:sgx]) # Uses Bandwidth backend + )) + target_customer = customer(sgx: sgx) + + exe.customer_repo.expect( + :find_by_jid, + EMPromise.resolve(target_customer), + [Matching.new do |jid| + assert jid.is_a? Blather::JID + assert_equal jid.stripped.to_s, "test@example.com" + end] + ) + + Command::COMMAND_MANAGER.expect( + :write, + EMPromise.resolve(change_number_form( + new_tel: "" + )) + ) do |iq| + assert_undoable_form(iq) + assert_change_number_form(iq) + end + + result = assert_raises { + AdminCommand.for( + target_customer, + exe.customer_repo + ).action_number_change.sync + } + + assert_kind_of AdminAction::NumberChange::Orphan, result + + assert_mock Command::COMMAND_MANAGER + assert_mock exe.customer_repo + assert_mock bandwidth_tn_repo_mock + assert_mock sgx + } + end + end + em :test_change_num_for_unregistered_customer_blank_new_tel + def test_change_num_same_num_same_backend # Same as default given by `sgx.registered?` returned by `admin_command` tel = "+15556667777" @@ -473,12 +628,6 @@ class AdminCommandTest < Minitest::Test end] ) - AdminAction::NumberChange::REDIS.expect( - :exists, - EMPromise.resolve(1), - ["catapult_jid-#{tel}"] - ) - Command::COMMAND_MANAGER.expect( :write, EMPromise.resolve(change_number_form( @@ -526,12 +675,6 @@ class AdminCommandTest < Minitest::Test end] ) - AdminAction::NumberChange::REDIS.expect( - :exists, - EMPromise.resolve(1), - ["catapult_jid-#{tel}"] - ) - Command::COMMAND_MANAGER.expect( :write, EMPromise.resolve(change_number_form), @@ -625,12 +768,6 @@ class AdminCommandTest < Minitest::Test ["catapult_jid-#{new_tel}", "customer_test@component"] ) - AdminAction::NumberChange::REDIS.expect( - :exists, - EMPromise.resolve(1), - ["catapult_jid-#{old_tel}"] - ) - Command::COMMAND_MANAGER.expect( :write, EMPromise.resolve(change_number_form( @@ -771,12 +908,6 @@ class AdminCommandTest < Minitest::Test ["jmp_customer_backend_sgx-#{target_customer.customer_id}"] ) - AdminAction::NumberChange::REDIS.expect( - :exists, - EMPromise.resolve(1), - ["catapult_jid-+15556667777"] - ) - BackendSgx::REDIS.expect( :set, EMPromise.resolve(nil), @@ -899,12 +1030,6 @@ class AdminCommandTest < Minitest::Test ) end - AdminAction::NumberChange::REDIS.expect( - :exists, - EMPromise.resolve(1), - ["catapult_jid-#{old_tel}"] - ) - AdminAction::NumberChange::REDIS.expect( :rename, EMPromise.resolve(nil), @@ -1019,12 +1144,6 @@ class AdminCommandTest < Minitest::Test end] ) - AdminAction::NumberChange::REDIS.expect( - :exists, - EMPromise.resolve(1), - ["catapult_jid-#{old_tel}"] - ) - AdminAction::NumberChange::REDIS.expect( :rename, EMPromise.resolve(nil), @@ -1175,12 +1294,6 @@ class AdminCommandTest < Minitest::Test [Blather::JID.new("test@example.com")] ) - AdminAction::NumberChange::REDIS.expect( - :exists, - EMPromise.resolve(1), - ["catapult_jid-+15556667777"] - ) - AdminAction::NumberChange::REDIS.expect( :rename, EMPromise.resolve(nil),