Wishlist - Action Trigger for periodic billing statement

Bug #1496522 reported by Blake GH on 2015-09-16
36
This bug affects 8 people
Affects Status Importance Assigned to Milestone
Evergreen
Wishlist
Unassigned

Bug Description

We would like an action trigger that that will send an email/text with a patron's "balance owed". We envision the hook to simply happen when the cron script runs. The patron selection should be scoped to the configured "owning library" supplied by the event definition. It would be nice if the patrons that were selected had an email address in the case of "SendEmail" and a non-null "opac.default_sms_notify" value for SMS. Furthermore, the event definition should be able to specify a threshold that the "balance owed" needs to exceed before the patron is included in the scope.

We can utilize the view money.usr_summary. I don't see how the current A/T event definition settings will compare a supplied numeric value against money.usr_summary.balance_owed. Would that go under "validator"?

Blake GH (bmagic) on 2015-10-02
tags: added: wishlist
Blake GH (bmagic) wrote :

Here is what I came up with so far.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/blake/LP1496522_action_trigger_for_periodic_billing_statement

This method will target all of the patrons who have ever had a bill in a given org unit (owner of the AT compared to the billing_ou in the view). Then the validator step will weed them out according to the OU library setting. I recommend using a granularity although the stock insert does not setup a granularity. I couldn't decide which granularity to use by default. I suppose "Monthly"

Blake GH (bmagic) on 2015-10-05
Changed in evergreen:
status: New → In Progress
assignee: nobody → Blake GH (blake-j)
Blake GH (bmagic) on 2015-10-05
tags: added: pullrequest
Changed in evergreen:
assignee: Blake GH (blake-j) → nobody
Kathy Lussier (klussier) on 2015-12-09
Changed in evergreen:
status: In Progress → Triaged
importance: Undecided → Wishlist
milestone: none → 2.next
Changed in evergreen:
assignee: nobody → Josh Stompro (u-launchpad-stompro-org)
Changed in evergreen:
assignee: Josh Stompro (u-launchpad-stompro-org) → nobody
Michele Morgan (mmorgan) on 2017-02-06
Changed in evergreen:
milestone: 2.next → 2.12-beta
Dan Wells (dbw2) wrote :

At Calvin we need something similar to this feature, and spent some time today looking at this branch. We ran into a few bugs, so Remington is going to post a branch with potential fixes shortly.

I don't feel this is quite ready for prime-time, as it generates a large number of events later deemed invalid. Specifically, I believe we will end up with an invalid event generated for every patron with a balance below or at the threshold every 30 days. Some of that may be hard to avoid if we want a configurable threshold per org unit, but we probably want to at least filter 0 balance events.

Remington Steed (rjs7) wrote :

Here's the working branch:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/rsteed/LP1496522_action_trigger_for_periodic_billing_statement

Leaving the pullrequest tag for now, but as Dan mentioned, it may not be ready yet.

Blake GH (bmagic) wrote :

Glad to have more eyes on it! LOL typos. I like your query improvement. Can the invalid events be mitigated with a clever event definition? The delay default is set to last_payment_ts. Maybe there is something we can do there? I'm not sure exactly what. Perhaps the query could include a date that the trigger could compare to. I assume we want this notice to repeat even when the patron touched their account more than 30 days ago?

Remington Steed (rjs7) wrote :

Also note, the content of the upgrade script needs to be added to the seed data and schema DB files so they are included in a fresh install.

Mike Rylander (mrylander) wrote :

I like the changes that Remington made to the view. While not very widely used, I think we should add booking reservations to the UNION as well. Thoughts?

Thanks, all!

Changed in evergreen:
milestone: 2.12-beta → 2.next
Remington Steed (rjs7) on 2017-05-11
Changed in evergreen:
assignee: nobody → Remington Steed (rjs7)
Blake GH (bmagic) wrote :

Remington,

I put your view on our production. I found that the query was causing action trigger cron jobs to dog pile. The query was too slow. To fix it in the immediate, I editing the query to only return balance_owed > 0

CREATE OR REPLACE VIEW money.usr_summary_per_org_unit AS
 WITH located_xact AS (
         SELECT circulation.id,
            circulation.circ_lib AS billing_ou
           FROM action.circulation
        UNION
         SELECT grocery.id,
            grocery.billing_location AS billing_ou
           FROM money.grocery
        )
 SELECT mmbts.usr,
    located_xact.billing_ou,
    sum(mmbts.total_paid) AS total_paid,
    sum(mmbts.total_owed) AS total_owed,
    sum(mmbts.balance_owed) AS balance_owed,
    COALESCE(max(mmbts.last_payment_ts), '0001-01-01 00:00:00'::timestamp without time zone::timestamp with time zone) AS last_payment_ts
   FROM money.materialized_billable_xact_summary mmbts
     JOIN located_xact ON located_xact.id = mmbts.id
  WHERE mmbts.balance_owed > 0::numeric
  GROUP BY mmbts.usr, located_xact.billing_ou;

It also cut back on the number of "invalid" action_trigger.event rows.

Of course, it's not the solution unless we change the name of the view to reflect the filter.

Dan Wells (dbw2) wrote :

We've continued to poke at this with the intention of adopting it locally. The latest effort is now branchified here:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/dbwells/LP1496522_action_trigger_for_periodic_billing_statement

The branch incorporates Mike's suggestion regarding booking, then reworks the internals a fair bit.

To quote extensively from the most recent commit:

----------------------

