fix: Skip stale Press Role resources during sync#6558
Conversation
|
| Filename | Overview |
|---|---|
| press/press/doctype/team_member_resource/team_member_resource.py | Adds a stale-resource guard in sync_press_role that checks document ownership via frappe.db.get_value before inserting; logically correct but introduces an additional per-resource DB query in an already N+1 inner loop that runs on every Press Role save. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[sync_press_role triggered\non Press Role save / patch] --> B[Delete all Team Member Resources\nfor the team]
B --> C[For each team member user]
C --> D[Query Press Role Resources\nfor user in team]
D --> E[For each resource]
E --> F{NEW: frappe.db.get_value\ndocument_type, document_name, 'team'}
F -->|document_team != team\nor doc not found| G[Skip - stale resource\narchived / transferred / deleted]
F -->|document_team == team| H{frappe.db.exists\nTeam Member Resource}
H -->|exists| I[Skip - duplicate]
H -->|not exists| J[frappe.get_doc.insert\nignore_permissions=True]
E --> K{More resources?}
K -->|Yes| E
K -->|No| C
G --> K
I --> K
J --> K
Reviews (1): Last reviewed commit: "fix: Skip stale Press Role resources dur..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for resource in resources: | ||
| # Skip stale resources whose document has been transferred or archived. | ||
| document_team = frappe.db.get_value(resource.document_type, resource.document_name, "team") | ||
| if document_team != team: | ||
| continue |
There was a problem hiding this comment.
frappe.db.get_value is issued for every resource entry in the inner loop, which already runs once per user. Because sync_press_role fires as a hook on every Press Role save (not only during the migration patch), a team with many users and many resources will generate a large number of individual SELECT queries on each save. The existing frappe.db.exists call at line 148 compounds the same pattern. Consider collecting all unique (document_type, document_name) pairs from resources before the loop and resolving their teams in per-type batches (one frappe.db.get_all per distinct document_type), then look up results from a local dict during the iteration.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #6558 +/- ##
===========================================
+ Coverage 50.11% 50.14% +0.03%
===========================================
Files 954 954
Lines 78895 78917 +22
Branches 369 369
===========================================
+ Hits 39537 39573 +36
+ Misses 39332 39318 -14
Partials 26 26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Problem
The decouple_press_role_resources patch fails with a ValidationError when a Press Role Resource references a document that is no longer owned by the team. This happens when:
A site, bench, or server has been archived (renamed to {name}.archived)
A document has been transferred to another team (team switch)
A document has been deleted entirely
In all these cases, validate_document_name throws because the current document owner doesn't match the team on the resource being created.
Fix
In sync_press_role, check the document's current team before attempting to insert a Team Member Resource entry. If the document no longer belongs to the team (or doesn't exist), skip it silently.
This check is type-agnostic and covers all three permitted document types: Site, Release Group, and Server.