NEUSPRT-571: Add Setting For Relationship End Date Trigger - #7
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new CiviRules cron trigger that fires when a relationship's end date is reached, allowing users to target relationships that ended within a specified interval. The feedback focuses on PHP 8 compatibility improvements, specifically recommending defensive checks to prevent warnings when unserializing trigger parameters or accessing potentially non-array properties like triggerParams.
erawat
left a comment
There was a problem hiding this comment.
Approved. Core mechanism is sound and upgrade-safe — verified the de-dup join keys (entity_table/entity_id) are populated by logRule() even on the legacy path, so no double-firing on upgrade; interval binding + unit whitelist are injection-safe.
Non-blocking follow-ups:
- Product sign-off needed on the once-per-latest → once-per-every-relationship behavioral change (currently only a footnote).
default: return parent::getHelpText($context)in RelationshipEndDate.php calls a non-existent base method — latent fatal, should return ''.- Trigger help text never renders because the form does not override getHelpText() (cf. MembershipEndDate form).
- Consider unit tests around the window boundary and once-per-end-date de-dup.
Whatever is adjusted here should be mirrored on the sibling PR since the diffs are identical.
efef7c8 to
01247d3
Compare
01247d3 to
07bdf81
Compare
@erawat 1 and 3 should be fine imo, 2and 4 have been addressed |
Overview
The CiviRules "Relationship end date reached" trigger was checking every relationship that had ever ended, on every cron run (every ~15 minutes). On a large database this generated a huge number of queries and made the scheduled job very slow. This PR adds a configurable time window to the trigger so it only picks up recently ended relationships, and makes each relationship trigger a rule only once instead of every day.
Before
civirule_rule_logtable using a non-index-friendly date comparison, so cron runs became progressively slower and consumed significant DB capacity.After
Technical Details
CRM/CivirulesCronTrigger/RelationshipEndDate.phpqueryForTriggerEntities()now branches: if the trigger hasinterval+interval_unitparams, it runs a windowed query; otherwise it runs the legacy query.end_date >= DATE_SUB(CURDATE(), INTERVAL %2 <UNIT>)bounds the scan, and de-duplication is per relationship viaLEFT JOIN civirule_rule_log ON rule_id / entity_table = 'civicrm_relationship' / entity_id = r.id AND log_date >= r.end_date ... WHERE rule_log.id IS NULL.days/weeks/months/years→DAY/WEEK/MONTH/YEAR) before being interpolated into SQL; the interval value is passed as a boundIntegerparameter.getExtraDataInputUrl(),intervals(),getTriggerDescription()andgetHelpText().DATE(log_date) = DATE(NOW())was replaced with the equivalent but index-friendly (sargable)log_date >= CURDATE().CRM/CivirulesCronTrigger/Form/RelationshipEndDate.php(new) +templates/CRM/CivirulesCronTrigger/Form/RelationshipEndDate.tpl(new)civirule_rule.trigger_params. Validates the interval as a positive integer greater than 0.xml/Menu/civirules.xmlcivicrm/civirule/form/trigger/relationshipenddatefor the form.Comments