Create pending memberships when a member chooses "in person" on the payment screen

app/controllers/admin/members/memberships_controller.rb

@@ -19,18 +19,6 @@ def create
         end
       end
 
-      # This should probably be a MembershipActivation controller since it doesn't rely on params.
-      def update
-        @membership = @member.pending_membership
-
-        if @membership
-          @membership.start!
-          redirect_to admin_member_memberships_path(@member), success: "Membership started.", status: :see_other
-        else
-          redirect_to admin_member_path(@member), error: "Could not start membership", status: :see_other
-        end
-      end
-
       private
 
       def membership_form_params

app/controllers/renewal/payments_controller.rb

@@ -30,7 +30,10 @@ def create
     end
 
     def skip
-      # completing in person
+      # completing in person: create a pending membership so the renewal banner
+      # clears right away. Staff record the payment and start it when the member
+      # comes in to pay.
+      Membership.create_for_member(@member, start_membership: false) unless @member.pending_membership
       MemberMailer.with(member: @member).renewal_message.deliver_later
       redirect_to renewal_confirmation_url, status: :see_other
     end

app/controllers/signup/payments_controller.rb

@@ -28,7 +28,9 @@ def create
     end
 
     def skip
-      # completing in person
+      # completing in person: create a pending membership so staff have a record
+      # to start when the member comes in to pay.
+      Membership.create_for_member(@member, start_membership: false) unless @member.pending_membership
       MemberMailer.with(member: @member).welcome_message.deliver_later
       reset_session
       redirect_to signup_confirmation_url, status: :see_other

app/forms/membership_form.rb

@@ -21,6 +21,11 @@ def errors
   end
 
   def save
+    if with_payment && !start_membership
+      @payment.errors.add(:base, "Can't accept a payment without starting the membership. Choose \"Create without payment\" to leave it pending.")
+      return false
+    end
+
     if with_payment
       save_with_payment
     else

app/models/membership.rb

@@ -65,7 +65,7 @@ def status_text
   end
 
   def start!(now = Time.current)
-    update!(started_at: now, ended_at: now + 364.days)
+    update!(started_at: now, ended_at: now + 1.year)
   end
 
   def self.next_start_date_for_member(member, now: Time.current)
