feat(patrons): add task for automatic deletion of inactive patrons#4074
feat(patrons): add task for automatic deletion of inactive patrons#4074PascalRepond wants to merge 3 commits into
Conversation
|
coverage: 91.333% (+0.02%) from 91.312% — PascalRepond:rep-patron-delete into rero:staging |
PascalRepond
force-pushed
the
rep-patron-delete
branch
from
36fe4e4 to
6ab5fd9
Compare
Co-Authored-by: Pascal Repond <pascal.repond@rero.ch>
…hecks - Add OperationLogObserverExtension to Patron._extensions so all patron CRUD operations are audited in operation logs. - Add "patrons": "ptrn" to RERO_ILS_ENABLE_OPERATION_LOG config. - Add ILL requests check to Patron.get_links_to_me() to prevent deleting patrons with linked ILL requests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PascalRepond
force-pushed
the
rep-patron-delete
branch
from
6ab5fd9 to
ac66856
Compare
PascalRepond
changed the title
PascalRepond
force-pushed
the
rep-patron-delete
branch
4 times, most recently
from
cc0f0d3 to
57fa9a8
Compare
WalkthroughThis PR implements an inactive patron cleanup feature with supporting infrastructure. It adds configurable organisation-level policies (expiration and inactivity years), a monthly Celery task that filters patrons via Elasticsearch and applies multi-criteria exclusions (blocked, recent activity, active loans/requests, professional roles, excluded types), and supports dry-run mode. Operation log support for patrons is added by resolving circular imports, extending the operation log schema to include "ptrn" records, and registering the observer extension on Patron records. The patron API is enhanced to include inter-library loan request links. Comprehensive test coverage validates all exclusion conditions, positive deletion, and end-to-end execution. Supporting test improvements include a refactored unpaid subscription test, dynamic operation log assertions, and mock patch location updates. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
CLAUDE.md (1)
147-151: 💤 Low valueAdd language identifier to fenced code block.
The code block should specify a language for better markdown tooling support.
📝 Proposed fix
-``` +```text 1. [Step] → verify: [check] 2. [Step] → verify: [check] 3. [Step] → verify: [check]</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@CLAUDE.mdaround lines 147 - 151, The fenced code block with the checklist
lacks a language identifier; update the triple-backtick fence in the CLAUDE.md
snippet (the block showing "1. [Step] → verify: [check]" etc.) to include a
language tag such as "text" or "txt" (e.g., replacewithtext) so
markdown tooling and syntax highlighters can properly recognize the block.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In@tests/api/patrons/test_patrons_tasks.py:
- Line 125: The test currently unpacks unused return values from
_delete_inactive_patrons_for_org (e.g., deleted, skipped) and unused user_id
variables across tests; replace those unused variable names with _ (or assert
them if needed) in the calls and setups referencing
_delete_inactive_patrons_for_org and any places setting user_id to avoid lint
errors (ruff) — update each occurrence listed (lines around the calls noted in
tests/api/patrons/test_patrons_tasks.py) so unused tuple elements are assigned
to _ and remove or assert any unused user_id.
Nitpick comments:
In@CLAUDE.md:
- Around line 147-151: The fenced code block with the checklist lacks a language
identifier; update the triple-backtick fence in the CLAUDE.md snippet (the block
showing "1. [Step] → verify: [check]" etc.) to include a language tag such as
"text" or "txt" (e.g., replacewithtext) so markdown tooling and syntax
highlighters can properly recognize the block.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `339509ff-47e3-49b6-9fa4-28124f9defe1` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 994996ed05d486e863967c662e16bd9b536751b0 and 57fa9a83865a63ab9410fd16c7294f596642b2e8. </details> <details> <summary>📒 Files selected for processing (14)</summary> * `CLAUDE.md` * `rero_ils/config.py` * `rero_ils/modules/operation_logs/extensions.py` * `rero_ils/modules/operation_logs/jsonschemas/operation_logs/operation_log-v0.0.1.json` * `rero_ils/modules/organisations/jsonschemas/organisations/organisation-v0.0.1.json` * `rero_ils/modules/organisations/mappings/v7/organisations/organisation-v0.0.1.json` * `rero_ils/modules/patrons/api.py` * `rero_ils/modules/patrons/tasks.py` * `tests/api/circulation/test_borrow_limits.py` * `tests/api/operation_logs/test_operation_logs_rest.py` * `tests/api/patrons/test_patrons_rest.py` * `tests/api/patrons/test_patrons_tasks.py` * `tests/ui/stats/test_stats_librarian.py` * `tests/ui/stats/test_stats_pricing.py` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Add a monthly Celery task that deletes inactive patron profiles per organisation based on configurable criteria: expiration date, last login date, patron type exclusion list. - Add patron_cleanup config to organisation schema and ES mapping - Register task in CELERY_BEAT_SCHEDULE - Add ptrn to operation log record type enum - Fix circular import: move current_librarian to local import in operation_logs/extensions.py - Skip operation log creation when the record has no pid (e.g. hard delete of a soft-deleted record whose data dict has been cleared) - Use timezone-aware datetimes in the cleanup task to compare safely with user.current_login_at (UTC-aware from PostgreSQL) - Update test_stats_librarian and test_stats_pricing mocks to target rero_ils.modules.patrons.api.current_librarian after the import move - Refactor test_unpaid_subscription: replace setdefault (which left the limit at its fixture value of False) with direct assignment, wrap the test in try/finally to restore the module-scoped patron_type, fix the response check to assert res.status_code, and document a re-login workaround for the babel-triggered _load_user - Add tests for task_delete_inactive_patrons covering each deletion criterion (expiration, inactivity, blocked, excluded type, active loans/transactions/ILL requests, never-logged-in user, dry-run, end-to-end) - Add test for ill_requests in Patron.get_links_to_me - Also adds Claude behavioural guidelines for implementation. Closes US#2802 Co-Authored-by: Pascal Repond <pascal.repond@rero.ch>
PascalRepond
force-pushed
the
rep-patron-delete
branch
from
57fa9a8 to
0f82cd2
Compare
| if not config: | ||
| continue | ||
| _delete_inactive_patrons_for_org(org, config, dry_run=dry_run) | ||
| set_timestamp("delete_inactive_patrons") |
There was a problem hiding this comment.
nice to have also the values org, deleted, skipped
| """ | ||
| for org in Organisation.get_all(): | ||
| config = org.get("patron_cleanup") | ||
| if not config: |
There was a problem hiding this comment.
Dont like to much jumps. Maybe:
delete_inactive_patrons = {}
for org in Organisation.get_all():
if config := org.get("patron_cleanup")
deleted, skipped = _delete_inactive_patrons_for_org(org, config, dry_run=dry_run)
delete_inactive_patrons[org.pid] = {
"deleted": deleted,
"skipped": skipped
}
if not dry_run:
set_timestamp("delete_inactive_patrons", **delete_inactive_patrons)| set_timestamp("clear_and_renew_subscriptions") | ||
|
|
||
|
|
||
| def _delete_inactive_patrons_for_org(org, config, dry_run=False): |
There was a problem hiding this comment.
If we give config we need only org.pid or we don't give config and get the config directly from org
| .filter("term", roles="patron") | ||
| .filter("range", patron__expiration_date={"lt": expiration_cutoff.strftime("%Y-%m-%d")}) | ||
| .exclude("term", patron__blocked=True) | ||
| .exclude("terms", roles=list(UserRole.PROFESSIONAL_ROLES)) |
There was a problem hiding this comment.
Is .filter("term", roles="patron") and .exclude("terms", roles=list(UserRole.PROFESSIONAL_ROLES)) not redundant?
Better to use UserRole.PATRON for "patron".
| for hit in query.scan(): | ||
| # Criterion: patron_type not in exclusion list | ||
| ptty_pid = hit.patron.type.pid | ||
| if ptty_pid in excluded_ptty_pids: |
There was a problem hiding this comment.
if hit.patron.type.pid in excluded_ptty_pids:| skipped += 1 | ||
| continue | ||
|
|
||
| # Criterion: no active links (loans, open transactions, ILL |
There was a problem hiding this comment.
we have 120 characters on line, the command should be passing on one line.
3 participants
feat(patrons): wire operation logs and add ILL requests to deletion checks
CRUD operations are audited in operation logs.
patrons with linked ILL requests.
feat(patrons): add task for automatic deletion of inactive patrons
feat(patrons): add task for automatic deletion of inactive patrons
Add a monthly Celery task that deletes inactive patron profiles per
organisation based on configurable criteria: expiration date, last login
date, patron type exclusion list.
operation_logs/extensions.py
delete of a soft-deleted record whose data dict has been cleared)
with user.current_login_at (UTC-aware from PostgreSQL)
rero_ils.modules.patrons.api.current_librarian after the import move
the limit at its fixture value of False) with direct assignment,
wrap the test in try/finally to restore the module-scoped
patron_type, fix
assert res == 200to check status_code, anddocument a re-login workaround for the babel-triggered _load_user
criterion (expiration, inactivity, blocked, excluded type, active
loans/transactions/ILL requests, never-logged-in user, dry-run,
end-to-end)
Closes US2802