This commit builds on the work that Blake had done, with two main goals.
First, it pushes some of the logic down the stack to reduce churn.
Second, it renames and relabels a number of things to better match
precedent.

For the first part, one particular concern with the existing code is
that it unavoidably generates 'invalid' events. With the balance_owed
filter applied, this problem was reduced but not eliminated. The
solution in this commit may be overkill, but it might also serve as a
model for similar cases.

In brief, this code extends the new view to do the necessary setting
lookup and subsequent filtering in the DB layer, thereby avoiding any
dead ends, and in fact eliminating the need for event validation at all.

For the second, the following strings were changed, mostly to match
existing precedent, but occasionally just for clarity:

Trigger name:
- was: patron_has_bills
- now: money.usr_exceeds_balance_threshold

View name:
- was: money.usr_summary_per_org_unit (still exists for future reuse)
- now: money.usr_exceeds_balance_threshold (builds on original generic
  view)

Setting name:
- was: patron.notify_bills_when_exceeds
- now: circ.notify_when_balance_exceeds

Setting label:
- was: Notify Patron bill when exceeds
- now: Notify Patron When Balance Exceeds

Event definition name:
- was: Patron recurring 1 month billing notice
- now: Monthly Patron Balance Notice

Additional notes:
- I did not understand the supplied tests as written, as they appeared
  to merely create everything, then see if those things were created. I
  left one test which checks if the base view exists and at least
  functions. We probably really want a live test for this if possible.

- The template in the seed file was all indented within the multi-line
  quote, which seemed likely to cause problems. This indent is removed.

- The new view requires (I believe) PG 9.3+. It references a previous
  column in a later join, similar to a lateral join. Version 9.3 is the
  baseline since 2.11, so this should be fine for all supported EG
  versions (and likely wouldn't be backported anyway?).

----------------------

This code is tested and works, but the branch as assembled may still contain a few missing bits. Please let me know if you run into any issues. Thanks!

Changed in evergreen:
assignee: Remington Steed (rjs7) → nobody
Galen Charlton (gmc) on 2017-08-10
Changed in evergreen:
assignee: nobody → Galen Charlton (gmc)
milestone: 3.next → 3.0-alpha
Galen Charlton (gmc) wrote :
Download full text (3.4 KiB)

I've looked at it, and while it appears that it would work as expected for single-branch systems, it doesn't look like it will handle multi-branch systems and consortia as well as it ought to.

In particular:

- there's no way to aggregate user balances at an OU and its descendants, meaning that a patron who uses more than one branch in a system would be getting a billing notice from each branch
- the template bases the from address on the user's home OU, but doesn't include anything to indicate that the billing may be from an entirely different system

Also, some testing of the new views in a test database with 30 million circulation rows gives me pause:

# explain analyze SELECT COUNT(*) FROM money.usr_summary_per_org_unit WHERE billing_ou = 123; QUERY PLAN
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 Aggregate (cost=7798960.10..7798960.11 rows=1 width=0) (actual time=59222.777..59222.777 rows=1 loops=1)
   -> HashAggregate (cost=7797444.95..7798118.35 rows=67340 width=8) (actual time=59221.012..59222.440 rows=6253 loops=1)
         Group Key: mmbts.usr, located_xact.billing_ou
         CTE located_xact
           -> Unique (cost=6323431.77..6559224.81 rows=31439072 width=12) (actual time=39596.034..51380.077 rows=31436345 loops=1)
                 -> Sort (cost=6323431.77..6402029.45 rows=31439072 width=12) (actual time=39596.032..46771.153 rows=31436345 loops=1)
                       Sort Key: circulation.id, circulation.circ_lib
                       Sort Method: external merge Disk: 676072kB
                       -> Append (cost=0.00..1640755.44 rows=31439072 width=12) (actual time=0.031..8541.612 rows=31436345 loops=1)
                             -> Seq Scan on circulation (cost=0.00..1315601.01 rows=30938501 width=12) (actual time=0.029..6779.568 rows=30935769 loops=1)
                             -> Seq Scan on grocery (cost=0.00..10762.41 rows=500541 width=12) (actual time=0.019..79.894 rows=500546 loops=1)
                             -> Seq Scan on reservation (cost=0.00..1.30 rows=30 width=12) (actual time=0.007..0.014 rows=30 loops=1)
         -> Nested Loop (cost=0.56..1237434.17 rows=157195 width=8) (actual time=39851.844..59120.777 rows=435150 loops=1)
               -> CTE Scan on located_xact (cost=0.00..707379.12 rows=157195 width=12) (actual time=39851.793..57277.208 rows=435150 loops=1)
                     Filter: (billing_ou = 123)
                     Rows Removed by Filter: 31001195
               -> Index Scan using materialized_billable_xact_summary_pkey on materialized_billable_xact_summary mmbts (cost=0.56..3.36 rows=1 width=12) (actual time=0.004..0.004 rows=1 loops=435150)
                     Index Cond: (id = located_xact.id)
 Planning time: 0.495 ms
 Execution time: 59442.586 ms
(20 rows)

In the context of this specific A/T event definition, the sequential scans on action.circulation and action.circulation aren't necessarily a huge ...

Read more...

tags: added: needsrepatch
Changed in evergreen:
assignee: Galen Charlton (gmc) → nobody
milestone: 3.0-alpha → 3.next
Dan Wells (dbw2) on 2018-02-13
tags: removed: pullrequest
Scott Thomas (scott-thomas-9) wrote :

PaILS is very interested in this feature.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers