Improve data limit UX

Amolith created

Show the default data limit instead of a blank value and include
range/options metadata so clients can render a stepped slider.

Check submitted values against the price of a 5GB top-up in the
customer's currency and cap them at a fixed multiplier.

Change summary

config-schema.dhall             |  4 +
config.dhall.sample             |  2 
forms/plan_settings.rb          |  9 +++
lib/monthly_data_limit.rb       | 40 +++++++++++++++++
lib/sim_pricing.rb              | 17 +++++++
sgx_jmp.rb                      | 11 +++
test/test_helper.rb             |  7 ++
test/test_monthly_data_limit.rb | 79 +++++++++++++++++++++++++++++++++++
test/test_plan_settings_form.rb | 73 ++++++++++++++++++++++++++++++++
9 files changed, 237 insertions(+), 5 deletions(-)

Detailed changes

config-schema.dhall 🔗

@@ -81,7 +81,9 @@
       }
 , simpleswap_api_key : Text
 , sims :
-    { esim :
+    { CAD : { per_gb : Natural }
+    , USD : { per_gb : Natural }
+    , esim :
         List
           { mapKey : < CAD | USD >
           , mapValue : { plan : Text, price : Natural }

config.dhall.sample 🔗

@@ -67,6 +67,8 @@ in
 		}
 	],
 	sims = {
+		CAD = { per_gb = 100 },
+		USD = { per_gb = 100 },
 		sim = [
 			{ mapKey = <CAD|USD>.CAD, mapValue = { price = 1, plan = "$1 / GB + $1 / year" } }
 		],

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_pricing.rb 🔗

@@ -0,0 +1,17 @@
+# 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.five_gb_refill_price(currency, config)
+		(BigDecimal(config[:sims][currency][:per_gb]) / 100) * 5
+	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"
@@ -709,15 +710,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!") }

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,6 +161,8 @@ CONFIG = {
 	electrum_notify_url: ->(*) { "http://notify.example.com" },
 	admin_notify: "admin_room@example.com",
 	sims: {
+		USD: { per_gb: 100 },
+		CAD: { per_gb: 120 },
 		sim: {
 			USD: { price: 500, plan: "1GB" },
 			CAD: { price: 600, plan: "1GB" }

test/test_monthly_data_limit.rb 🔗

@@ -0,0 +1,79 @@
+# 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_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(CAD: { per_gb: 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