Conversation
…text' type field.
…d parameters mixed with upgrade to ruby 3 caused 'ArgumentError: wrong number of arguments (given 3, expected 2)' during reindexing of DOIS
…ixed keyword and positional parameters which we use in reindexing.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds relationTypeInformation to params sanitizer and DOI index mappings; bumps Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
🤖 Fix all issues with AI agents
In `@spec/requests/datacite_dois/post_spec.rb`:
- Around line 2485-2522: The test payload mixes symbol-style keys (e.g.,
"types":, "relatedItems":, "relatedItemIdentifier":) with string hash-rocket
keys, causing inconsistent key types; update the entries for types,
relatedItems, and relatedItemIdentifier to use the hash-rocket/string-key syntax
(same as the other keys, e.g., "types" => { ... }, "relatedItems" => [ ... ],
"relatedItemIdentifier" => { ... }) so the payload uses consistent string keys
throughout (locate the occurrences of types, relatedItems, and
relatedItemIdentifier in the failing spec and replace the colon-style keys with
=>).
🧹 Nitpick comments (4)
app/lib/params_sanitizer.rb (1)
168-182: Pre-existing duplicate keys inrelatedIdentifiers— consider cleaning up.
relatedMetadataScheme,schemeUri, andschemeTypeeach appear twice in this%ilist (lines 174–176 and 179–181). The newrelationTypeInformationaddition (line 178) is correct, but it was inserted just before these existing duplicates, making them more visible. While duplicate symbols in a permit list are harmless in Rails, removing them would reduce confusion.♻️ Suggested cleanup
{ relatedIdentifiers: %i[ relatedIdentifier relatedIdentifierType relationType relatedMetadataScheme schemeUri schemeType resourceTypeGeneral relationTypeInformation - relatedMetadataScheme - schemeUri - schemeType ], },app/models/doi.rb (1)
303-308: NewrelationTypeInformationfield mappings look correct.Using
type: :textis appropriate for free-form descriptive content. Bothrelated_identifiersandrelated_itemsare consistently updated, aligned with the params sanitizer changes.Operational note: Adding a new field to the ES mapping requires a reindex (or at minimum a mapping update via
PUT /_mapping) on existing indices. Ensure this is part of your deployment plan.spec/requests/datacite_dois/patch_spec.rb (1)
1001-1038: Test context is outside thedescribe "PATCH /dois/:id"block and has a duplicate name.Two issues with this new test block:
Structural placement: The
describe "PATCH /dois/:id"block ends at line 781, so this new context (line 1004) sits as a sibling rather than being nested within it. It still performspatch "/dois/#{doi.doi}", so it logically belongs inside that describe block.Duplicate context name:
"when the record exists"(line 1004) duplicates the context name at line 21, which will produce confusing test output. Consider renaming to something like"when the request uses schema 4.7 - xml patch".♻️ Suggested restructuring
Move the block inside the
describe "PATCH /dois/:id"block (before itsendat line 781) and rename the context:- # Metadata 4.7 - elements - - context "when the record exists" do + context "when the request uses schema 4.7 - xml" do let(:xml) { Base64.strict_encode64(file_fixture("datacite-example-full-v4.7.xml").read) }And relocate this block to be inside
describe "PATCH /dois/:id".spec/requests/datacite_dois/post_spec.rb (1)
2619-2619: Commented-out assertion — consider removing or replacing.This commented-out expectation is presumably stale because the payload now includes multiple
relatedIdentifiers. Either remove it or replace with a working assertion (e.g., checkingdoc.css("relatedIdentifier").map(&:content)).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@spec/requests/datacite_dois/post_spec.rb`:
- Line 2619: Uncomment or document the commented-out XML assertion for
relatedIdentifiers in the spec (the line with
expect(doc.at_css("relatedIdentifiers").content).to eq("10.5438/55e5-t5c0")): if
the XML generation is correct, re-enable the assertion to restore coverage; if
it was disabled because relationTypeInformation/RAiD changes break the expected
output, add a TODO comment explaining the reason, link to the issue/PR that
tracks the fix, and include a TODO tag with when to re-enable so tests
explicitly document the known gap instead of silently reducing coverage.
🧹 Nitpick comments (1)
spec/requests/datacite_dois/post_spec.rb (1)
2430-2475: Schema 4.7 XML test context looks good overall.The test validates key 4.7 elements well: new
resourceTypeGeneralvalues (Poster,Presentation),RAiDidentifier type,relationTypeInformation,SWHIDidentifier type, andrelationType"Other".One concern: the assertions rely on hard-coded array indices (36, 37, 38, 40) into
relatedIdentifiers, which are brittle — any change in the fixture file's ordering or number of entries will silently shift indices and cause false failures or pass incorrectly. Consider asserting viafind/selecton a distinguishing field value, or at minimum adding a comment noting which fixture entry each index corresponds to.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@spec/requests/datacite_dois/datacite_dois_spec.rb`:
- Line 2249: Update the RSpec example description string in the it block named
"finds the dois based on relatedIdentiers.relationTypeInformation" to correct
the typo: change "relatedIdentiers" to "relatedIdentifiers" so the description
reads "finds the dois based on relatedIdentifiers.relationTypeInformation";
locate the it block in spec/requests/datacite_dois/datacite_dois_spec.rb and
edit the string accordingly.
- Around line 2184-2186: Remove the extra blank line at the start of the block
body for the describe "GET /dois/query=...relationTypeInformation" example and
delete the trailing whitespace after the let!(:datacite_doi) line; specifically,
edit the block containing describe "GET /dois/query=...relationTypeInformation"
and the let!(:datacite_doi) { create(:doi, client: client, aasm_state:
"findable", ... } declaration to ensure no empty line immediately after the
block header and no trailing spaces at the end of the let! line.
- Line 2: Remove the debug require by deleting the line `require "pp"` from
spec/requests/datacite_dois/datacite_dois_spec.rb and ensure there is a blank
line after the frozen string literal comment (if present) to satisfy RuboCop's
Layout/EmptyLineAfterMagicComment; no other changes are needed since `pp` is in
the stdlib.
🧹 Nitpick comments (2)
spec/requests/datacite_dois/datacite_dois_spec.rb (2)
2241-2255: Tests only cover the happy path with a single matching DOI — consider adding a non-matching record.Both tests create only one DOI that contains
relationTypeInformation. Thetotal == 1assertion doesn't prove the query actually filters on the new field — it would pass even if the query matched everything. Adding a second DOI withoutrelationTypeInformationwould make the assertions meaningful by confirming the unmatched record is excluded.
2247-2249: Remove extra blank line.There's a superfluous blank line between the two
itblocks (line 2248). Minor nit for consistency with the rest of the file.
jrhoads
left a comment
jrhoads
left a comment
There was a problem hiding this comment.
Overall looks good. Minor issues picked up by coderabbit. Misspelling in the spec description and a left-over pp.
…open_licenses to what it was before and it seems to work, although it is a little funky since it uses the resource_types agg.
3 participants
Purpose
Changes to lupo as required for the Metadata 4.7 release, described in the document at the link, below.
Metadata Schema Release 4.7 - Product Spec
closes: Add github issue that originated this PR
Approach
Open Questions and Pre-Merge TODOs
Learning
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Reviewer, please remember our guidelines:
Summary by CodeRabbit
New Features
Chores
Tests