CMR-11240: We no longer exclude concepts that are not in progress when a concept identifier is used in the search query#2405
CMR-11240: We no longer exclude concepts that are not in progress when a concept identifier is used in the search query#2405daniel-zamora wants to merge 3 commits intomasterfrom
Conversation
…de non-operational collections
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 37 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughUpdated non-operational collection filtering so identifier-based searches bypass the non-operational filter. Changes touch API docs, query-parameter parsing (adds identifier detection), test URL helper, and expanded system integration tests covering filter behavior and identifier bypass cases. Removed prior "experimental" wording from docs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
daniel-zamora
changed the title
![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 (2)
system-int-test/test/cmr/system_int_test/search/collection/collection_progress_search_test.clj (2)
79-100: Consider wrapping flag toggle in try-finally for test isolation.If any assertion fails between enabling the flag (line 79) and disabling it (line 100), the flag remains enabled, potentially affecting subsequent tests in the same test run. This applies to both
search-collection-progress-active-filterandsearch-collection-progress-identifier-bypass.♻️ Suggested refactor using try-finally
(testing "flag OFF - param ignored, all collections returned" (util/are3 [items search] (d/refs-match? items (search/find-refs :collection search)) "no params" all-colls {} "include-non-operational=false" all-colls {:include-non-operational "false"} "include-non-operational=true" all-colls {:include-non-operational "true"})) - (side/eval-form `(cmr.search.config/set-enable-non-operational-collection-filter! true)) - - (testing "flag ON - filter applies unless overridden" - (util/are3 [items search] - (d/refs-match? items (search/find-refs :collection search)) - - "no params - only operational collections (excludes PLANNED, DEPRECATED, PREPRINT, INREVIEW)" - active-colls {} - - "include-non-operational=false - only operational collections" - active-colls {:include-non-operational "false"} - - "include-non-operational=true - all collections" - all-colls {:include-non-operational "true"} - - "explicit collection-progress=PLANNED - PLANNED returned (filter not applied)" - [coll2] {:collection-progress "PLANNED"} - - "collection-progress=PLANNED + include-non-operational=false - empty (explicit filter wins)" - [] {:collection-progress "PLANNED" :include-non-operational "false"})) - - (side/eval-form `(cmr.search.config/set-enable-non-operational-collection-filter! false)))) + (try + (side/eval-form `(cmr.search.config/set-enable-non-operational-collection-filter! true)) + + (testing "flag ON - filter applies unless overridden" + (util/are3 [items search] + (d/refs-match? items (search/find-refs :collection search)) + + "no params - only operational collections (excludes PLANNED, DEPRECATED, PREPRINT, INREVIEW)" + active-colls {} + + "include-non-operational=false - only operational collections" + active-colls {:include-non-operational "false"} + + "include-non-operational=true - all collections" + all-colls {:include-non-operational "true"} + + "explicit collection-progress=PLANNED - PLANNED returned (filter not applied)" + [coll2] {:collection-progress "PLANNED"} + + "collection-progress=PLANNED + include-non-operational=false - empty (explicit filter wins)" + [] {:collection-progress "PLANNED" :include-non-operational "false"})) + (finally + (side/eval-form `(cmr.search.config/set-enable-non-operational-collection-filter! false))))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-int-test/test/cmr/system_int_test/search/collection/collection_progress_search_test.clj` around lines 79 - 100, Wrap the flag toggle calls to cmr.search.config/set-enable-non-operational-collection-filter! in a try/finally so the flag is always reset to false even if assertions fail; specifically, around the test block that sets the flag to true (the code invoking (side/eval-form `(cmr.search.config/set-enable-non-operational-collection-filter! true))) put the tests (the util/are3 block for search-collection-progress-active-filter and similarly in search-collection-progress-identifier-bypass) inside the try and call (side/eval-form `(cmr.search.config/set-enable-non-operational-collection-filter! false))) in the finally to guarantee test isolation.
156-157: Native-id correctly defaults to EntryTitle—optional clarity improvement.The test is correct:
coll3is ingested withEntryTitle "ET3", and since no explicit:native-idis set,umm-c-collection->conceptdefaults it to the trimmed EntryTitle via theitem->native-idfunction. The search{:native-id "ET3"}therefore correctly matchescoll3.This assumption is safe and consistent with the test pattern used for entry-title on line 150–151. However, to improve clarity, consider either explicitly setting
:native-id "ET3"in the collection data or retrieving it from the ingested reference (e.g.,(:native-id coll3)), which would make the implicit derivation explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@system-int-test/test/cmr/system_int_test/search/collection/collection_progress_search_test.clj` around lines 156 - 157, Test relies on implicit defaulting of native-id from EntryTitle for coll3 which is correct but unclear; make it explicit by either adding :native-id "ET3" to the collection data used to ingest coll3 or by using the actual native-id from the ingested reference (e.g., refer to (:native-id coll3)) in the search assertion. Update the test where coll3 is defined or where the search vector is built to use the explicit native-id so readers don't need to infer umm-c-collection->concept / item->native-id behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@search-app/docs/api.md`:
- Line 1944: Update the NOTE sentence to use the API's underscore-style query
parameter names instead of hyphens: replace "concept-id", "entry-id",
"entry-title", "short-name" and "native-id" with "concept_id", "entry_id",
"entry_title", "short_name" (with "version" where noted) and "native_id", and
keep the reference to the include-non-operational=true flag and the rule about
searching by collection identifiers unchanged.
---
Nitpick comments:
In
`@system-int-test/test/cmr/system_int_test/search/collection/collection_progress_search_test.clj`:
- Around line 79-100: Wrap the flag toggle calls to
cmr.search.config/set-enable-non-operational-collection-filter! in a try/finally
so the flag is always reset to false even if assertions fail; specifically,
around the test block that sets the flag to true (the code invoking
(side/eval-form
`(cmr.search.config/set-enable-non-operational-collection-filter! true))) put
the tests (the util/are3 block for search-collection-progress-active-filter and
similarly in search-collection-progress-identifier-bypass) inside the try and
call (side/eval-form
`(cmr.search.config/set-enable-non-operational-collection-filter! false))) in
the finally to guarantee test isolation.
- Around line 156-157: Test relies on implicit defaulting of native-id from
EntryTitle for coll3 which is correct but unclear; make it explicit by either
adding :native-id "ET3" to the collection data used to ingest coll3 or by using
the actual native-id from the ingested reference (e.g., refer to (:native-id
coll3)) in the search assertion. Update the test where coll3 is defined or where
the search vector is built to use the explicit native-id so readers don't need
to infer umm-c-collection->concept / item->native-id behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a57a5d0d-f659-46ee-8cfc-64940665e1e6
📒 Files selected for processing (4)
search-app/docs/api.mdsearch-app/src/cmr/search/services/parameters/conversion.cljsystem-int-test/src/cmr/system_int_test/utils/url_helper.cljsystem-int-test/test/cmr/system_int_test/search/collection/collection_progress_search_test.clj
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@search-app/docs/api.md`:
- Line 1944: Update the NOTE to clarify that the identifier-based bypass only
prevents the service from auto-applying the default non-operational collection
filter — it does not override an explicit query parameter; state that when
caller sets include-non-operational=false (or true) explicitly, that explicit
setting still applies even if identifier params (concept_id, entry_id,
entry_title, short_name+version, native_id) are present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2405 +/- ##
===========================================
- Coverage 57.90% 25.46% -32.44%
===========================================
Files 1067 1007 -60
Lines 73454 70014 -3440
Branches 2125 1041 -1084
===========================================
- Hits 42532 17831 -24701
- Misses 28942 51142 +22200
+ Partials 1980 1041 -939 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Overview
What is the objective?
Bypass the non-operational collection filter when searching by collection identifiers (concept-id, entry-id, entry-title, short-name+version, native-id) to fix broken client workflows.
What are the changes?
What areas of the application does this impact?
Required Checklist
Additional Checklist
Summary by CodeRabbit
Documentation
Improvements