Merge branch 'monthly-data-limit-ux' of https://git.secluded.site/sgx-jmp

Stephen Paul Weber created

* 'monthly-data-limit-ux' of https://git.secluded.site/sgx-jmp:
  Flatten SIM job pricing config
  Flatten main SIM pricing config
  Improve data limit UX

Change summary

bin/sim_job                     |  9 +-
config-schema.dhall             | 14 +---
config.dhall.sample             | 10 +--
forms/plan_settings.rb          |  9 +++
lib/monthly_data_limit.rb       | 40 +++++++++++++++
lib/sim_kind.rb                 | 23 +++++---
lib/sim_pricing.rb              | 43 ++++++++++++++++
sgx_jmp.rb                      | 17 ++++--
test/test_helper.rb             | 21 ++++++-
test/test_monthly_data_limit.rb | 92 +++++++++++++++++++++++++++++++++++
test/test_plan_settings_form.rb | 73 +++++++++++++++++++++++++++
11 files changed, 310 insertions(+), 41 deletions(-)

Detailed changes

bin/sim_job 🔗

@@ -32,8 +32,8 @@ SCHEMA = "{
 	component: { jid: Text },
 	keepgo: Optional { access_token: Text, api_key: Text },
 	sims: {
-		CAD: { per_gb: Natural, annual: Natural },
-		USD: { per_gb: Natural, annual: Natural }
+		annual: { CAD: Natural, USD: Natural },
+		per_gb: { CAD: Natural, USD: Natural }
 	},
 	plans: List {
 		currency: < CAD | USD >,
@@ -69,6 +69,7 @@ require_relative "../lib/expiring_lock"
 require_relative "../lib/low_balance"
 require_relative "../lib/postgres"
 require_relative "../lib/sim_repo"
+require_relative "../lib/sim_pricing"
 require_relative "../lib/transaction"
 
 BRAINTREE = AsyncBraintree.new(**CONFIG[:braintree])
@@ -107,7 +108,7 @@ module SimAction
 	end
 
 	def refill_price
-		(BigDecimal(CONFIG[:sims][customer.currency][:per_gb]) / 100) * 5
+		SIMPricing.five_gb_refill_price(customer.currency, CONFIG)
 	end
 
 	def refill_and_bill(data, price, note)
@@ -206,7 +207,7 @@ class SimAnnual
 	end
 
 	def annual_price
-		BigDecimal(CONFIG[:sims][customer.currency][:annual]) / 100
+		SIMPricing.annual_price(customer.currency, CONFIG)
 	end
 
 	def call

config-schema.dhall 🔗

@@ -81,16 +81,10 @@
       }
 , simpleswap_api_key : Text
 , sims :
