Skip to content

Fix SnapTrade cursorbot comments#741

Merged
jjmata merged 1 commit into
we-promise:mainfrom
luckyPipewrench:snaptrade-cursor-comments
Jan 22, 2026
Merged

Fix SnapTrade cursorbot comments#741
jjmata merged 1 commit into
we-promise:mainfrom
luckyPipewrench:snaptrade-cursor-comments

Conversation

@luckyPipewrench

@luckyPipewrench luckyPipewrench commented Jan 22, 2026

Copy link
Copy Markdown
Collaborator
  1. Duplicated SDK object conversion methods
  • Extracted sdk_object_to_hash into SnaptradeAccount::DataHelpers
  • Included DataHelpers in SnaptradeItem::Importer, SnaptradeActivitiesFetchJob, and SnaptradeAccount
  • Removed 3 duplicate implementations
  1. Errors during account linking silently discarded
  • Added partial_success message when some accounts link but others fail
  • Added link_failed message when all accounts fail to link
  • Added i18n keys for new messages

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced account linking feedback with more granular messaging: users now receive detailed notifications showing successful and failed account links separately.
  • Localization

    • Added new localization strings for account linking outcomes including partial success scenarios and error details.

✏️ Tip: You can customize this high-level summary in your review settings.

…ate all references for improved reusability and consistency. Add error handling for partial and failed SnapTrade account linking.
@coderabbitai

coderabbitai Bot commented Jan 22, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

This PR extracts a reusable sdk_object_to_hash helper into a SnaptradeAccount::DataHelpers mixin, which is included across multiple classes to standardize SDK object-to-hash conversion. It also enhances the account linking workflow with granular error messaging and corresponding localization strings.

Changes

Cohort / File(s) Summary
Helper Method Extraction
app/models/snaptrade_account/data_helpers.rb, app/models/snaptrade_account.rb, app/jobs/snaptrade_activities_fetch_job.rb, app/models/snaptrade_item/importer.rb
Created new SnaptradeAccount::DataHelpers mixin with sdk_object_to_hash helper. Mixin included in multiple classes; replaced deep_convert_to_hash calls and removed locally defined duplicate methods. Helper implements JSON serialization fallback with error handling.
Controller Error Handling
app/controllers/snaptrade_items_controller.rb
Enhanced complete_account_setup redirect logic to branch on linking success/failure. Introduced partial_success redirect when some accounts link with errors, and link_failed alert when no accounts link.
Localization
config/locales/views/snaptrade_items/en.yml
Added two new translation keys: partial_success and link_failed for granular account linking feedback.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • jjmata
  • sokie

Poem

🐰 A helper hops from place to place,
Converting SDK objects with grace,
Mixins unite the common task,
While messages reveal what was asked,
Linking accounts with feedback so clear!

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title check ⚠️ Warning The pull request title 'Fix SnapTrade cursorbot comments' is unrelated to the actual changes. The PR removes duplicate code, extracts a helper module, and improves account-linking error handling—none of which involve fixing 'cursorbot comments'. Revise the title to accurately reflect the main changes, such as 'Centralize SDK object conversion and improve account-linking error handling' or similar.
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@jjmata jjmata merged commit 9858b36 into we-promise:main Jan 22, 2026
6 checks passed
@luckyPipewrench luckyPipewrench deleted the snaptrade-cursor-comments branch January 22, 2026 21:27
@coderabbitai coderabbitai Bot mentioned this pull request Jan 22, 2026
binarygituser referenced this pull request in binarygituser/sure-dev Apr 26, 2026
…ate all references for improved reusability and consistency. Add error handling for partial and failed SnapTrade account linking. (maybe-finance#741)

Co-authored-by: luckyPipewrench <luckypipewrench@proton.me>
jaxver pushed a commit to jaxver/sure that referenced this pull request May 31, 2026
…ate all references for improved reusability and consistency. Add error handling for partial and failed SnapTrade account linking. (we-promise#741)

Co-authored-by: luckyPipewrench <luckypipewrench@proton.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

2 participants