feat(investments): add India investment subtypes and exchange support#1659
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 India-specific investment subtypes and property subtype entries and translations; updates Investment region ordering; and enhances Provider::YahooFinance with symbol normalization, INR/default-currency resolution, and dual-listing deduplication. Tests and locale entries updated accordingly. ChangesIndia Investment & Property Support
Yahoo Finance Indian Exchange Support
sequenceDiagram
participant Client as rgba(63,81,181,0.5) Client
participant Provider as rgba(0,150,136,0.5) Provider::YahooFinance
participant Yahoo as rgba(255,152,0,0.5) Yahoo API
participant Cache as rgba(233,30,99,0.5) Cache/DB
Client->>Provider: search_securities(query, exchange_mic)
Provider->>Provider: map_exchange_mic(exchange_mic)
Provider->>Provider: normalize_symbol(symbol, mapped_mic)
Provider->>Yahoo: fetch /chart or /quoteSummary with normalized symbol
Yahoo-->>Provider: chart/quote response (includes meta.exchange, meta.currency)
Provider->>Provider: default_currency_for_exchange(chart_meta) / set currency
Provider->>Provider: deduplicate_dual_listings(securities)
Provider->>Cache: cache results
Provider-->>Client: return normalized, deduplicated securities (with currency)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: Turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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 |
superagent-security
Bot
added
the
contributor:flagged
|
| Dimension | Score | What it measures |
|---|---|---|
| Identity | 30 | Account age, contribution history, GPG keys, org memberships |
| Behavior | 80 | PR patterns, unsolicited contribution ratio, activity cadence |
| Content | 100 | PR body substance, issue linkage, contribution quality |
| Graph | 30 | Cross-repo trust, co-contributor relationships |
Analyzed by Brin · Full profile
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 694e879f44
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/models/provider/yahoo_finance_test.rb (1)
412-432: ⚡ Quick winAdd a regression test for preserving distinct Indian listings.
Current coverage verifies NSE is preferred over BSE for one pair, but it doesn’t protect against collapsing all Indian securities into one result.
🧪 Suggested test addition
+ test "prefer_indian_exchange preserves distinct Indian securities while preferring NSE over BSE duplicates" do + rel_nse = Provider::SecurityConcept::Security.new(symbol: "RELIANCE.NS", name: "Reliance", exchange_operating_mic: "XNSE") + rel_bse = Provider::SecurityConcept::Security.new(symbol: "500325.BO", name: "Reliance", exchange_operating_mic: "XBOM") + infy_nse = Provider::SecurityConcept::Security.new(symbol: "INFY.NS", name: "Infosys", exchange_operating_mic: "XNSE") + + result = `@provider.send`(:prefer_indian_exchange, [ rel_nse, rel_bse, infy_nse ]) + + assert result.map(&:symbol).include?("RELIANCE.NS") + assert_not result.map(&:symbol).include?("500325.BO") + assert result.map(&:symbol).include?("INFY.NS") + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/provider/yahoo_finance_test.rb` around lines 412 - 432, Add a regression test in test/models/provider/yahoo_finance_test.rb that verifies prefer_indian_exchange does not collapse distinct Indian listings: create two different Provider::SecurityConcept::Security instances (different symbol/name) where one is on XNSE and the other on XBOM, call `@provider.send`(:prefer_indian_exchange, ...) and assert both Indian listings remain in the result (i.e., both XNSE and XBOM are present) while still preferring NSE only when duplicates for the same security exist; reference the prefer_indian_exchange method and Provider::SecurityConcept::Security to locate where to add the test.app/models/investment.rb (1)
57-84: 🏗️ Heavy liftSubtype display labels are hard-coded in the model, which can drift from locale translations.
These new entries embed user-facing
short/longstrings directly inInvestment::SUBTYPES. Since translation keys were also added, this creates duplicate sources of truth and weakens localization consistency.Consider storing only non-display metadata here (e.g.,
region,tax_treatment) and resolving labels via I18n at render/select-build time.As per coding guidelines
**/*.{erb,rb}: “Always uset()helper for user-facing strings… Updateconfig/locales/en.ymlfor new strings.”Also applies to: 90-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/investment.rb` around lines 57 - 84, Investment::SUBTYPES currently embeds user-facing "short"/"long" strings (e.g., entries like "nps" => { short: "NPS", long: "National Pension System", ... }) which duplicates locale keys; remove the hard-coded display labels from Investment::SUBTYPES so it only holds stable metadata (region, tax_treatment) and leave keys like "nps", "apy", "demat" etc. as identifiers; update codepaths that render or build select options (where SUBTYPES is read) to call I18n.t with the corresponding translation keys (use the existing locale keys in config/locales/*.yml) to obtain short/long labels at render time; ensure tests and any usages of methods that reference SUBTYPES (e.g., any code referencing Investment::SUBTYPES[...] .fetch(:short) or .fetch(:long)) are updated to fetch labels via I18n.t("investments.subtypes.<key>.short") / .long instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/provider/yahoo_finance.rb`:
- Around line 371-377: The current prefer_indian_exchange method removes all but
one Indian listing by selecting a single preferred Indian security; instead,
change it to perform deduplication only within likely-duplicate groups: in
prefer_indian_exchange, group the securities by a stable identifier for
duplicates (e.g. s.ticker or [s.ticker, s.name] using group_by), and for each
group pick the preferred entry by ranking exchange_operating_mic with
INDIAN_EXCHANGE_PREFERENCE (using min_by) while preserving non-duplicate groups
intact; return the flattened array of selected entries in original group order
so unrelated Indian listings are not dropped (update variables
indian/preferred/non_indian logic to operate per-group rather than globally).
In `@test/models/investment_test.rb`:
- Around line 161-166: The test "India subtypes all belong to the 'in' region"
currently checks a hardcoded subset; change it to derive the keys from
Investment::SUBTYPES itself so all current and future India subtypes are
covered: compute india_keys = Investment::SUBTYPES.keys.select { |k|
Investment::SUBTYPES.dig(k, :region) == 'in' } (referenced symbol:
Investment::SUBTYPES) and then iterate that list asserting each entry's region
equals "in" in the test method "India subtypes all belong to the 'in' region".
---
Nitpick comments:
In `@app/models/investment.rb`:
- Around line 57-84: Investment::SUBTYPES currently embeds user-facing
"short"/"long" strings (e.g., entries like "nps" => { short: "NPS", long:
"National Pension System", ... }) which duplicates locale keys; remove the
hard-coded display labels from Investment::SUBTYPES so it only holds stable
metadata (region, tax_treatment) and leave keys like "nps", "apy", "demat" etc.
as identifiers; update codepaths that render or build select options (where
SUBTYPES is read) to call I18n.t with the corresponding translation keys (use
the existing locale keys in config/locales/*.yml) to obtain short/long labels at
render time; ensure tests and any usages of methods that reference SUBTYPES
(e.g., any code referencing Investment::SUBTYPES[...] .fetch(:short) or
.fetch(:long)) are updated to fetch labels via
I18n.t("investments.subtypes.<key>.short") / .long instead.
In `@test/models/provider/yahoo_finance_test.rb`:
- Around line 412-432: Add a regression test in
test/models/provider/yahoo_finance_test.rb that verifies prefer_indian_exchange
does not collapse distinct Indian listings: create two different
Provider::SecurityConcept::Security instances (different symbol/name) where one
is on XNSE and the other on XBOM, call `@provider.send`(:prefer_indian_exchange,
...) and assert both Indian listings remain in the result (i.e., both XNSE and
XBOM are present) while still preferring NSE only when duplicates for the same
security exist; reference the prefer_indian_exchange method and
Provider::SecurityConcept::Security to locate where to add the test.
🪄 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: 87e86b79-a103-4fab-a052-07ffa25f5332
📥 Commits
Reviewing files that changed from the base of the PR and between a8425a2 and 694e879f440633f47e5c5177b9e3f6f215008414.
📒 Files selected for processing (8)
app/models/investment.rbapp/models/property.rbapp/models/provider/yahoo_finance.rbconfig/locales/views/accounts/en.ymlconfig/locales/views/investments/en.ymlconfig/locales/views/properties/en.ymltest/models/investment_test.rbtest/models/provider/yahoo_finance_test.rb
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d598507301
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jjmata
removed
the
contributor:flagged
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Thanks for taking another crack at this, @ahnv!
I do see great progress, but still functions like prefer_indian_exchange and normalize_indian_symbol show that this is too specific.
What would we do, have a function like that for each country? Those code paths (and others, if they exist!) need to be generalized as well. 🙏
Thanks for the feedback @jjmata — totally agree these shouldn't be India-specific. My plan is to replace all the India-specific constants and methods with a single data-driven EXCHANGE_CONFIG mapping keyed by MIC (Market Identifier Code): This lets me collapse the India-specific code paths into three generic methods:
Adding a new market (e.g. LSE/XLON) becomes a one-line hash entry instead of a new method. Does this direction work for you, or would you prefer a different structure? |
superagent-security
Bot
added
the
contributor:flagged
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/models/provider/yahoo_finance.rb (1)
340-345: Remove dead code:YAHOO_EXCHANGE_CURRENCYconstant is never referenced.The constant at lines 343–345 is defined but never used. The
default_currency_for_exchangemethod (line 379) retrieves currency directly viaEXCHANGE_CONFIG.dig(mic, :default_currency), not from this constant. Additionally, the comment is inaccurate: the iteration is overEXCHANGE_CONFIGkeys (MIC codes like"XNSE"), not raw Yahoo codes, andmap_exchange_micis never invoked during construction.Proposed fix — remove the unused constant and misleading comment
- # Reverse lookup: Yahoo exchange name (from chart metadata) → default currency. - # Built from EXCHANGE_CONFIG via map_exchange_mic so the raw Yahoo codes - # ("NSE", "BSE", etc.) resolve to the same config. - YAHOO_EXCHANGE_CURRENCY = EXCHANGE_CONFIG.each_with_object({}) do |(mic, cfg), h| - h[mic] = cfg[:default_currency] - end.freeze -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/provider/yahoo_finance.rb` around lines 340 - 345, Remove the dead constant YAHOO_EXCHANGE_CURRENCY and its accompanying misleading comment: delete the YAHOO_EXCHANGE_CURRENCY constant definition and the comment above it, since default_currency_for_exchange already reads directly from EXCHANGE_CONFIG (via EXCHANGE_CONFIG.dig(mic, :default_currency)) and map_exchange_mic is not used here; leave EXCHANGE_CONFIG and the method default_currency_for_exchange as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/provider/yahoo_finance.rb`:
- Around line 390-394: The grouping key in deduplicate_dual_listings uses s.name
verbatim so listings with different casing/whitespace (e.g., "Reliance
Industries Ltd" vs "RELIANCE INDUSTRIES LTD") are treated as distinct; update
the group_by block key to normalize the name (call to_s.strip.downcase on
s.name) so the grouping uses
[EXCHANGE_CONFIG[s.exchange_operating_mic][:dual_list_group],
s.name.to_s.strip.downcase], ensuring case- and whitespace-insensitive duplicate
detection while leaving EXCHANGE_CONFIG and preference_rank logic unchanged.
---
Nitpick comments:
In `@app/models/provider/yahoo_finance.rb`:
- Around line 340-345: Remove the dead constant YAHOO_EXCHANGE_CURRENCY and its
accompanying misleading comment: delete the YAHOO_EXCHANGE_CURRENCY constant
definition and the comment above it, since default_currency_for_exchange already
reads directly from EXCHANGE_CONFIG (via EXCHANGE_CONFIG.dig(mic,
:default_currency)) and map_exchange_mic is not used here; leave EXCHANGE_CONFIG
and the method default_currency_for_exchange as-is.
🪄 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: ceb69f6b-8f7b-4201-a72a-a954e64786df
📥 Commits
Reviewing files that changed from the base of the PR and between d598507301c719ff3e187d8151d2f037c218ce8e and 20727be9d6cc940bb386bd179266337c43643c2d.
📒 Files selected for processing (2)
app/models/provider/yahoo_finance.rbtest/models/provider/yahoo_finance_test.rb
✅ Files skipped from review due to trivial changes (1)
- test/models/provider/yahoo_finance_test.rb
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/provider/yahoo_finance.rb`:
- Line 168: The deduplication step currently calls
deduplicate_dual_listings(securities) unconditionally which both loses Yahoo’s
original relevance ordering and can cause exchange_scoped searches (where params
include exchange_operating_mic) to return the other market’s listing; change the
logic so that you skip calling deduplicate_dual_listings when an
exchange_operating_mic filter is present, and when you must deduplicate preserve
stability by maintaining Yahoo's original securities order rather than
rebuilding as preferred + others (avoid the preferred + others reorder in the
block currently around lines 386-400); locate the calls to
deduplicate_dual_listings and the code that constructs preferred + others and
modify them to either no-op for exchange-scoped lookups or to perform a stable
in-place dedupe that keeps the original array order and only removes duplicates
while marking a preferred item.
🪄 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: 76d5b337-aafc-4524-b434-cc5ae107b4c3
📥 Commits
Reviewing files that changed from the base of the PR and between 20727be9d6cc940bb386bd179266337c43643c2d and f27c31e643d6027ece0aebd4a7f8a54ee8729a49.
📒 Files selected for processing (1)
app/models/provider/yahoo_finance.rb
|
Updated per feedback — the India-specific methods (normalize_indian_symbol, prefer_indian_exchange,
No more country-specific functions. Adding a new market (e.g. LSE, Tokyo) is a one-line hash entry in |
jjmata
removed
contributor:flagged
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Review
This is a well-structured PR that correctly follows the project's subtype pattern (no new tables, no new accountable models) and addresses the prior feedback from #1413. The EXCHANGE_CONFIG-driven approach in Provider::YahooFinance is genuinely extensible. A few issues worth addressing before merging:
Bug: cache key mismatch in fetch_security_price
fetch_security_price builds its cache key from the pre-normalization symbol, but stores a Price object whose .symbol field is the post-normalization value (e.g. "RELIANCE" → "RELIANCE.NS"). On a cache hit the caller gets back a Price with symbol: "RELIANCE.NS" even though they passed "RELIANCE". Downstream code that relies on the returned symbol matching the input will silently see a mismatch.
# fetch_security_price (before normalization happens)
cache_key = "security_price_#{symbol}_#{exchange_operating_mic}_#{date}"
# ...
prices_response = fetch_security_prices(symbol: symbol, ...) # normalizes inside
# target_price.symbol == "RELIANCE.NS", cached under key "RELIANCE"The simplest fix is to normalize the symbol at the top of fetch_security_price as well, so the cache key and the stored object agree.
Dead code: YAHOO_EXCHANGE_CURRENCY
The constant is built but never referenced anywhere:
YAHOO_EXCHANGE_CURRENCY = EXCHANGE_CONFIG.each_with_object({}) do |(mic, cfg), h|
h[mic] = cfg[:default_currency]
end.freezedefault_currency_for_exchange goes straight through map_exchange_mic → EXCHANGE_CONFIG.dig(mic, :default_currency) and doesn't use this reverse-lookup table. Either wire it in or remove it.
normalize_symbol guard is too broad for dotted symbols on Indian exchanges
def normalize_symbol(symbol, exchange_operating_mic)
return symbol if symbol.include?(".") # ← skips suffix if symbol already has any dot
...
endA symbol like BRK.A listed on XNSE (contrived, but the logic is general) would silently skip the .NS suffix. A tighter guard — e.g. checking for the specific configured suffix — is safer:
suffix = EXCHANGE_CONFIG.dig(exchange_operating_mic, :yahoo_suffix)
return symbol if suffix.nil? || symbol.end_with?(suffix)
"#{symbol}#{suffix}"Property subtypes are region-agnostic; the # India comment is misleading
Property::SUBTYPES has no region: key, so every subtype is shown to all users globally. apartment and commercial are legitimate property types everywhere (US, UK, EU…), not India-specific. The # India comment in property.rb incorrectly implies these are scoped to Indian users. Consider either removing the comment or — if you want these to be India-only — opening a follow-up to add region support to Property::SUBTYPES the same way Investment::SUBTYPES has it.
rented is a use-case descriptor, not a property type
"rented" describes what the owner does with a property, not what kind of property it is. The existing investment_property covers the rental-income use case. Consider dropping rented in favour of strengthening investment_property, or renaming it to something like rental_property that aligns with the naming convention of the other entries.
demat as an investment subtype is conceptually imprecise
A Demat account is a container that holds securities (stocks, ETFs, bonds), not an investment product in itself. A user who opens one won't know what balance to enter. Indian users tracking NSE/BSE equities would typically use indian_equity for individual stock positions; the demat entry adds ambiguity. It could be removed or renamed (e.g. indian_stocks) to reduce confusion.
Test asserts India comes last for INR users — is that intentional UX?
test "subtypes_grouped_for_select places India region last" do
grouped = Investment.subtypes_grouped_for_select(currency: "INR")
last_group_label = grouped.last[0]
assert_equal I18n.t("accounts.subtype_regions.in"), last_group_label
endBecause CURRENCY_REGION_MAP deliberately omits "INR", user_region resolves to nil for INR users and India appears after Generic in the dropdown. The test is passing the correct, intentional behavior — but it means Indian users scroll past all other regions to find their instruments. The PR description explains the tradeoff, but it's worth a brief comment in the code (or a TODO) to make this discoverable for the next contributor who wonders why INR is absent from CURRENCY_REGION_MAP.
Minor: Set may need an explicit require
deduplicate_dual_listings calls Set.new. In Ruby 3.2+ Set is no longer auto-loaded, and Rails 7 no longer auto-requires it via Active Support. The rest of the codebase may already require 'set' somewhere in the load path, but it's worth verifying a require 'set' (or require "set") is present — either at the top of yahoo_finance.rb or in an initializer — to avoid a NameError in production environments where the constant hasn't been loaded yet.
Generated by Claude Code
superagent-security
Bot
added
the
contributor:flagged
…instead of globally Resolves feedback from Codex and CodeRabbit on #1413. prefer_indian_exchange previously collapsed all Indian securities into a single entry, silently dropping unrelated tickers. Now groups Indian listings by name and only de-duplicates within each group, so distinct companies (e.g. Reliance and Infosys) are preserved while NSE/BSE dual-listings still prefer NSE. - Derive India subtype keys dynamically from Investment::SUBTYPES in tests - Fix missing keyword arguments in Security.new test calls
…de-duplication Replaces hardcoded Indian exchange logic with a declarative EXCHANGE_CONFIG hash that maps ISO MIC codes to Yahoo-specific settings (symbol suffix, default currency, dual-listing group, and preference rank). This makes adding new markets a one-line hash entry instead of scattered conditionals.
- fix cache key mismatch in fetch_security_price by normalizing symbol before building cache key
- remove dead YAHOO_EXCHANGE_CURRENCY constant
- tighten normalize_symbol guard to use end_with?(suffix) instead of include?('.')
- remove misleading '# India' comment from Property::SUBTYPES
- remove 'rented' property subtype in favor of 'investment_property'
- rename 'demat' to 'indian_stocks' for clarity
- add INR to CURRENCY_REGION_MAP so India appears first for INR users
- add dotted-symbol regression test for normalize_symbol
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Follow-up review
Good progress — almost everything from my first pass has been addressed cleanly. One real bug remains.
Still open: locale key mismatch for indian_stocks
The latest commit renamed the model key from "demat" to "indian_stocks", but the locale file was not updated to match. The result:
config/locales/views/investments/en.ymladds ademat:key that no longer corresponds to any subtype (orphaned).indian_stockshas no locale entry at all — it will fall back to the raw key or raise a missing-translation error in development.
The locale block should read:
indian_stocks:
short: Indian Stocks
long: Indian Stocks (Demat)instead of the current demat: entry.
Everything else resolved
For completeness, confirming each earlier point is now closed:
| Issue | Status |
|---|---|
Cache key mismatch in fetch_security_price |
✅ Fixed — normalize_symbol now runs before the cache key is built |
Dead YAHOO_EXCHANGE_CURRENCY constant |
✅ Removed |
normalize_symbol guard too broad (include?(".")) |
✅ Fixed — now uses end_with?(suffix), test added for BRK.A case |
# India comment misleading in property.rb |
✅ Removed |
rented as use-case descriptor |
✅ Removed |
demat as container-account subtype |
✅ Renamed to indian_stocks in the model |
| INR users see India last in dropdown | ✅ Fixed — "INR" => "in" added to CURRENCY_REGION_MAP, test updated |
Set missing explicit require |
✅ Fixed — require "set" added at top of file |
Just fix the locale key and this is clean.
Generated by Claude Code
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
jjmata
removed
the
contributor:flagged
github-project-automation
Bot
moved this from Todo
to Done
in Latest release train (v0.7.1) - May 2026
2 participants
Adds first-class support for Indian investors by extending the existing Investment and Property accountable types with India-region subtypes, and by teaching
Provider::YahooFinanceto handle NSE/BSE listings. Indian mutual fund NAVs are already covered by the existing [Provider::Mfapi(https://github.com/we-promise/sure/blob/694e879f440633f47e5c5177b9e3f6f215008414/app/models/provider/mfapi.rb); this PR builds on top of that.Addressing the prior review on #1413
Maintainer feedback on that PR from @jjmata:
This PR is designed to resolve exactly that. No country-specific tables, no new accountable types — Indian investments live on the existing Investment model via subtypes (same pattern already used for
401k,isa,rrsp,super,pea, etc.), and Indian property types live on Property. Zero new migrations, zero schema changes.Changes
Investment subtypes (
SUBTYPEShash, India section)Region
in:nps,apy,life_insurancedemat,indian_equity,indian_etfppf,ssy,nsc,scss,fd,rd,pomis,kvpg_sec,sdl,corporate_bond,infrastructure_bond,tax_free_bondgold_etf,gold_mf,sgbGeneric:
gold(covers physical + digital gold, region-agnostic)Each subtype carries a proper tax_treatment (
:tax_exemptfor PPF/SSY/tax-free bonds,:tax_advantagedfor NPS/SGB/infra bonds/life insurance,:taxableotherwise).Property subtypes (
SUBTYPEShash)apartment,plot,commercial,rented,agri_landSubtype ordering
Provider::YahooFinance— NSE/BSE supportNSE/NSI→XNSE;BSE/BOM→XBOM— seemap_exchange_micIN(viamap_country_code).NS/.BOsuffix in fetch_security_info and fetch_security_prices vianormalize_indian_symbolprefer_indian_exchangeprefers NSE over BSE when the same ticker is listed on bothmeta.currencyfor Indian exchangesCURRENCY_REGION_MAPintentionally leaves"INR"unlisted — India coverage still works via the explicitregion: "in"tag on subtypes, without displacing the existing currency-based region prioritizationTests
test/models/provider/yahoo_finance_test.rb— MIC mapping, symbol normalization, INR default, NSE preference over BSE for dual-listed tickersAll existing tests continue to pass.
Screenshots
Checklist
bin/rubocopcleannpm run lint) cleanmainSummary by CodeRabbit