fix(spp_scoring,spp_scoring_programs): scoring validation, eligibility enforcement, and access control#161
fix(spp_scoring,spp_scoring_programs): scoring validation, eligibility enforcement, and access control#161
Conversation
…laps Previously only positive gaps were detected. Now also checks for overlapping thresholds (negative gaps) across ALL consecutive boundaries, not just the first. Error messages include actual max/min values for easier debugging.
…dels Error messages now include model name, bullet points, and actionable text directing users to the Indicators/Thresholds tabs.
The _enroll_eligible_registrants method wrote state=enrolled directly without calling pre/post enrollment hooks. This bypassed scoring eligibility checks from spp_scoring_programs. Now the pre-enrollment hook runs per member before enrollment, and members that fail (e.g., NON_POOR classification when only EXTREME_POOR/POOR are allowed) are marked not_eligible instead of enrolled.
…r enrollment The _post_enrollment_hook calculated a score but did not write it to the membership record. Now writes enrollment_score and enrollment_classification on the membership after enrollment. Falls back to latest existing score if auto-scoring is disabled or fails.
…hip list Changed enrollment_score, enrollment_classification, latest_score, and latest_classification from optional=hide to optional=show so they are visible by default when spp_scoring_programs is installed.
…lation score_max_age_days=0 was treated as 'no limit' (passed None to engine) instead of 'always recalculate'. Now explicitly passes 0 which the engine treats as 'skip cache, always recalculate'. Also made the engine check explicit: max_age_days > 0 to use cache.
Scoring viewer users got an access error on spp.scoring.model.program_ids because they had no read access to spp.program. Added read-only ACL for spp.program and spp.program.membership to the scoring viewer group.
…agers Officers and viewers could see and click Activate/Deactivate buttons on scoring models even though their ACLs prevented the write from succeeding. Now the buttons are only visible to scoring managers.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces pre- and post-enrollment hooks within the program manager to handle scoring eligibility and record enrollment scores on membership records. It also enhances scoring model validation to detect both gaps and overlaps in thresholds, restricts scoring model activation to managers, and updates membership views to display scoring results by default. Review feedback highlights performance concerns regarding N+1 database queries and per-member hook execution during bulk enrollment, alongside a potential type error in threshold validation if score fields are null.
| for member in not_enrolled: | ||
| try: | ||
| program._pre_enrollment_hook(member.partner_id) | ||
| except (ValidationError, UserError) as e: | ||
| _logger.info( | ||
| "Pre-enrollment hook rejected registrant %s: %s", | ||
| member.partner_id.id, | ||
| str(e), | ||
| ) | ||
| hook_failed |= member |
There was a problem hiding this comment.
Calling the pre-enrollment hook inside a loop for each member can lead to significant performance bottlenecks during bulk enrollment, as each hook execution might involve complex logic or database queries (like scoring). Consider refactoring the hook API to support batch processing of recordsets for better efficiency.
| sorted_thresholds = self.threshold_ids.sorted(key=lambda t: t.min_score) | ||
|
|
||
| # Check for gaps between thresholds | ||
| # Check all consecutive threshold boundaries for gaps and overlaps | ||
| for i, threshold in enumerate(sorted_thresholds[:-1]): | ||
| next_threshold = sorted_thresholds[i + 1] | ||
| gap = next_threshold.min_score - threshold.max_score |
There was a problem hiding this comment.
The sorting and gap calculation will fail with a TypeError if min_score or max_score are None. Since these fields might not be strictly required at the database level, it's safer to handle potential None values or ensure they default to 0.0 during validation.
sorted_thresholds = self.threshold_ids.sorted(key=lambda t: t.min_score or 0.0)
# Check all consecutive threshold boundaries for gaps and overlaps
for i, threshold in enumerate(sorted_thresholds[:-1]):
next_threshold = sorted_thresholds[i + 1]
if threshold.max_score is None or next_threshold.min_score is None:
continue
gap = next_threshold.min_score - threshold.max_score| membership = self.env["spp.program.membership"].search( | ||
| [ | ||
| ("partner_id", "=", partner.id), | ||
| ("program_id", "=", self.id), | ||
| ("state", "=", "enrolled"), | ||
| ], | ||
| limit=1, | ||
| ) | ||
| if membership: | ||
| membership.write( | ||
| { | ||
| "enrollment_score": score_result.score, | ||
| "enrollment_classification": score_result.classification_code, | ||
| } | ||
| ) |
There was a problem hiding this comment.
This search and subsequent write are performed inside a loop for each partner being enrolled. This creates an N+1 performance issue, especially during bulk enrollment. Since the caller (ProgramManager._enroll_eligible_registrants) already has the membership record, the hook API should ideally be refactored to accept the membership record directly to avoid redundant database lookups and multiple write calls.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #161 +/- ##
=======================================
Coverage 71.33% 71.33%
=======================================
Files 932 932
Lines 54975 54980 +5
=======================================
+ Hits 39217 39221 +4
- Misses 15758 15759 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
1 participant
Why is this change needed?
Multiple bugs in the scoring and scoring-programs modules were blocking QA testing:
How was the change implemented?
spp_scoring
group_scoring_managerspp_programs
_enroll_eligible_registrants()now callsprogram._pre_enrollment_hook()before enrollment and_post_enrollment_hook()after. Members failing the hook are markednot_eligiblespp_scoring_programs
_post_enrollment_hook()now writesenrollment_scoreandenrollment_classificationto the membership recordoptional="hide"tooptional="show"for enrollment/latest score columnsscore_max_age_days=0now correctly forces recalculation instead of being treated as "no limit"spp.programandspp.program.membershipto scoring viewer groupNew unit tests
N/A — changes are in validation logic, views, and ACLs. Existing tests cover the core paths.
Unit tests executed by the author
Existing spp_scoring and spp_programs tests pass.
How to test manually
Related links