-    { esim :
-        List
-          { mapKey : < CAD | USD >
-          , mapValue : { plan : Text, price : Natural }
-          }
-    , sim :
-        List
-          { mapKey : < CAD | USD >
-          , mapValue : { plan : Text, price : Natural }
-          }
+    { annual : { CAD : Natural, USD : Natural }
+    , esim : { CAD : Natural, USD : Natural }
+    , per_gb : { CAD : Natural, USD : Natural }
+    , sim : { CAD : Natural, USD : Natural }
     }
 , sip : { app : Text, realm : Text }
 , sip_host : Text

config.dhall.sample 🔗

@@ -67,12 +67,10 @@ in
 		}
 	],
 	sims = {
-		sim = [
-			{ mapKey = <CAD|USD>.CAD, mapValue = { price = 1, plan = "$1 / GB + $1 / year" } }
-		],
-		esim = [
-			{ mapKey = <CAD|USD>.CAD, mapValue = { price = 1, plan = "$1 / GB + $1 / year" } }
-		]
+		annual = { CAD = 100, USD = 100 },
+		esim = { CAD = 1, USD = 1 },
+		per_gb = { CAD = 100, USD = 100 },
+		sim = { CAD = 1, USD = 1 }
 	},
 	electrum = {
 		rpc_uri = "",

forms/plan_settings.rb 🔗

@@ -21,6 +21,11 @@ field(
 )
 
 if @sims && !@sims.empty?
+	monthly_data_limit_options =
+		(0..@monthly_data_limit_max).step(@monthly_data_limit_step).map { |value|
+			{ value: value.to_s }
+		}
+
 	field(
 		var: "monthly_data_limit",
 		type: "text-single",
@@ -28,6 +33,8 @@ if @sims && !@sims.empty?
 		label: "Dollars of data charges to allow each month",
 		description:
 			"0 means you will never be automatically charged",
-		value: @data_limit
+		value: (@data_limit || @monthly_data_limit_step).to_s,
+		range: (0..@monthly_data_limit_max),
+		options: monthly_data_limit_options
 	)
 end

lib/monthly_data_limit.rb 🔗

@@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+require_relative "sim_pricing"
+
+module MonthlyDataLimit
+	MAX_MULTIPLIER = 10
+
+	def self.for_sims(sims, currency, config)
+		sims.empty? ? {} : for_currency(currency, config)
+	end
+
+	def self.for_currency(currency, config)
+		step = SIMPricing.whole_dollars(
+			SIMPricing.five_gb_refill_price(currency, config)
+		)
+		{
+			step: step,
+			max: step * MAX_MULTIPLIER
+		}
+	end
+
+	def self.from_form(form, step:, max:)
+		limit = form.field("monthly_data_limit")&.value
+		return if limit.to_s.empty?
+
+		parse(limit, step: step, max: max)
+	end
+
+	def self.parse(limit, step:, max:)
+		limit = Integer(limit, 10)
+		unless (0..max).cover?(limit)
+			raise "Monthly data limit must be between 0 and #{max}"
+		end
+		unless (limit % step).zero?
+			raise "Monthly data limit must be a multiple of #{step}"
+		end
+
+		limit
+	end
+end

lib/sim_kind.rb 🔗

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require_relative "sim_pricing"
+
 class SIMKind
 	def initialize(variant)
 		@klass =
@@ -19,13 +21,16 @@ class SIMKind
 	end
 
 	# @param [Customer] customer
-	# @raise [ArgumentError] If matching `CONFIG[:sims]`
-	# 	key not found for `@variant` and customer
+	# @raise [ArgumentError] If matching SIM price is missing from `CONFIG[:sims]`
 	def get_processor(customer)
-		cfg = cfg(customer.currency)
-		raise ArgumentError, "Invalid config" unless cfg
-
-		@klass.for(customer, **cfg)
+		price = price_cents(customer.currency)
+		raise ArgumentError, "Invalid config" unless price
+
+		@klass.for(
+			customer,
+			price: price,
+			plan: SIMPricing.plan_label(customer.currency, CONFIG)
+		)
 	end
 
 	# @return [String] The result of either
@@ -38,15 +43,15 @@ class SIMKind
 	# @return [Float, NilClass] Returns nil if `customer.currency`
 	# 	is nil, or if @variant is malformed.
 	def price(customer)
-		cfg = cfg(customer.currency || :USD)
-		cfg[:price] / 100.to_d
+		price = price_cents(customer.currency || :USD)
+		price / 100.to_d if price
 	end
 
 protected
 
 	# @param [Symbol] currency One of the currencies from
 	# 	CONFIG[:plans] entries
-	def cfg(currency)
+	def price_cents(currency)
 		CONFIG.dig(:sims, @variant.to_sym, currency)
 	end
 end

lib/sim_pricing.rb 🔗

@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+require "bigdecimal"
+
+module SIMPricing
+	# SIM refill billing records currency amounts in transactions. Use BigDecimal
+	# because the configured per-GB price is stored in cents.
+	def self.per_gb_price(currency, config)
+		configured_price_dollars(:per_gb, currency, config)
+	end
+
+	def self.annual_price(currency, config)
+		configured_price_dollars(:annual, currency, config)
+	end
+
+	def self.configured_price_dollars(price_type, currency, config)
+		BigDecimal(
+			config.fetch(:sims).fetch(price_type).fetch(currency)
+		) / 100
+	end
+	private_class_method :configured_price_dollars
+
+	def self.five_gb_refill_price(currency, config)
+		per_gb_price(currency, config) * 5
+	end
+
+	def self.per_gb_label(currency, config)
+		"$%.2f / GB" % per_gb_price(currency, config)
+	end
+
+	def self.plan_label(currency, config)
+		"$%.2f / GB + $%.2f / year" % [
+			per_gb_price(currency, config),
+			annual_price(currency, config)
+		]
+	end
+
+	# CustomerRepo#put_monthly_limits stores limits with to_i, so render monthly
+	# data limit form steps in that same integer-dollar unit.
+	def self.whole_dollars(price)
+		price.to_i
+	end
+end

sgx_jmp.rb 🔗

@@ -93,6 +93,7 @@ require_relative "lib/expiring_lock"
 require_relative "lib/em"
 require_relative "lib/form_to_h"
 require_relative "lib/low_balance"
+require_relative "lib/monthly_data_limit"
 require_relative "lib/port_in_order"
 require_relative "lib/patches_for_sentry"
 require_relative "lib/payment_methods"
@@ -712,15 +713,21 @@ Command.new(
 			SIMRepo.new.owned_by(customer)
 		]).then { |(limit, sims)| [customer, sims, limit] }
 	}.then do |(customer, sims, limit)|
