@@ -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
@@ -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),