Skip to content

feat(items): add sort_call_number index fields#4047

Merged
PascalRepond merged 1 commit intorero:stagingfrom
PascalRepond:rep-call-numbers
Mar 12, 2026
Merged

feat(items): add sort_call_number index fields#4047
PascalRepond merged 1 commit intorero:stagingfrom
PascalRepond:rep-call-numbers

Conversation

@PascalRepond
Copy link
Contributor

@PascalRepond PascalRepond commented Mar 5, 2026

@coderabbitai
Copy link

coderabbitai bot commented Mar 5, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f589ca76-74cb-457c-9235-d3d9ce60fef3

📥 Commits

Reviewing files that changed from the base of the PR and between 65f5228 and f35f9df.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (5)
  • rero_ils/config.py
  • rero_ils/modules/items/listener.py
  • rero_ils/modules/items/mappings/v7/items/item-v0.0.1.json
  • tests/ui/holdings/test_issues_reindex.py
  • tests/ui/items/test_items_mapping.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • rero_ils/modules/items/mappings/v7/items/item-v0.0.1.json
  • rero_ils/config.py

Walkthrough

Adds dedicated Elasticsearch sort fields for items: sort_call_number and sort_second_call_number. Updates index mappings, indexing/enrichment logic to populate those fields (with fallback to inherited holding values), changes sort configuration to use them, and adds tests validating the behavior.

Changes

Cohort / File(s) Summary
Sorting Configuration
rero_ils/config.py
Replaced item sort keys to use sort_call_number and sort_second_call_number instead of the previous .raw fields.
Indexing Logic
rero_ils/modules/items/listener.py
Populate new sort_call_number and sort_second_call_number in item enrichment; derive from item fields or fallback to issue.inherited_* holdings values.
Elasticsearch Mappings
rero_ils/modules/items/mappings/v7/items/item-v0.0.1.json
Added sort_call_number and sort_second_call_number as keyword fields to the item mapping.
Test Coverage
tests/ui/holdings/test_issues_reindex.py, tests/ui/items/test_items_mapping.py
Added/extended tests asserting that sort fields reflect item values or inherited holding values and update correctly after reindexing.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding sort_call_number index fields to support improved sorting behavior for call numbers.
Description check ✅ Passed The description references the linked issue (#4009) and mentions the requirement for full reindex, relating to the changeset's purpose.
Linked Issues check ✅ Passed The PR fully implements issue #4009 by adding sort_call_number and sort_second_call_number fields that fall back to inherited values, with proper configuration, mapping, enrichment logic, and test coverage.
Out of Scope Changes check ✅ Passed All changes are directly within scope: configuration updates, enrichment logic, mapping definitions, and comprehensive tests for the new sorting feature.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/ui/holdings/test_issues_reindex.py (1)

125-157: Scope sort-field assertions by item PID for stronger test determinism.

These checks currently count all matching docs in the index. Filtering by pid=item.pid makes the assertions robust against incidental fixture overlap.

♻️ Proposed fix
-    assert ItemsSearch().filter("term", sort_call_number="cote1").count() == 1
+    assert (
+        ItemsSearch()
+        .filter("term", pid=item.pid)
+        .filter("term", sort_call_number="cote1")
+        .count()
+        == 1
+    )
@@
-    assert ItemsSearch().filter("term", sort_call_number="item-own-cote").count() == 1
-    assert ItemsSearch().filter("term", sort_call_number="cote1").count() == 0
+    assert (
+        ItemsSearch()
+        .filter("term", pid=item.pid)
+        .filter("term", sort_call_number="item-own-cote")
+        .count()
+        == 1
+    )
+    assert (
+        ItemsSearch()
+        .filter("term", pid=item.pid)
+        .filter("term", sort_call_number="cote1")
+        .count()
+        == 0
+    )
@@
-    assert ItemsSearch().filter("term", sort_call_number="cote1").count() == 1
+    assert (
+        ItemsSearch()
+        .filter("term", pid=item.pid)
+        .filter("term", sort_call_number="cote1")
+        .count()
+        == 1
+    )
@@
-    assert ItemsSearch().filter("term", sort_second_call_number="cote2").count() == 1
-    assert ItemsSearch().filter("term", sort_call_number="cote1").count() == 0
+    assert (
+        ItemsSearch()
+        .filter("term", pid=item.pid)
+        .filter("term", sort_second_call_number="cote2")
+        .count()
+        == 1
+    )
+    assert (
+        ItemsSearch()
+        .filter("term", pid=item.pid)
+        .filter("term", sort_call_number="cote1")
+        .count()
+        == 0
+    )
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/ui/holdings/test_issues_reindex.py` around lines 125 - 157, The
assertions in test_issues_reindex.py use ItemsSearch().filter(...) without
scoping to the specific item, causing flakiness; update each
ItemsSearch().filter call (including checks for sort_call_number,
sort_second_call_number, issue__inherited_first_call_number__raw,
issue__inherited_second_call_number__raw, call_numbers__raw, etc.) to also
filter by pid=item.pid so the counts only consider the target item; locate these
calls around the block that reindexes the item/holding (referencing the item
variable and Item.get_record(item.id)) and add the pid filter to each assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/ui/holdings/test_issues_reindex.py`:
- Around line 125-157: The assertions in test_issues_reindex.py use
ItemsSearch().filter(...) without scoping to the specific item, causing
flakiness; update each ItemsSearch().filter call (including checks for
sort_call_number, sort_second_call_number,
issue__inherited_first_call_number__raw,
issue__inherited_second_call_number__raw, call_numbers__raw, etc.) to also
filter by pid=item.pid so the counts only consider the target item; locate these
calls around the block that reindexes the item/holding (referencing the item
variable and Item.get_record(item.id)) and add the pid filter to each assertion.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9978fe17-a5cf-486d-9dd3-017edc39e150

📥 Commits

Reviewing files that changed from the base of the PR and between 5214d0d and 65f5228.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (5)
  • rero_ils/config.py
  • rero_ils/modules/items/listener.py
  • rero_ils/modules/items/mappings/v7/items/item-v0.0.1.json
  • tests/ui/holdings/test_issues_reindex.py
  • tests/ui/items/test_items_mapping.py

@PascalRepond PascalRepond requested a review from jma March 5, 2026 14:34
@coveralls
Copy link

coveralls commented Mar 5, 2026

Coverage Status

coverage: 92.057% (+0.001%) from 92.056%
when pulling f35f9df on PascalRepond:rep-call-numbers
into 5214d0d on rero:staging.

* Closes rero#4009.
* Updates dependencies.
* ⚠️ Requires a full reindex of items.

Co-Authored-by: Pascal Repond <pascal.repond@rero.ch>
@PascalRepond PascalRepond merged commit eb29bed into rero:staging Mar 12, 2026
5 of 6 checks passed
@PascalRepond PascalRepond deleted the rep-call-numbers branch March 12, 2026 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inventory list: sorting by call_number should also use inherited call_numbers

2 participants