fix(documents): count documents instead of holdings in org facet#4180
fix(documents): count documents instead of holdings in org facet#4180jma wants to merge 1 commit into
Conversation
WalkthroughThe organisation facet aggregation now adds reverse-nested document counts at organisation and location levels. Serializer post-processing rewrites nested buckets to use those counts, removes empty buckets, sorts results, and applies local-view filtering. REST expectations and new unit coverage validate document-based counts, bucket pruning, and ordering. Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
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 `@rero_ils/config.py`:
- Around line 2129-2147: Update the nested terms aggregations in the
record-count query so both library and location buckets order by their
reverse-nested record_count in descending document-count order before applying
size limits. Add a regression case with more than 50 libraries and more than 100
locations that verifies the highest-record-count buckets are retained.
In `@rero_ils/modules/documents/serializers/json.py`:
- Around line 178-193: Update the organisation, library, and location bucket
rewriting flow around _rewrite_nested_buckets so returned aggregations no longer
retain stale sum_other_doc_count or doc_count_error_upper_bound metadata from
the nested terms response. Remove those fields or recompute them consistently
for the rewritten buckets, and add coverage using a non-zero metadata case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e7f301e-64af-432e-80fb-e1c88fa84bb6
📒 Files selected for processing (4)
rero_ils/config.pyrero_ils/modules/documents/serializers/json.pytests/api/documents/test_documents_rest.pytests/unit/documents/test_documents_serializers.py
The organisation -> library -> location facet is built on the `nested_holdings` nested field, so terms sub-aggregations count holdings, not documents: counts were inflated whenever a document had several holdings in the same bucket. Add a `reverse_nested` sub-aggregation (`record_count`) at each level to count distinct parent documents, and expose it as `doc_count` in the serializer. Order the terms buckets by `record_count` in the query so the `size` cut keeps the buckets with the most documents: Elasticsearch orders terms by holdings count by default, so a library or location with many documents but few holdings could otherwise be dropped before the serializer sees it. The query now returns the buckets already ordered by document count, so the serializer no longer re-sorts them. Drop the `sum_other_doc_count` and `doc_count_error_upper_bound` terms metadata: they still describe the holdings-count aggregation and no longer match the rewritten (document-count) buckets. They cannot be recomputed for documents, as a document may belong to several buckets. * Closes rero#4003. Co-Authored-by: Johnny Mariéthoz <johnny.mariethoz@rero.ch> Co-Authored-by: Ana Gomes <anagoncalves7@icloud.com>
The organisation -> library -> location facet is built on the
nested_holdingsnested field, so terms sub-aggregations count holdings,not documents: counts were inflated whenever a document had several
holdings in the same bucket.
Add a
reverse_nestedsub-aggregation (record_count) at each level tocount distinct parent documents, and expose it as
doc_countin theserializer. Buckets are also re-sorted by document count, since
Elasticsearch orders terms by holdings count and the displayed number is
now the document count.
Co-Authored-by: Johnny Mariéthoz johnny.mariethoz@rero.ch
Co-Authored-by: Ana Gomes anagoncalves7@icloud.com