Fix bug where a concept could be mapped to an unknown concept rather … #306
Conversation
…than excluded. Added unit tests.
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in concept resolution where concepts could incorrectly map to unknown concepts instead of being excluded. The fix refactors the StandardConceptResolver class by extracting data access logic into a separate provider and correcting the resolution logic to properly exclude concepts when no valid mapping exists.
Key Changes:
- Introduced
IStandardConceptResolverDataProviderinterface and implementation to separate data access from resolution logic - Fixed concept resolution logic to return empty array instead of unknown concept ID (0) when domain mapping is invalid
- Added comprehensive unit tests covering various concept resolution scenarios
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| OmopTransformer/ConceptResolution/StandardConceptResolver.cs | Fixed return logic to exclude concepts (return early with resolved concepts) and refactored to use data provider interface |
| OmopTransformer/ConceptResolution/IStandardConceptResolverDataProvider.cs | New interface for data provider abstraction |
| OmopTransformer/ConceptResolution/StandardConceptResolverDataProvider.cs | New implementation extracting data access logic from StandardConceptResolver |
| OmopTransformer/ConceptResolution/ConceptCodeMapRow.cs | Extracted data model class previously nested in StandardConceptResolver |
| OmopTransformer/ConceptResolution/ConceptRelationshipRow.cs | Extracted data model class previously nested in StandardConceptResolver |
| OmopTransformerTests/ConceptResolution/StandardConceptResolverTests.cs | Comprehensive unit tests covering various concept resolution scenarios |
| OmopTransformer/Program.cs | Registered new data provider in dependency injection container |
| OmopTransformer/SUS/APC/SusAPCTransformer.cs | Added namespace import and test code in constructor |
| Multiple files | Added namespace imports for ConceptResolution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| return resolvedConcepts; |
There was a problem hiding this comment.
This return statement is inside the foreach loop, causing the method to return after processing only the first row. The return should be outside the foreach loop to process all mappings.
There was a problem hiding this comment.
return resolvedConcepts; is not inside a loop?
There was a problem hiding this comment.
@copilot return resolvedConcepts; is not inside a loop?
|
@james-cockayne I've opened a new pull request, #307, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@james-cockayne I've opened a new pull request, #308, to work on those changes. Once the pull request is ready, I'll request review from you. |
…than excluded. Added unit tests.