Add publish lists and enhance report sharing for RoboInvestor#573
Add publish lists and enhance report sharing for RoboInvestor#573
Conversation
…tion - Updated CreateSecurityRequest and SecurityResponse models to include source_graph_id for better entity association. - Refactored ShareReportRequest to utilize publish_list_id instead of target_graph_ids, streamlining report sharing. - Implemented logic in share_report to resolve publish list members and target graph IDs, ensuring accurate report distribution. - Added tests for publish list handling in report sharing, including scenarios for missing publish lists and validation of existing linked entities.
|
@claude please review this PR |
|
Claude finished @jfrench9's task in 3m 8s —— View job PR Review: Add publish lists and enhance report sharing for RoboInvestor
Good overall direction — the publish list abstraction for report sharing is clean and the multi-tenant wiring looks correct. A few bugs and gaps need attention before merge. 🔴 Critical Bugs1. Incomplete
def downgrade() -> None:
...
op.drop_index("idx_plm_target", table_name="publish_list_members")
op.drop_index("idx_plm_list", table_name="publish_list_members")
op.drop_table("publish_list_members")
op.drop_index("idx_publish_lists_name", table_name="publish_lists")
# ← op.drop_table("publish_lists") IS MISSING
2. Silent partial rollback in
for target_id in body.target_graph_ids:
member = PublishListMember(...)
session.add(member)
try:
session.flush()
added.append(member)
except IntegrityError:
session.rollback() # ← rolls back ALL previously flushed members
raise HTTPException(status_code=409, ...)If you add 3 IDs and the 3rd is a duplicate,
Either: (a) pre-check for duplicates before inserting anything and return a clear error, or (b) use 🟠 Significant Issues3. No ownership authorization on publish list mutations
Unlike 4.
for target_graph_id in target_graph_ids:
result = _share_to_target(...)
if result.status == "shared":
with extensions_session(graph_id) as source_session: # ← N times
share_record = ReportShare(...)
source_session.commit()One session open-commit-close cycle per target graph just to write a ReportShare row. Collect successful results and batch-insert at the end. Fix this → 5. N+1 queries in
for r in rows:
structures = _load_structures(session, r.taxonomy_id) # ← 1 query per report
🟡 Guidelines / Minor Issues6. Migration was manually created — violates CLAUDE.md The revision ID 7.
detail=f"A publish list named '{body.name}' already exists.",
8.
target_graph_ids: list[str] = Field(..., min_length=1) # no max_lengthWithout a cap, a caller can submit thousands of IDs. Consider 9. Missing tests for the entire publish list router
10. Previously ✅ What Looks Good
|
- Added a maximum length constraint for graph IDs in AddMembersRequest to improve data validation. - Implemented authorization checks in update and delete publish list routes to ensure only the creator can modify or delete their lists. - Improved error handling for duplicate entries when adding members to a publish list. - Optimized report listing by caching structure loading to reduce redundant database calls.
1 participant
Summary
Introduces publish list functionality to the RoboLedger extension and enhances report sharing capabilities for the RoboInvestor platform. This feature enables investors to manage curated publish lists and integrates them with existing report and security workflows.
Key Accomplishments
Publish Lists (New Feature)
PublishListdatabase model in the RoboLedger extension with full CRUD support/ledger/publish_lists) with endpoints for creating, reading, updating, and deleting publish listsReport Enhancements
Model & Schema Updates
a1b2c3d4e5f7) introducing publish list tables and security source columnsSecuritymodel in RoboInvestor with a newsourcefieldEntityandInvestorAPI models with additional attributes to support publish list relationships__init__modulesInvestor Securities Refactor
Breaking Changes
Testing Notes
test_reports.py— +84 lines) covering new publish list integration scenariostest_backups.py,test_feature_flags.py) — likely fixture or setup adjustments to accommodate schema changesInfrastructure Considerations
.gitignorerules should be verified to ensure no necessary files are inadvertently excluded🤖 Generated with Claude Code
Branch Info:
feature/roboinvestor-share-reportsmainCo-Authored-By: Claude noreply@anthropic.com