Skip to content

Commit

Permalink
Extend signup timeout to 2 hours from 30 minutes (#1557)
Browse files Browse the repository at this point in the history
# What it does

We had an instance of a member taking longer than 30 minutes to complete
the signup steps, and it timed out just as they were paying for their
membership. You can see [the sample of this occurrence in
AppSignal](https://appsignal.com/chicago-tool-library/sites/60596f9214ad662e17191689/performance/incidents/59/samples/60596f9214ad662e17191689-158228531489123241821718233200?selectedTime=2024-06-12T23%3A05%3A00.000Z&activeFilter=date).
This meant that the callback from Square was rejected and their
membership payment wasn't recorded correctly in our system. The logic to
calculate if the time had passed was also hard to follow as it was
previously implemented.

This change gives people 2 hours to get through signup. The original
time was chosen arbitrarily, and in hindsight I can't say why it needs
to be so short. I'm open to making it even longer; the main risk I can
think of is on shared computers where folks might enter some of their
info, walk away, and leave the window open for someone else to complete.

The other part of a complete solution to this problem is to implement
web hooks for payments, which it captured by #1556.
  • Loading branch information
jim authored Jun 22, 2024
1 parent a92152f commit c453963
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 2 deletions.
4 changes: 3 additions & 1 deletion app/controllers/signup/base_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
module Signup
class BaseController < ApplicationController
SIGNUP_TIMEOUT = 2.hours

before_action :load_steps
before_action :set_page_title

Expand All @@ -16,7 +18,7 @@ def is_membership_enabled?
private

def load_member
if session[:member_id] && session[:timeout] && session[:timeout] > Time.current - 15.minutes
if session[:member_id] && session[:timeout] && session[:timeout] > Time.current
@member = Member.find(session[:member_id])
else
reset_session
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/signup/members_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def create

if @member_signup_form.save
session[:member_id] = @member_signup_form.member_id
session[:timeout] = Time.current + 15.minutes
session[:timeout] = Time.current + SIGNUP_TIMEOUT

redirect_to signup_agreement_url
else
Expand Down
24 changes: 24 additions & 0 deletions test/controllers/signup/members_controller_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
require "test_helper"

module Signup
class MembersControllerTest < ActionDispatch::IntegrationTest
include Devise::Test::IntegrationHelpers

setup do
create(:agreement_document)
end

test "submits the member form" do
travel_to Time.current do
assert_difference("Member.count", 1) do
post signup_members_url, params: {
member_signup_form: attributes_for(:member, password: "password", password_confirmation: "password")
}
end

assert_redirected_to signup_agreement_url
assert_equal Time.current + Signup::BaseController::SIGNUP_TIMEOUT, session[:timeout]
end
end
end
end
15 changes: 15 additions & 0 deletions test/controllers/signup/payments_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,21 @@ class PaymentsControllerTest < ActionDispatch::IntegrationTest
assert_mock mock_checkout
end

test "failed callback invocation by timing out the session" do
mock_result = Minitest::Mock.new
mock_result.expect :success?, false
mock_result.expect :error, [{code: "ERROR_CODE"}]

travel_to(session[:timeout] + 1.minute) do
get callback_signup_payments_url, params: {orderId: "abcd1234"}
end

assert_redirected_to "http://example.com/signup"

follow_redirect!
assert_select ".toast-error", text: "Your session expired. Please come into the library to complete signup."
end

test "failed callback invocation by not finding a transaction" do
mock_result = Minitest::Mock.new
mock_result.expect :success?, false
Expand Down

0 comments on commit c453963

Please sign in to comment.