fix/regression: support 'migrate billing'

Phillip Davis created

- `MigrateBilling` passes a string for a tel, previously causing
  NoMethodError when `Payment.for` says `product.price`
- Introduces a new MigrateBilling class in lib so that unit tests
  capture any changes that break this in the future
- Edit .rubocop.yml to exclude `when_ready` from
  `Lint/ConstantDefinitionInBlock`, which was raised just now because
  this commit touched sgx_jmp.rb . I think this is the best solution
  because those constants (`REDIS`, `DB`, etc.) are used as globals
  throughout the codebase. Since many of them require a running EM
  reactor, changing this would require extensive refactoring.

Change summary

.rubocop.yml                 |   4 +
lib/migrate_billing.rb       |  42 ++++++++++++++
sgx_jmp.rb                   |  26 --------
test/test_helper.rb          |   8 ++
test/test_migrate_billing.rb | 113 ++++++++++++++++++++++++++++++++++++++
5 files changed, 169 insertions(+), 24 deletions(-)

Detailed changes

.rubocop.yml 🔗

@@ -122,6 +122,10 @@ Lint/OutOfRangeRegexpRef:
 Lint/MissingSuper:
   Enabled: false
 
+Lint/ConstantDefinitionInBlock:
+  AllowedMethods:
+    - when_ready
+
 Style/BlockDelimiters:
   EnforcedStyle: semantic
   AllowBracesOnProceduralOneLiners: true

lib/migrate_billing.rb 🔗

@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+require_relative "./paypal_done"
+
+class MigrateBilling
+	def initialize(customer, finish: PaypalDone)
+		@customer = customer
+		@finish = finish
+	end
+
+	def parse(iq)
+		plan_name = iq.form.field("plan_name").value.to_s
+		(@customer = @customer.with_plan(plan_name)).save_plan!.then {
+			Registration::Payment.for(
+				iq, @customer,
+				OpenStruct.new(price: 0, to_s: @customer.registered?.phone),
+				finish: @finish
+			)
+		}.then(&:write)
+	end
+
+	def msg
+		"#{@customer.customer_id} migrated to #{@customer.currency}"
+	end
+
+	def write
+		Command.reply(&method(:form)).then(&method(:parse)).catch_only(
+			Command::Execution::FinalStanza
+		) do |s|
+			BLATHER.join(CONFIG[:notify_admin], "sgx-jmp")
+			BLATHER.say(CONFIG[:notify_admin], msg, :groupchat)
+			EMPromise.reject(s)
+		end
+	end
+
+protected
+
+	def form(stanza)
+		stanza.allowed_actions = [:next]
+		stanza.command << FormTemplate.render("migrate_billing")
+	end
+end

sgx_jmp.rb 🔗

@@ -96,6 +96,7 @@ require_relative "lib/port_in_order"
 require_relative "lib/patches_for_sentry"
 require_relative "lib/payment_methods"
 require_relative "lib/paypal_done"
+require_relative "lib/migrate_billing"
 require_relative "lib/postgres"
 require_relative "lib/reachability_form"
 require_relative "lib/reachability_repo"
