Conversation
|
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 ISO 19115 correction processing with shared XML helpers, namespace-aware XML editing, scheme-specific correction wiring, real delegate orchestration, tests, and local smoke coverage. ChangesISO 19115 Metadata Correction Pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #115 +/- ##
==========================================
- Coverage 99.81% 99.76% -0.06%
==========================================
Files 223 228 +5
Lines 5516 5875 +359
Branches 1592 1727 +135
==========================================
+ Hits 5506 5861 +355
- Misses 10 13 +3
- Partials 0 1 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
serverless/src/shared/__tests__/Iso19115DomEditor.test.js (1)
54-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the real platform formatter instead of duplicating it locally.
This test reimplements the formatting logic inside the assertion, so a regression in
ISO_19115_SCHEME_EDITORS.platformswould still pass. It would be stronger to capture the config passed toupdateBlockNodeand invokereplace[0].source.getValue(...)directly.🤖 Prompt for 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. In `@serverless/src/shared/__tests__/Iso19115DomEditor.test.js` around lines 54 - 67, The platform formatting test is duplicating the formatter logic locally instead of exercising ISO_19115_SCHEME_EDITORS.platforms, so it can miss regressions. Update the test to capture the config passed into updateBlockNode and call replace[0].source.getValue(...) directly on the real platform formatter output. Use the existing ISO_19115_SCHEME_EDITORS.platforms and updateBlockNode symbols to locate the right assertion path.serverless/src/shared/__tests__/Iso19115MetadataPathEditor.test.js (1)
138-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the XML mutation, not just the boolean result.
This case never inspects the updated
gmd:MD_TopicCategoryCode, so a no-op replace implementation would still pass. Adding a DOM assertion here would also help close the remaining uncovered lines inIso19115MetadataPathEditor.js.Suggested test tightening
const result = editor.updateLeafNode(correction, config) expect(result).toBe(true) - // Verification would depend on the implementation of setElementText + const updatedNode = editor.selectNodes('//gmd:MD_TopicCategoryCode')[0] + expect(updatedNode.textContent).toBe('updated-value')🤖 Prompt for 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. In `@serverless/src/shared/__tests__/Iso19115MetadataPathEditor.test.js` around lines 138 - 155, The test for updateLeafNode currently only checks the boolean return value, so it would pass even if the XML is not actually updated; tighten it by asserting the mutated DOM state after calling Iso19115MetadataPathEditor.updateLeafNode, specifically verifying that the gmd:MD_TopicCategoryCode text content changed as expected via the existing setElementText path.serverless/src/shared/applyIso19115MetadataCorrections.js (1)
2-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the stub-only JSDoc.
This header still describes the pre-implementation behavior even though the function now performs real mutations and returns
stubbed: false.🤖 Prompt for 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. In `@serverless/src/shared/applyIso19115MetadataCorrections.js` around lines 2 - 7, The JSDoc at the top of applyIso19115MetadataCorrections.js still describes stub-only behavior, but the implementation now performs real mutations and returns stubbed: false. Update or remove this header so it accurately documents the current behavior of applyIso19115MetadataCorrections and any related helper symbols in this module, replacing the outdated “delegate/stub/hand off” wording with the actual mutation behavior.serverless/src/shared/__tests__/applyIso19115MetadataCorrections.test.js (1)
7-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExercise the public orchestrator in this suite.
These tests go straight to
ISO_19115_SCHEME_EDITORS, so they do not coverapplyIso19115MetadataCorrections()'s scheme normalization, unsupported-scheme filtering, orcorrectionsAppliedbookkeeping.🤖 Prompt for 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. In `@serverless/src/shared/__tests__/applyIso19115MetadataCorrections.test.js` around lines 7 - 8, The tests are bypassing the public orchestrator, so they miss scheme normalization, unsupported-scheme filtering, and correctionsApplied tracking. Update applyIso19115MetadataCorrections() test coverage to invoke the exported orchestrator directly instead of reaching into ISO_19115_SCHEME_EDITORS, and assert its behavior for normalized schemes, ignored unsupported schemes, and the correctionsApplied flag while still using Iso19115MetadataPathEditor through the public flow.
🤖 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 `@scripts/local/list_collection_concept_ids_by_native_format.mjs`:
- Around line 34-36: The unsupported-format hint is out of sync with the format
aliases added in list_collection_concept_ids_by_native_format.mjs. Update the
error suggestion list in the parsing/validation logic that throws for unknown
formats so it includes the new shortcuts alongside dif10, echo10, and umm, using
the same alias symbols defined in the format map (including iso19115 and
isosmap) to keep the hint accurate.
In `@serverless/src/shared/__tests__/applyIso19115MetadataCorrections.test.js`:
- Around line 512-529: The platform delete test is asserting the wrong
serialized keyword text, so it may not catch a regression in
`ISO_19115_SCHEME_EDITORS.platforms` or
`Iso19115MetadataPathEditor.serialize()`. Update the expectation in `should
delete existing platform keyword block correctly` to match the actual fixture
text for AQUA, and keep the TERRA assertion so the test verifies only the
targeted platform keyword block is removed.
In `@serverless/src/shared/__tests__/metadataCorrectionDelegateStubs.test.js`:
- Around line 181-200: The mock XML fixture is missing the gmi namespace binding
even though the root element uses gmi:MI_Metadata, so update the metadata string
in metadataCorrectionDelegateStubs.test.js to declare xmlns:gmi alongside the
existing gmd and gco namespaces. Keep the change localized to the mockPayload in
this test fixture so the XML is well-formed and parsed consistently.
In `@serverless/src/shared/Iso19115DomEditor.js`:
- Around line 111-139: The productlevelid matcher in createProductLevelIdEditor
only uses the processingLevel XPath, but the delete/replace rules also target
processingLevelCode; update the nodeXPath (or matching logic in updateLeafNode
flow) so it matches every location you later mutate, including the
processingLevelCode MD_Identifier path, and keep the existing sync behavior in
the replace entries aligned with that broader match.
In `@serverless/src/shared/Iso19115MetadataPathEditor.js`:
- Around line 166-173: The selection logic in updateBlockNode currently only
uses the first node returned by this.selectNodes(config.nodeXPath), so
corrections targeting a later MD_Keywords block are missed. Update
updateBlockNode to iterate through all nodes returned by selectNodes and apply
the existing matching/delete/update flow to each candidate until a matching node
is found, rather than stopping at [0].
---
Nitpick comments:
In `@serverless/src/shared/__tests__/applyIso19115MetadataCorrections.test.js`:
- Around line 7-8: The tests are bypassing the public orchestrator, so they miss
scheme normalization, unsupported-scheme filtering, and correctionsApplied
tracking. Update applyIso19115MetadataCorrections() test coverage to invoke the
exported orchestrator directly instead of reaching into
ISO_19115_SCHEME_EDITORS, and assert its behavior for normalized schemes,
ignored unsupported schemes, and the correctionsApplied flag while still using
Iso19115MetadataPathEditor through the public flow.
In `@serverless/src/shared/__tests__/Iso19115DomEditor.test.js`:
- Around line 54-67: The platform formatting test is duplicating the formatter
logic locally instead of exercising ISO_19115_SCHEME_EDITORS.platforms, so it
can miss regressions. Update the test to capture the config passed into
updateBlockNode and call replace[0].source.getValue(...) directly on the real
platform formatter output. Use the existing ISO_19115_SCHEME_EDITORS.platforms
and updateBlockNode symbols to locate the right assertion path.
In `@serverless/src/shared/__tests__/Iso19115MetadataPathEditor.test.js`:
- Around line 138-155: The test for updateLeafNode currently only checks the
boolean return value, so it would pass even if the XML is not actually updated;
tighten it by asserting the mutated DOM state after calling
Iso19115MetadataPathEditor.updateLeafNode, specifically verifying that the
gmd:MD_TopicCategoryCode text content changed as expected via the existing
setElementText path.
In `@serverless/src/shared/applyIso19115MetadataCorrections.js`:
- Around line 2-7: The JSDoc at the top of applyIso19115MetadataCorrections.js
still describes stub-only behavior, but the implementation now performs real
mutations and returns stubbed: false. Update or remove this header so it
accurately documents the current behavior of applyIso19115MetadataCorrections
and any related helper symbols in this module, replacing the outdated
“delegate/stub/hand off” wording with the actual mutation 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: ec44b374-f19a-4de5-9254-3b11203bc329
📒 Files selected for processing (14)
scripts/local/fixtures/metadata_correction_service.iso-19115.corrections.jsonscripts/local/fixtures/metadata_correction_service.iso-19115.example.xmlscripts/local/list_collection_concept_ids_by_native_format.mjsscripts/local/run_metadata_correction_iso-19115_smoke.mjsserverless/src/shared/Iso19115DomEditor.jsserverless/src/shared/Iso19115MetadataPathEditor.jsserverless/src/shared/XmlMetadataPathEditor.jsserverless/src/shared/XmlUtils.jsserverless/src/shared/__tests__/Iso19115DomEditor.test.jsserverless/src/shared/__tests__/Iso19115MetadataPathEditor.test.jsserverless/src/shared/__tests__/XmlUtils.test.jsserverless/src/shared/__tests__/applyIso19115MetadataCorrections.test.jsserverless/src/shared/__tests__/metadataCorrectionDelegateStubs.test.jsserverless/src/shared/applyIso19115MetadataCorrections.js
| umm: 'application/vnd.nasa.cmr.umm+json', | ||
| iso19115: 'application/iso19115+xml', | ||
| isosmap: 'application/iso:smap+xml' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the unsupported-format hint in sync with these aliases.
If a user mistypes one of the new shortcuts, the thrown error still only suggests dif10, echo10, and umm.
Suggested fix
- `Unsupported format "${formatArg}". Use dif10, echo10, umm, or pass an exact MIME type.`
+ `Unsupported format "${formatArg}". Use dif10, echo10, umm, iso19115, isosmap, or pass an exact MIME type.`🤖 Prompt for 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.
In `@scripts/local/list_collection_concept_ids_by_native_format.mjs` around lines
34 - 36, The unsupported-format hint is out of sync with the format aliases
added in list_collection_concept_ids_by_native_format.mjs. Update the error
suggestion list in the parsing/validation logic that throws for unknown formats
so it includes the new shortcuts alongside dif10, echo10, and umm, using the
same alias symbols defined in the format map (including iso19115 and isosmap) to
keep the hint accurate.
| test('should delete existing platform keyword block correctly', () => { | ||
| const editor = new Iso19115MetadataPathEditor(mockIso19115) | ||
| const correction = { | ||
| scheme: 'platforms', | ||
| action: 'delete', | ||
| oldKeywordObject: { ShortName: 'AQUA' } | ||
| } | ||
|
|
||
| const config = ISO_19115_SCHEME_EDITORS.platforms | ||
| const success = config(editor, correction) | ||
|
|
||
| expect(success).toBe(true) | ||
|
|
||
| // Verify the specific keyword is gone | ||
| const updatedXml = editor.serialize() | ||
| expect(updatedXml).not.toContain('AQUA > Earth Observing System, AQUA') | ||
| expect(updatedXml).toContain('TERRA > Earth Observing System, TERRA (AM-1)') | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the platform delete expectation.
Line 527 checks for AQUA > Earth Observing System, AQUA, but the fixture contains AQUA > Earth Observing System. This test can still pass if the delete path regresses.
Suggested fix
- expect(updatedXml).not.toContain('AQUA > Earth Observing System, AQUA')
+ expect(updatedXml).not.toContain('AQUA > Earth Observing System')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('should delete existing platform keyword block correctly', () => { | |
| const editor = new Iso19115MetadataPathEditor(mockIso19115) | |
| const correction = { | |
| scheme: 'platforms', | |
| action: 'delete', | |
| oldKeywordObject: { ShortName: 'AQUA' } | |
| } | |
| const config = ISO_19115_SCHEME_EDITORS.platforms | |
| const success = config(editor, correction) | |
| expect(success).toBe(true) | |
| // Verify the specific keyword is gone | |
| const updatedXml = editor.serialize() | |
| expect(updatedXml).not.toContain('AQUA > Earth Observing System, AQUA') | |
| expect(updatedXml).toContain('TERRA > Earth Observing System, TERRA (AM-1)') | |
| }) | |
| test('should delete existing platform keyword block correctly', () => { | |
| const editor = new Iso19115MetadataPathEditor(mockIso19115) | |
| const correction = { | |
| scheme: 'platforms', | |
| action: 'delete', | |
| oldKeywordObject: { ShortName: 'AQUA' } | |
| } | |
| const config = ISO_19115_SCHEME_EDITORS.platforms | |
| const success = config(editor, correction) | |
| expect(success).toBe(true) | |
| // Verify the specific keyword is gone | |
| const updatedXml = editor.serialize() | |
| expect(updatedXml).not.toContain('AQUA > Earth Observing System') | |
| expect(updatedXml).toContain('TERRA > Earth Observing System, TERRA (AM-1)') | |
| }) |
🤖 Prompt for 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.
In `@serverless/src/shared/__tests__/applyIso19115MetadataCorrections.test.js`
around lines 512 - 529, The platform delete test is asserting the wrong
serialized keyword text, so it may not catch a regression in
`ISO_19115_SCHEME_EDITORS.platforms` or
`Iso19115MetadataPathEditor.serialize()`. Update the expectation in `should
delete existing platform keyword block correctly` to match the actual fixture
text for AQUA, and keep the TERRA assertion so the test verifies only the
targeted platform keyword block is removed.
| const mockPayload = ` | ||
| <gmi:MI_Metadata xmlns:gmd="http://www.isotc211.org/2005/gmd" xmlns:gco="http://www.isotc211.org/2005/gco"> | ||
| <gmd:descriptiveKeywords> | ||
| <gmd:MD_Keywords> | ||
| <gmd:keyword> | ||
| <gco:CharacterString>EARTH SCIENCE > ATMOSPHERE > AEROSOLS</gco:CharacterString> | ||
| </gmd:keyword> | ||
| <gmd:type> | ||
| <gmd:MD_KeywordTypeCode codeList="http://www.isotc211.org/2005/resources/Codelist/gmxCodelists.xml#MD_KeywordTypeCode" codeListValue="theme">theme</gmd:MD_KeywordTypeCode> | ||
| </gmd:type> | ||
| <gmd:thesaurusName> | ||
| <gmd:CI_Citation> | ||
| <gmd:title> | ||
| <gco:CharacterString>NASA / GCMD Science Keywords</gco:CharacterString> | ||
| </gmd:title> | ||
| </gmd:CI_Citation> | ||
| </gmd:thesaurusName> | ||
| </gmd:MD_Keywords> | ||
| </gmd:descriptiveKeywords> | ||
| </gmi:MI_Metadata>` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Declare the gmi namespace in this fixture.
The root element uses gmi: without an xmlns:gmi binding, so this mock XML is malformed and only passes if the parser is lenient.
Suggested fix
- const mockPayload = `
- <gmi:MI_Metadata xmlns:gmd="http://www.isotc211.org/2005/gmd" xmlns:gco="http://www.isotc211.org/2005/gco">
+ const mockPayload = `
+ <gmi:MI_Metadata
+ xmlns:gmi="http://www.isotc211.org/2005/gmi"
+ xmlns:gmd="http://www.isotc211.org/2005/gmd"
+ xmlns:gco="http://www.isotc211.org/2005/gco">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const mockPayload = ` | |
| <gmi:MI_Metadata xmlns:gmd="http://www.isotc211.org/2005/gmd" xmlns:gco="http://www.isotc211.org/2005/gco"> | |
| <gmd:descriptiveKeywords> | |
| <gmd:MD_Keywords> | |
| <gmd:keyword> | |
| <gco:CharacterString>EARTH SCIENCE > ATMOSPHERE > AEROSOLS</gco:CharacterString> | |
| </gmd:keyword> | |
| <gmd:type> | |
| <gmd:MD_KeywordTypeCode codeList="http://www.isotc211.org/2005/resources/Codelist/gmxCodelists.xml#MD_KeywordTypeCode" codeListValue="theme">theme</gmd:MD_KeywordTypeCode> | |
| </gmd:type> | |
| <gmd:thesaurusName> | |
| <gmd:CI_Citation> | |
| <gmd:title> | |
| <gco:CharacterString>NASA / GCMD Science Keywords</gco:CharacterString> | |
| </gmd:title> | |
| </gmd:CI_Citation> | |
| </gmd:thesaurusName> | |
| </gmd:MD_Keywords> | |
| </gmd:descriptiveKeywords> | |
| </gmi:MI_Metadata>` | |
| const mockPayload = ` | |
| <gmi:MI_Metadata | |
| xmlns:gmi="http://www.isotc211.org/2005/gmi" | |
| xmlns:gmd="http://www.isotc211.org/2005/gmd" | |
| xmlns:gco="http://www.isotc211.org/2005/gco"> | |
| <gmd:descriptiveKeywords> | |
| <gmd:MD_Keywords> | |
| <gmd:keyword> | |
| <gco:CharacterString>EARTH SCIENCE > ATMOSPHERE > AEROSOLS</gco:CharacterString> | |
| </gmd:keyword> | |
| <gmd:type> | |
| <gmd:MD_KeywordTypeCode codeList="http://www.isotc211.org/2005/resources/Codelist/gmxCodelists.xml#MD_KeywordTypeCode" codeListValue="theme">theme</gmd:MD_KeywordTypeCode> | |
| </gmd:type> | |
| <gmd:thesaurusName> | |
| <gmd:CI_Citation> | |
| <gmd:title> | |
| <gco:CharacterString>NASA / GCMD Science Keywords</gco:CharacterString> | |
| </gmd:title> | |
| </gmd:CI_Citation> | |
| </gmd:thesaurusName> | |
| </gmd:MD_Keywords> | |
| </gmd:descriptiveKeywords> | |
| </gmi:MI_Metadata>` |
🤖 Prompt for 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.
In `@serverless/src/shared/__tests__/metadataCorrectionDelegateStubs.test.js`
around lines 181 - 200, The mock XML fixture is missing the gmi namespace
binding even though the root element uses gmi:MI_Metadata, so update the
metadata string in metadataCorrectionDelegateStubs.test.js to declare xmlns:gmi
alongside the existing gmd and gco namespaces. Keep the change localized to the
mockPayload in this test fixture so the XML is well-formed and parsed
consistently.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@serverless/src/shared/__tests__/applyIso19115MetadataCorrections.test.js`:
- Line 337: The inline XML fixtures used in
applyIso19115MetadataCorrections.test.js are missing the gmi namespace, so the
metadataPayload values are malformed. Update the affected test fixtures that
build a gmi:MI_Metadata root to include xmlns:gmi alongside the existing
namespaces, keeping the test data representative of valid ISO 19115 input.
Locate the inline XML strings in the test cases around the metadataPayload
entries and add the missing namespace declaration consistently to both payloads.
🪄 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: cb21a078-0c23-4586-a13f-5e8eb761bdec
📒 Files selected for processing (2)
serverless/src/shared/__tests__/Iso19115MetadataPathEditor.test.jsserverless/src/shared/__tests__/applyIso19115MetadataCorrections.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- serverless/src/shared/tests/Iso19115MetadataPathEditor.test.js
| } | ||
| }), | ||
|
|
||
| providers: createKeywordBlock('dataCentre', { |
There was a problem hiding this comment.
The ISO19115 providers mapping looks pointed at the wrong XML structure. Right now Iso19115DomEditor.js (line 175) treats providers as gmd:descriptiveKeywords/gmd:MD_Keywords with codeListValue='dataCentre', but the mapping sheet’s ISO19115 provider rows point to gmd:pointOfContact/gmd:CI_ResponsibleParty and the related distributor/processor/archive responsible-party paths instead. If a record follows the mapped responsible-party structure and does not also carry a dataCentre keyword block, this delegate will skip the provider update entirely for those places.
There was a problem hiding this comment.
Added additional paths.
There was a problem hiding this comment.
So I think this still might be too broad, for example if the incoming provider correction is for ARCHIVER = NSIDC, this global path will update every CI_ResponsibleParty/organisationName with NSIDC, including unrelated roles like distributor. The workbook maps provider updates through specific role-based paths, so I think this should be constrained to the matching responsible-party locations for that provider role rather than every org-name occurrence in the document.
There was a problem hiding this comment.
Since role (example ARCHIVER) is not part of correction object, we find it safer to not touch CI_ResponsibleParty.
| * @returns {boolean} True if the operation was successful. | ||
| */ | ||
| updateBlockNode(correction, config) { | ||
| const targetNode = this.selectNodes(config.nodeXPath)[0] || null |
There was a problem hiding this comment.
It seems like updateBlockNode() only searches the first matching MD_Keywords block because it does this.selectNodes(config.nodeXPath)[0]. If the target keyword lives in a later matching block, thinking we never look at it and the update/delete returns false even though the record contains a valid match. This probably needs to iterate across all matching blocks and stop on the first one that actually contains the keyword.
| fieldPath: ({ node, editor }) => (editor.selectNodes('./gmx:Anchor', node).length > 0 ? 'gmx:Anchor' : 'gco:CharacterString'), | ||
| source: { | ||
| type: 'computed', | ||
| getValue: getValue || (({ correction }) => fieldKeys.map((k) => correction.newKeywordObject[k]).filter(Boolean).join(' > ')) |
There was a problem hiding this comment.
The full-path ISO serializer is collapsing empty hierarchy slots with .filter(Boolean). The mapping html page says on ISO write that if an intermediate keyword is missing but a later keyword exists, we should fill the gap with NONE rather than removing the empty segment.
There was a problem hiding this comment.
Added 'NONE' for missing levels
cgokey
left a comment
cgokey
left a comment
There was a problem hiding this comment.
Only a few comments, overall looks good.
|
|
||
| const context = path.startsWith('//') ? this.document : matchingNode | ||
| const relativePath = path.startsWith('//') ? path : `./${path}`; | ||
| [fieldNode] = this.selectNodes(relativePath, context) |
There was a problem hiding this comment.
Take a closer look at this, think it is only updating the first match here on this destructure.
| // Clean up synchronized paths globally first | ||
| if (config.replace) { | ||
| config.replace | ||
| .filter((replConfig) => typeof replConfig.fieldPath === 'string' && replConfig.fieldPath.startsWith('//')) |
There was a problem hiding this comment.
looks like delete iterates every node returned and removes them all. With the provider config’s //gmd:CI_ResponsibleParty/gmd:organisationName/gco:CharacterString, deleting one matched provider keyword would also remove unrelated responsible-party organization names elsewhere in the record. This seems too broad for provider cleanup and probably needs to be constrained to the mapped ISO target nodes?
There was a problem hiding this comment.
Now constrained to the mapped ISO target nodes.
| } | ||
| ), | ||
|
|
||
| platforms: createKeywordBlock('platform', { |
There was a problem hiding this comment.
The platforms, instruments, and projects mappings currently only update the gmd:descriptiveKeywords/gmd:MD_Keywords text. The mapping workbook calls out additional ISO write targets for these values in the acquisition/identifier structures as well (for example platform rows, instrument row, and project rows). As written, a correction can update the visible keyword block while leaving the corresponding identifier/code/description fields stale, so I think these mappings are only partially implemented right now.
There was a problem hiding this comment.
Now added paths to update the Acquisition nodes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@serverless/src/shared/__tests__/Iso19115MetadataPathEditor.test.js`:
- Around line 402-445: The current test in Iso19115MetadataPathEditor.test.js
only covers duplicate gco:CharacterString nodes within a single gmd:keyword, not
multiple matching keyword blocks. Update the fixture in updateBlockNode to
include two separate matching gmd:keyword elements (or rename the test to
reflect the narrower behavior) so it actually verifies how updateBlockNode
handles multiple matches, especially since the implementation uses .find() in
Iso19115MetadataPathEditor.
🪄 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: 253224b0-2486-41b0-98c7-17ac74c80e0c
📒 Files selected for processing (2)
serverless/src/shared/Iso19115MetadataPathEditor.jsserverless/src/shared/__tests__/Iso19115MetadataPathEditor.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- serverless/src/shared/Iso19115MetadataPathEditor.js
| if (isPlatformOrInstrument && oldShortName && config.replace) { | ||
| // Find all MD_Identifier nodes that match this platform/instrument | ||
| const allIdentifiers = this.selectNodes('//gmd:MD_Identifier', this.document) | ||
| const identifiersToDelete = allIdentifiers.filter((identifier) => { |
There was a problem hiding this comment.
The acquisition cleanup here is matching identifiers by gmd:code text alone, but the ISO mapping is narrower than that. For platform/instrument sync we should also require the mapped gmd:codeSpace value, otherwise an unrelated acquisition identifier with the same short name can be updated or deleted too. Example: if one identifier is the UMM platform short-name field and another different identifier also happens to have code AQUA, this logic will treat both as the same target.
| ] | ||
| }), | ||
|
|
||
| projects: createKeywordBlock('project', { |
There was a problem hiding this comment.
The project mapping has one more addition field to update. This code updates the project keyword block, but it does not also sync the mapped acquisition-side gmi:MI_Operation identifier fields from the workbook. Example: if MEASURES > Old Description becomes MEASURES-1 > New Description, the keyword block changes but MI_Operation can still be left with code MEASURES and the old description.
There was a problem hiding this comment.
Updated, added new paths
| return LongName ? `${ShortName} > ${LongName}` : ShortName | ||
| }, | ||
| // Sync with acquisition information section (gmi:MI_Operation) | ||
| additionalPaths: [ |
There was a problem hiding this comment.
This adds project sync into gmi:MI_Operation on replace, but I don’t see symmetric cleanup for delete in Iso19115MetadataPathEditor. That means we can remove the MD_Keywords project entry while leaving the synced acquisition operation values behind. Can we either add matching delete cleanup?
| node, | ||
| matchingNode: this.findMatchingNode(node, correction, config) | ||
| })) | ||
| .find((data) => data.matchingNode !== null) |
There was a problem hiding this comment.
Should this also guard undefined? findMatchingNode() appears to return undefined on a miss, so !== null may still pass.
3 participants
Overview
What is the feature?
Update Keywords in ISO19115 (ISO-MENDS) native records
What is the Solution?
Update keywords in following schemes as they appear in ISO-MENDS records, according to correction requests:
sciencekeywords
locations
platforms
instruments
projects
providers
isotopiccategory
productlevelid
What areas of the application does this impact?
Consumer - Update Keywords in ISO-MENDS native records
Testing
Run:
The request fixture currently includes the supported UMM-C correction matrix:
sciencekeywordslocationsplatformsinstrumentsprojectsprovidersisotopiccategoryproductlevelidThe script prints a JSON summary like:
The important signals are:
nativeFormatisISO19115requestedCorrectionsmatches the fixtureappliedCorrectionsmatches the expected applied countwriteResult.targetComponentiscmr-writebackThe corrected XML is written to:
See scripts/local/fixtures/metadata_correction_service.iso-19115.corrections.json for requested corrections and compare them with result in tmp/metadata_correction_service_iso-19115_smoke_output.xml
Checklist
Summary by CodeRabbit
iso19115andisosmapfor command-line resolution.