Conversation
jim
changed the title
phinze
left a comment
phinze
left a comment
There was a problem hiding this comment.
Looks good overall! A few inlines, only important one is that 500 one.
| # 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 |
There was a problem hiding this comment.
I think this creates a 500 on the member's own profile page for the early-renewal case. After this runs, a member can have an active membership and a pending one, and since last_membership orders ended_at DESC NULLS FIRST, the pending row becomes last_membership. app/views/account/members/show.html.erb:49 does date_with_time_title(@member.last_membership.ended_at) inside if @member.active_membership, so ended_at is nil and nil.to_fs(:short_date) raises. The admin equivalent in _member_details.html.erb is fine because display_date nil-guards.
The fix is likely just switching that view to @member.active_membership.ended_at, since it's already inside the if @member.active_membership branch. CI doesn't catch it because the new system test visits account_home_url (loans), not account_member_url.
| assert @member.pending_membership, "expected a pending membership after completing in person" | ||
|
|
||
| # The renewal banner should no longer appear now that a (pending) renewal exists. | ||
| visit account_home_url |
There was a problem hiding this comment.
Worth a conscious call: the same mechanism that clears the banner here also suppresses it indefinitely. If the member picks "in person" and never comes in, their active membership lapses, borrow? goes false, and the account page shows no renewal prompt because the pending row is still last_membership. That's probably the right tradeoff (the banner would just loop them into a flow that no-ops now), but it means staff are the only ones who'll notice a stale pending renewal. If that bothers you, a small follow-up could show a different message when last_membership.pending? ("your renewal is pending until you pay in person") so the member still gets a nudge without being sent back through the renewal flow.
There was a problem hiding this comment.
Added a message for the member to remind them to come in to complete the process.
| # 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.exists?(adjustable: membership) |
There was a problem hiding this comment.
Small thing: when this guard trips with amount > 0, the payment is dropped silently, with no adjustment and no error. The view hides the payment radios when the pending membership is already paid, so reaching this takes a hand-crafted request, and it's fine as defense in depth. If you'd rather it be loud, raising here would make a misrouted cash payment impossible to lose quietly.
| 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:) |
There was a problem hiding this comment.
Not new to this PR, but now that the two branches sit side by side: start! gives 364 days and this path gives 365. Might be a nice moment to pick one.
There was a problem hiding this comment.
Good call, how 'bout we go with 1.year instead!
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| Member.create!(member_attrs.merge( | ||
| email: pending_member_user.email, full_name: "Pending Member", preferred_name: "Pending", user: pending_member_user | ||
| )) |
| 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 | ||
| )) |
This branch has not been deployed
4 participants
Fixes #2168 by resolving a gap in how we were modeling the membership signup and renewal process. After a member completed renewal "in person" (#skip), no membership record was created, so the "your membership expired" banner never cleared. Fixing that surfaced two follow-on issues — how staff finish those renewals, and a payment double-charge — which this branch also addresses.
The main change is that we always create a
Membershipobject when going through either the signup or renewal flows (previously, we didn't if you chose the "pay in person" option). Unifying the flow removes a bunch of edge cases from the app going forward.