+		limits = MonthlyDataLimit.for_sims(sims, customer.currency, CONFIG)
 		Command.reply { |reply|
 			reply.allowed_actions = [:next]
 			reply.command << FormTemplate.render(
-				"plan_settings", customer: customer, sims: sims, data_limit: limit
+				"plan_settings",
+				customer: customer, sims: sims, data_limit: limit,
+				monthly_data_limit_step: limits[:step],
+				monthly_data_limit_max: limits[:max]
 			)
 		}.then { |iq|
 			kwargs = {
 				monthly_overage_limit: iq.form.field("monthly_overage_limit")&.value,
-				monthly_data_limit: iq.form.field("monthly_data_limit")&.value
+				monthly_data_limit: (
+					MonthlyDataLimit.from_form(iq.form, **limits) unless limits.empty?
+				)
 			}.compact
 			Command.execution.customer_repo.put_monthly_limits(customer, **kwargs)
 		}.then { Command.finish("Configuration saved!") }
@@ -779,11 +786,9 @@ Command.new(
 		}.then { |iq|
 			case iq.form.field("http://jabber.org/protocol/commands#actions")&.value
 			when "order-sim"
-				SIMOrder.for(customer, **CONFIG.dig(:sims, :sim, customer.currency))
+				SIMKind.new("sim").get_processor(customer)
 			when "order-esim"
-				SIMOrder::ESIM.for(
-					customer, **CONFIG.dig(:sims, :esim, customer.currency)
-				)
+				SIMKind.new("esim").get_processor(customer)
 			when "edit-nicknames"
 				EditSimNicknames.new(customer, sims)
 			else

test/test_helper.rb 🔗

@@ -132,7 +132,10 @@ CONFIG = {
 		{
 			name: "test_cad",
 			currency: :CAD,
-			monthly_price: 10000
+			monthly_price: 10000,
+			messages: :unlimited,
+			minutes: { included: 10440, price: 87 },
+			allow_register: true
 		},
 		{
 			name: "test_usd_old_billing",
@@ -158,13 +161,21 @@ CONFIG = {
 	electrum_notify_url: ->(*) { "http://notify.example.com" },
 	admin_notify: "admin_room@example.com",
 	sims: {
+		annual: {
+			USD: 100,
+			CAD: 120
+		},
+		per_gb: {
+			USD: 100,
+			CAD: 120
+		},
 		sim: {
-			USD: { price: 500, plan: "1GB" },
-			CAD: { price: 600, plan: "1GB" }
+			USD: 500,
+			CAD: 600
 		},
 		esim: {
-			USD: { price: 300, plan: "500MB" },
-			CAD: { price: 400, plan: "500MB" }
+			USD: 300,
+			CAD: 400
 		}
 	},
 	keep_area_codes: [

test/test_monthly_data_limit.rb 🔗

@@ -0,0 +1,92 @@
+# frozen_string_literal: true
+
+require "test_helper"
+require "monthly_data_limit"
+
+class MonthlyDataLimitTest < Minitest::Test
+	def parse(limit)
+		MonthlyDataLimit.parse(limit, step: 5, max: 50)
+	end
+
+	def test_accepts_multiple_of_five
+		assert_equal 10, parse("10")
+	end
+
+	def test_accepts_zero
+		assert_equal 0, parse("0")
+	end
+
+	def test_accepts_maximum
+		assert_equal 50, parse("50")
+	end
+
+	def test_ignores_blank_form_field
+		form = Struct.new(:value) do
+			def field(*)
+				self
+			end
+		end.new("")
+
+		assert_nil MonthlyDataLimit.from_form(form, step: 5, max: 50)
+	end
+
+	def test_rejects_non_multiple_of_five
+		error = assert_raises(RuntimeError) { parse("11") }
+
+		assert_equal "Monthly data limit must be a multiple of 5", error.message
+	end
+
+	def test_rejects_non_multiple_of_currency_step
+		error = assert_raises(RuntimeError) {
+			MonthlyDataLimit.parse("10", step: 6, max: 60)
+		}
+
+		assert_equal "Monthly data limit must be a multiple of 6", error.message
+	end
+
+	def test_rejects_negative_values
+		error = assert_raises(RuntimeError) { parse("-5") }
+
+		assert_equal "Monthly data limit must be between 0 and 50", error.message
+	end
+
+	def test_rejects_values_above_maximum
+		error = assert_raises(RuntimeError) { parse("55") }
+
+		assert_equal "Monthly data limit must be between 0 and 50", error.message
+	end
+
+	def test_uses_sim_refill_price_for_step
+		limits = MonthlyDataLimit.for_currency(:CAD, CONFIG)
+
+		assert_equal 6, limits[:step]
+		assert_equal 60, limits[:max]
+	end
+
+	def test_formats_refill_price_for_display
+		assert_equal "$1.20 / GB", SIMPricing.per_gb_label(:CAD, CONFIG)
+	end
+
+	def test_formats_plan_price_for_display
+		assert_equal(
+			"$1.20 / GB + $1.20 / year",
+			SIMPricing.plan_label(:CAD, CONFIG)
+		)
+	end
+
+	def test_does_not_need_sim_pricing_without_sims
+		assert_equal({}, MonthlyDataLimit.for_sims([], :CAD, CONFIG))
+	end
+
+	def test_uses_integer_dollars_for_fractional_refill_price
+		config = CONFIG.merge(
+			sims: CONFIG[:sims].merge(
+				per_gb: CONFIG[:sims][:per_gb].merge(CAD: 125)
+			)
+		)
+		limits = MonthlyDataLimit.for_currency(:CAD, config)
+
+		assert_equal 6, limits[:step]
+		assert_equal 60, limits[:max]
+	end
+end

test/test_plan_settings_form.rb 🔗

@@ -0,0 +1,73 @@
+# frozen_string_literal: true
+
+require "test_helper"
+require "customer"
+require "form_template"
+require "monthly_data_limit"
+
+class PlanSettingsFormTest < Minitest::Test
+	def render(data_limit: nil, plan_name: "test_usd")
+		customer = customer(plan_name: plan_name)
+		limits = MonthlyDataLimit.for_currency(customer.currency, CONFIG)
+		FormTemplate.render(
+			"plan_settings",
+			customer: customer,
+			sims: [OpenStruct.new],
+			data_limit: data_limit,
+			monthly_data_limit_step: limits[:step],
+			monthly_data_limit_max: limits[:max]
+		)
+	end
+
+	def test_monthly_data_limit_is_integer_field
+		field = render.field("monthly_data_limit")
+		validate = field.find(
+			"ns:validate",
+			ns: "http://jabber.org/protocol/xdata-validate"
+		).first
+
+		assert_equal "text-single", field.type
+		assert_equal "xs:integer", validate[:datatype]
+	end
+
+	def test_monthly_data_limit_has_slider_range
+		validate = render.field("monthly_data_limit").find(
+			"ns:validate",
+			ns: "http://jabber.org/protocol/xdata-validate"
+		).first
+		range = validate.children.first
+
+		assert_equal "range", range.name
+		assert_equal "0", range[:min]
+		assert_equal "50", range[:max]
+	end
+
+	def test_monthly_data_limit_options_step_by_five_gb_cost
+		options = render.field("monthly_data_limit").options.map(&:value)
+
+		assert_equal (0..50).step(5).map(&:to_s), options
+	end
+
+	def test_monthly_data_limit_defaults_to_one_five_gb_step
+		assert_equal "5", render.field("monthly_data_limit").value
+	end
+
+	def test_monthly_data_limit_uses_customer_currency
+		field = render(plan_name: "test_cad").field("monthly_data_limit")
+		validate = field.find(
+			"ns:validate",
+			ns: "http://jabber.org/protocol/xdata-validate"
+		).first
+		range = validate.children.first
+
+		assert_equal "60", range[:max]
+		assert_equal "6", field.value
+		assert_equal (0..60).step(6).map(&:to_s), field.options.map(&:value)
+	end
+
+	def test_monthly_data_limit_keeps_existing_value
+		field = render(data_limit: "10").field("monthly_data_limit")
+
+		assert_equal "10", field.value
+	end
+end