fix(data-layer): DL-25 followup - Add story validation test - #101
Merged
Conversation
github-actions
Bot
added
area/data-layer
There was a problem hiding this comment.
Pull request overview
This PR contains follow-up fixes to DL-25 after the initial merge, addressing Neo4j API usage and adding missing test coverage for story validation.
- Refactored
mongodb_create_combatto use theNeo4jClient.execute_read()method instead of direct session management - Added test coverage for combat creation when story doesn't exist in Neo4j
- Updated existing test mocks to align with the new Neo4j client API
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/data-layer/src/monitor_data/tools/mongodb_tools.py |
Refactored story existence validation in combat creation to use execute_read() API consistently with other validation queries in the codebase |
packages/data-layer/tests/test_tools/test_combat_tools.py |
Simplified Neo4j mocking in success test and added new test case for story validation error handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Scene exists but story does not | ||
| mock_scenes.find_one.return_value = {"scene_id": str(scene_id)} | ||
| mock_get_neo4j.return_value = MagicMock(execute_read=MagicMock(return_value=[])) |
There was a problem hiding this comment.
The Neo4j mock setup should be consistent with the pattern used in test_create_combat_success. Instead of creating a nested MagicMock inline, create separate mock objects for better clarity and consistency within the test file. The current approach with MagicMock(execute_read=MagicMock(return_value=[])) is harder to debug and less explicit than the pattern used elsewhere in the file.
Suggested change
| mock_get_neo4j.return_value = MagicMock(execute_read=MagicMock(return_value=[])) | |
| mock_neo4j_client = MagicMock() | |
| mock_neo4j_client.execute_read = MagicMock(return_value=[]) | |
| mock_get_neo4j.return_value = mock_neo4j_client |
spuentesp
added a commit
that referenced
this pull request
Jan 5, 2026
* Fix combat creation Neo4j validation * Add combat story validation test --------- Co-authored-by: spuentesp <spuentesp@gmail.com>
spuentesp
added a commit
that referenced
this pull request
Jan 5, 2026
* Fix combat creation Neo4j validation * Add combat story validation test --------- Co-authored-by: spuentesp <spuentesp@gmail.com>
spuentesp
added a commit
that referenced
this pull request
Jan 5, 2026
* fix(data-layer): DL-25 followup - Add story validation test (#101) * Fix combat creation Neo4j validation * Add combat story validation test --------- Co-authored-by: spuentesp <spuentesp@gmail.com> * fix(data-layer): Address Copilot review comments for DL-24 Addressed both code review comments from Copilot: 1. **P1: Make magnitude optional** - Added default=0 to Effect.magnitude field since numeric magnitude doesn't apply to all effect types (e.g., CONDITION for "grappled", POSITION_CHANGE for movement) 2. **P2: Add default to natural field** - Added default=0 to RollResult.natural for consistency with other optional fields like critical and fumble These changes improve schema flexibility and consistency. All 256 tests still passing ✅ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * fix(data-layer): Format with black 25.12.0 Added trailing comma to function parameter to match black 25.12.0 formatting rules used in CI. This is a formatting-only change with no functional impact. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> --------- Co-authored-by: spuentesp <spuentesp@gmail.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up fixes to DL-25 after initial merge:
Fix combat creation Neo4j validation (d1c36cb)
mongodb_create_combatto useneo4j_client.execute_read()instead of raw session accessAdd combat story validation test (8caf6e3)
test_create_combat_story_not_foundto verify error handling when story doesn't existThese commits were added to the feature branch after PR #99 was merged.
Testing
All tests passing ✅
🤖 Generated with Claude Code
Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com