@@ -85,17 +85,34 @@ def self.create_for_member(member, amount: 0, source: nil, start_membership: fal
     membership_type = member.memberships.present? ? "renewal" : "initial"
 
     if start_membership
-      start_date = next_start_date_for_member(member, now: now)
-      raise PendingMembership.new("member with pending membership can't start a new membership") unless start_date
-
-      membership = member.memberships.create!(started_at: start_date, ended_at: start_date + 365.days, library: member.library, membership_type:)
+      if (membership = member.pending_membership)
+        # Finalize a membership that was left pending, e.g. a renewal the member
+        # chose to complete in person. If the member renewed early and still has
+        # an active membership, start this one when that one ends so the dates
+        # don't overlap; otherwise start it now.
+        start_date = member.active_membership&.ended_at || now
+        membership.start!(start_date)
+      else
+        start_date = next_start_date_for_member(member, now: now)
+        raise PendingMembership.new("member with pending membership can't start a new membership") unless start_date
+
+        membership = member.memberships.create!(started_at: start_date, ended_at: start_date + 1.year, library: member.library, membership_type:)
+      end
     else
       membership = member.memberships.create!(library: member.library, membership_type:)
     end
 
+    # Record the payment at most once per membership. A membership can reach here
+    # already paid — e.g. an online signup creates a pending, paid membership that
+    # staff later start in person — and must not be charged a second time.
     if amount > 0
-      Adjustment.record_membership(membership, amount)
-      Adjustment.record_member_payment(member, amount, source, square_transaction_id)
+      if !Adjustment.exists?(adjustable: membership)
+        Adjustment.record_membership(membership, amount)
+        Adjustment.record_member_payment(member, amount, source, square_transaction_id)
+      else
+        # UI should not allow this to happen
+        raise "Can not record payment for an already paid membership"
+      end
     end
     membership
   end

app/views/account/_membership_renewal_message.html.erb

@@ -4,3 +4,8 @@
     <%= link_to "Renew Membership", renewal_path, class: "btn btn-lg btn-default" %>
   </div>
 <% end %>
+<% if current_member.last_membership && current_member.last_membership.pending? && current_member.last_membership.membership_type == "renewal" %>
+  <div class="toast toast-success">
+    <p>You have started the renewal process for your membership. <strong>Come in to the library to complete it.</strong></p>
+  </div>
+<% end %>

app/views/account/members/show.html.erb

@@ -46,7 +46,7 @@
         <dd class="title_bold"><%= @member.number %></dd>
 
         <dt>Membership Expiration Date</dt>
-        <dd class="title_bold"><%= date_with_time_title(@member.last_membership.ended_at) %></dd>
+        <dd class="title_bold"><%= date_with_time_title(@member.active_membership.ended_at) %></dd>
       <% else %>
         <dd class="label"><%= @member.status %></dd>
       <% end %>

app/views/admin/members/_membership_status.html.erb

@@ -2,7 +2,7 @@
   <% if member.pending_membership %>
     <div class="toast member-membership clearfix">
       <p class="float-left"><i class="icon icon-stop"></i> Member has a pending membership.</p>
-      <p class="float-right"><%= button_to "Start Membership", admin_member_membership_path(member, member.pending_membership), method: :patch %></p>
+      <p class="float-right"><%= link_to "Complete Membership", new_admin_member_membership_path(member) %></p>
     </div>
   <% elsif member.memberships.count > 0 # expired memberships %>
     <div class="toast member-membership clearfix">

app/views/admin/members/memberships/index.html.erb

@@ -1,5 +1,7 @@
 <%= render "admin/members/profile" do %>
-  <% unless @member.pending_membership %>
+  <% if @member.pending_membership %>
+    <%= link_to "Complete Membership", new_admin_member_membership_path, class: "btn" %>
+  <% else %>
     <%= link_to @memberships.any? ? "Renew Membership" : "Create Membership", new_admin_member_membership_path, class: "btn" %>
   <% end %>
 

app/views/admin/members/memberships/new.html.erb

@@ -1,39 +1,51 @@
 <%= render "admin/members/profile" do %>
   <h3>
-    <% if @member.active_membership.present? %>
+    <% if @member.pending_membership %>
+      Complete Membership
+    <% elsif @member.active_membership.present? %>
       Renew Membership
     <% else %>
       New Membership
     <% end %>
   </h3>
 
-  <%= form_with model: @form, url: admin_member_memberships_path(@member), builder: SpectreFormBuilder do |form| %>
+  <% pending = @member.pending_membership %>
+  <%= form_with model: @form, url: admin_member_memberships_path(@member), builder: SpectreFormBuilder, class: "membership-form" do |form| %>
     <div class="columns">
       <div class="column col-12">
-        <fieldset>
-          <legend>
-            <%= form.radio_button :with_payment, true, label: "Accept payment today", required: false %>
-          </legend>
-          <div class="fieldset-radio-inputs">
-            <%= form.money_field :amount_dollars, label: "This year's membership fee", class: "input-lg" %>
-            <%= form.select :payment_source, options_for_select(membership_payment_source_options, @form.payment_source || "cash"), prompt: true %>
-          </div>
-        </fieldset>
+        <%= form.errors(true) %>
 
-        <fieldset>
-          <legend>
-            <%= form.radio_button :with_payment, false, label: "Create without payment", required: false %>
-          </legend>
-          <div class="fieldset-radio-inputs">
-            <p>Remember that we don't turn anyone away because they can't pay. Use this option to create a membership without accepting a payment.</p>
-          </div>
-        </fieldset>
+        <% if pending&.adjustment %>
+          <p>This membership was already paid (<%= pending.amount.format %>). Completing it will start the membership today.</p>
+          <%= form.hidden_field :with_payment, value: false %>
+          <%= form.submit "Complete and Start Membership" %>
+        <% else %>
+          <fieldset>
+            <legend>
+              <%= form.radio_button :with_payment, true, label: "Accept payment today", required: false %>
+            </legend>
+            <div class="fieldset-radio-inputs">
+              <%= form.money_field :amount_dollars, label: "This year's membership fee", class: "input-lg" %>
+              <%= form.select :payment_source, options_for_select(membership_payment_source_options, @form.payment_source || "cash"), prompt: true %>
+            </div>
+          </fieldset>
 
-        <% unless @member.active_membership.present? %>
-          <%= form.check_box :start_membership, label: "Start this membership", hint: "If unchecked, this membership will be pending and can be started at a later time.", checked: true %>
-        <% end %>
+          <fieldset>
+            <legend>
+              <%= form.radio_button :with_payment, false, label: "Create without payment", required: false %>
+            </legend>
+            <div class="fieldset-radio-inputs">
+              <p>Remember that we don't turn anyone away because they can't pay. Use this option to create a membership without accepting a payment.</p>
+            </div>
+          </fieldset>
 
-        <%= form.submit "Save Membership" %>
+          <% if pending.nil? && @member.active_membership.blank? %>
+            <%= form.check_box :start_membership, label: "Start this membership", hint: "If unchecked, this membership will be pending and can be started at a later time.", checked: true %>
+            <%= form.submit "Create Membership" %>
+          <% else %>
+            <%= form.submit "Update and Start Membership" %>
+          <% end %>
+        <% end %>
       </div>
     </div>
   <% end %>

config/routes.rb

@@ -145,7 +145,7 @@
         end
         resource :hold_loan, only: :create
         resource :lookup, only: :show
-        resources :memberships, only: [:index, :new, :create, :update]
+        resources :memberships, only: [:index, :new, :create]
         resources :payments, only: [:new, :create]
         resource :verification, only: [:edit, :update]
         resources :appointments, only: [:index, :create]

db/seeds.rb

@@ -24,6 +24,18 @@
       email: unconfirmed_email_user.email, full_name: "Unconfirmed Email", preferred_name: "Unconfirmed Email", user: unconfirmed_email_user
     ))
 