@@ -616,30 +617,7 @@ Command.new(
 	list_for: ->(tel:, customer:, **) { tel && !customer&.currency },
 	customer_repo: CustomerRepo.new(sgx_repo: Bwmsgsv2Repo.new)
 ) {
-	EMPromise.all([
-		Command.customer,
-		Command.reply do |reply|
-			reply.allowed_actions = [:next]
-			reply.command << FormTemplate.render("migrate_billing")
-		end
-	]).then do |(customer, iq)|
-		plan_name = iq.form.field("plan_name").value.to_s
-		customer = customer.with_plan(plan_name)
-		customer.save_plan!.then {
-			Registration::Payment.for(
-				iq, customer, customer.registered?.phone,
-				finish: PaypalDone
-			)
-		}.then(&:write).catch_only(Command::Execution::FinalStanza) do |s|
-			BLATHER.join(CONFIG[:notify_admin], "sgx-jmp")
-			BLATHER.say(
-				CONFIG[:notify_admin],
-				"#{customer.customer_id} migrated to #{customer.currency}",
-				:groupchat
-			)
-			EMPromise.reject(s)
-		end
-	end
+	Command.customer.then(&MigrateBilling.method(:new)).then(&:write)
 }.register(self).then(&CommandList.method(:register))
 
 Command.new(

test/test_helper.rb 🔗

@@ -131,6 +131,13 @@ CONFIG = {
 			name: "test_cad",
 			currency: :CAD,
 			monthly_price: 10000
+		},
+		{
+			name: "test_usd_old_billing",
+			currency: nil,
+			messages: :unlimited,
+			minutes: { included: 10440, price: 87 },
+			allow_register: false
 		}
 	],
 	braintree: {
@@ -147,6 +154,7 @@ CONFIG = {
 	],
 	credit_card_url: ->(*) { "http://creditcard.example.com?" },
 	electrum_notify_url: ->(*) { "http://notify.example.com" },
+	admin_notify: "admin_room@example.com",
 	sims: {
 		sim: {
 			USD: { price: 500, plan: "1GB" },

test/test_migrate_billing.rb 🔗

@@ -0,0 +1,113 @@
+# frozen_string_literal: true
+
+require "minitest"
+
+require "test_helper"
+
+require "paypal_done"
+require "migrate_billing"
+
+Command::COMMAND_MANAGER = Minitest::Mock.new
+CustomerPlan::DB = Minitest::Mock.new
+MigrateBilling::BLATHER = Minitest::Mock.new
+
+class TestMigrateBilling < Minitest::Test
+	def setup
+		@sgx = Minitest::Mock.new
+	end
+
+	def test_write_registered
+		execute_command do |exe|
+			Command::COMMAND_MANAGER.expect(
+				:write,
+				EMPromise.resolve(Blather::Stanza::Iq::Command.new.tap { |iq|
+					iq.form.fields = [
+						{ var: "activation_method", value: "credit_card" },
+						{ var: "plan_name", value: "test_usd" }
+					]
+					iq.from = "test@example.com"
+				}),
+				[Matching.new { |iq|
+					assert_equal :form, iq.form.type
+					assert iq.form.instructions.include?("legacy PayPal")
+				}]
+			)
+
+			CustomerPlan::DB.expect(
+				:exec_defer,
+				nil,
+				[
+					String,
+					Matching.new do |(customer_id, plan_name, parent_customer_id)|
+						assert_equal "test", customer_id
+						assert_equal "test_usd", plan_name
+						assert_nil parent_customer_id
+					end
+				]
+			)
+
+			Command::COMMAND_MANAGER.expect(
+				:write,
+				EMPromise.resolve(Blather::Stanza::Iq::Command.new.tap { |iq|
+					iq.action = :next
+				}),
+				[Matching.new { |iq|
+					assert_equal :executing, iq.status
+					assert_equal(
+						"http://creditcard.example.com?&amount=1",
+						OOB.find_or_create(iq.command).url
+					)
+				}]
+			)
+
+			@sgx.expect(
+				:registered?,
+				OpenStruct.new(phone: "+15550000")
+			)
+
+			exe.customer_repo.expect(
+				:find,
+				EMPromise.resolve(
+					customer(
+						plan_name: "test_usd",
+						sgx: @sgx
+					).with(balance: 100)
+				),
+				["test"]
+			)
+			# Apparently the constant assignment from
+			# `RegistrationTest::InviteCodeTest` pollutes this
+			# test
+			Registration::Payment::MaybeBill::BillPlan.expect(
+				:new,
+				PaypalDone.new
+			) { |*| true }
+
+			MigrateBilling::BLATHER.expect(
+				:join,
+				nil,
+				[CONFIG[:notify_admin], "sgx-jmp"]
+			)
+			MigrateBilling::BLATHER.expect(
+				:say,
+				nil,
+				[
+					CONFIG[:notify_admin],
+					"test migrated to USD",
+					:groupchat
+				]
+			)
+
+			cust = customer(plan_name: "test_usd_old_billing", sgx: @sgx)
+			result = MigrateBilling.new(cust).write.catch { |e|
+				e
+			}.sync
+
+			assert_equal(result.stanza.note.text, PaypalDone::MESSAGE)
+			assert_mock Registration::Payment::MaybeBill::BillPlan
+			assert_mock Command::COMMAND_MANAGER
+			assert_mock @sgx
+		end
+	end
+	em :test_write_registered
+end