+    pending_member_user = User.create!(email: "pending_member#{email_suffix}@example.com", password: "password", unconfirmed_email: "pending_member#{email_suffix}@example.com")
+    Member.create!(member_attrs.merge(
+      email: pending_member_user.email, full_name: "Pending Member", preferred_name: "Pending", user: pending_member_user
+    ))
+    Membership.create_for_member(pending_member_user.member)
+
+    pending_member_who_paid_user = User.create!(email: "pending_member_who_paid#{email_suffix}@example.com", password: "password", **confirmed_email_attrs)
+    Member.create!(member_attrs.merge(
+      email: pending_member_who_paid_user.email, full_name: "Pending Member Who Paid", preferred_name: "Pending Paid", user: pending_member_who_paid_user
+    ))
+    Membership.create_for_member(pending_member_who_paid_user.member, amount: Money.new(1000), source: "square", square_transaction_id: "fake-transaction-id")
+
     verified_user = User.create!(email: "verified_member#{email_suffix}@example.com", password: "password", **confirmed_email_attrs)
     verified_member = Member.create!(member_attrs.merge(
       email: verified_user.email, full_name: "Firstname Lastname", preferred_name: "Verified", status: 1, address_verified: true, user: verified_user

test/controllers/admin/memberships_controller_test.rb

@@ -49,8 +49,8 @@ class MembershipsControllerTest < ActionDispatch::IntegrationTest
     end
 
     %w[square cash].each do |payment_source|
-      test "creates new pending membership using #{payment_source}" do
-        assert_difference "Adjustment.count", 2 do
+      test "won't create a pending membership while accepting a #{payment_source} payment" do
+        assert_no_difference ["Adjustment.count", "Membership.count"] do
           post admin_member_memberships_url(@member), params: {
             membership_form: {
               amount_dollars: 12,
@@ -60,10 +60,7 @@ class MembershipsControllerTest < ActionDispatch::IntegrationTest
             }
           }
         end
-        assert_response :redirect
-
-        membership = @member.memberships.last
-        assert membership.pending?
+        assert_response :unprocessable_content
       end
     end
 

test/controllers/renewal/payments_controller_test.rb

@@ -70,6 +70,25 @@ class PaymentsControllerTest < ActionDispatch::IntegrationTest
       assert_mock mock_checkout
     end
 
+    test "skip creates a pending membership so the renewal banner clears" do
+      assert_difference "Membership.count" => 1 do
+        post skip_renewal_payments_url
+      end
+
+      assert_redirected_to renewal_confirmation_url
+      assert @member.reload.pending_membership
+    end
+
+    test "skip does not create a second pending membership" do
+      create(:pending_membership, member: @member)
+
+      assert_no_difference "Membership.count" do
+        post skip_renewal_payments_url
+      end
+
+      assert_redirected_to renewal_confirmation_url
+    end
+
     test "successful callback invocation" do
       mock_result = Minitest::Mock.new
       mock_result.expect :success?, true

test/controllers/signup/payments_controller_test.rb

@@ -64,6 +64,25 @@ class PaymentsControllerTest < ActionDispatch::IntegrationTest
       assert_mock mock_checkout
     end
 
+    test "skip creates a pending membership so staff can complete it in person" do
+      assert_difference "Membership.count" => 1 do
+        post skip_signup_payments_url
+      end
+      assert_redirected_to signup_confirmation_url
+
+      membership = @member.reload.pending_membership
+      assert membership
+      assert_equal "initial", membership.membership_type
+    end
+
+    test "skip does not create a second pending membership" do
+      create(:pending_membership, member: @member)
+      assert_no_difference "Membership.count" do
+        post skip_signup_payments_url
+      end
+      assert_redirected_to signup_confirmation_url
+    end
+
     test "successful callback invocation" do
       mock_result = Minitest::Mock.new
       mock_result.expect :success?, true

test/models/membership_test.rb

@@ -32,6 +32,68 @@ class MembershipTest < ActiveSupport::TestCase
     assert_equal "renewal", membership.membership_type
   end
 
+  test "starts an existing pending membership instead of creating a new one" do
+    member = create(:member)
+    pending = create(:pending_membership, member: member)
+    now = Time.current.beginning_of_day
+
+    membership = assert_no_difference("Membership.count") {
+      Membership.create_for_member(member, now: now, start_membership: true)
+    }
+
+    assert_equal pending, membership
+    assert_equal now, membership.started_at
+    assert_equal now + 1.year, membership.ended_at
+  end
+
+  test "completing a pending membership for a still-active member starts it when the active one ends" do
+    member = create(:member)
+    now = Time.current.beginning_of_day
+    active = create(:membership, member: member, started_at: now - 345.days, ended_at: now + 20.days)
+    create(:pending_membership, member: member)
+
+    membership = assert_no_difference("Membership.count") {
+      Membership.create_for_member(member, now: now, start_membership: true)
+    }
+
+    refute membership.pending?
+    assert_equal active.ended_at, membership.started_at
+    assert_equal active.ended_at + 1.year, membership.ended_at
+  end
+
+  test "records payment when starting an existing pending membership" do
+    member = create(:member)
+    create(:pending_membership, member: member)
+    now = Time.current.beginning_of_day
+    amount = Money.new(2500)
+
+    membership = assert_no_difference("Membership.count") {
+      assert_difference("Adjustment.count", 2) {
+        Membership.create_for_member(member, now: now, amount: amount, source: "cash", start_membership: true)
+      }
+    }
+
+    assert_equal now, membership.started_at
+    assert_equal amount * -1, membership.adjustment.amount
+  end
+
+  test "does not record a second payment when completing an already-paid pending membership" do
+    member = create(:member)
+    now = Time.current.beginning_of_day
+    amount = Money.new(2500)
+
+    # An online signup creates a pending membership that is already paid.
+    Membership.create_for_member(member, now: now, amount: amount, source: "square")
+    member.reload.pending_membership
+
+    # Staff later complete it in person; it must not be charged again.
+    assert_no_difference(["Membership.count", "Adjustment.count"]) {
+      assert_raises("Can not record payment for an already paid membership") {
+        Membership.create_for_member(member, now: now, amount: amount, source: "cash", start_membership: true)
+      }
+    }
+  end
+
   test "creates a pending membership for a member" do
     member = create(:member)
     now = Time.current.beginning_of_day