Repository navigation
feat(hangar): buy-back links, price, insurance and availability - #5473
Conversation
…surance The list sync takes each pledge's availability and answers with the pledges that have no details yet; a second endpoint stores the price and insurance read from RSI for them.
Reads the RSI buy-back page of each pledge the sync has no details for, and RSI's upgrade price for each upgrade, through the sync extension. Details are stored as they arrive, so a pass that stops keeps its progress; three unreadable pages in a row stop it.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (53)
📝 WalkthroughWalkthroughBuy-back sync now records availability and identifies pledges that need detail reads. The frontend reads RSI detail pages and submits price and insurance data through a new API endpoint. The buy-back list displays those details and links available pledges to RSI. ChangesBuy-back data and availability
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant BuybackSyncModal
participant useBuybackDetailsSync
participant FleetyardsSyncHandler
participant RSIBuybackDetailParser
participant HangarBuybacksController
participant StoreDetails
BuybackSyncModal->>useBuybackDetailsSync: run pending detail pass
useBuybackDetailsSync->>FleetyardsSyncHandler: request syncBuybackDetail for pledge ID
FleetyardsSyncHandler-->>useBuybackDetailsSync: return detail-page response
useBuybackDetailsSync->>RSIBuybackDetailParser: parse response HTML
RSIBuybackDetailParser-->>useBuybackDetailsSync: return price and insurance details
useBuybackDetailsSync->>HangarBuybacksController: submit detail records
HangarBuybacksController->>StoreDetails: update matching user pledges
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
…wers and gone pledges A late answer to a timed-out request no longer answers the next one, a pledge page without a price or one RSI answers 404 for is stored rather than read on every sync, and a failed submit or a cancelled pass keeps what was read.
…rest once An upgrade costs what the upgrade costs today, the difference between both ships' store prices, so it is computed on read instead of asking RSI. Every other buy-back costs what it was bought for, so its details are read once and never refreshed.
A price in the account's currency with tax included cannot be compared with anything else FleetYards shows, so only RSI's own USD figure is stored and served.
A buy-back page prices in the account's currency with tax. The detail pass reads the account's store pricing first and divides the page price back into USD, refusing any page whose currency the pricing does not cover.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| # Missing from a list read before availability was, so it says nothing. | ||
| available: ActiveModel::Type::Boolean.new.cast(item[:available]) != false, |
There was a problem hiding this comment.
🟡 Older extensions restore unavailable buy-back links
When an older extension omits available, Sync#row replaces a stored false with true. The pledge then shows a buy-back link despite its last known unavailable status.
Learn more
The list sync uses upsert_all to overwrite fields supplied in each snapshot, while older extensions do not send available. The newly added fallback turns that absence into true, even for a pledge already stored as unavailable. The next sync thus reverses a known unavailable state without any new availability evidence.
Example: A newer extension stores pledge 8 with available: false. The user later syncs with an older extension whose pledge 8 has no available field; the stored pledge changes to available and the UI displays its RSI buy-back action.
Recommended fix: Preserve the existing value when available is omitted from a snapshot. Default to true only for new rows without a stored value, while still accepting explicit false and true.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed. Fixed in a follow-up commit: a row without available is upserted separately without that column, so a stored false stays and a new pledge takes the column default. The push was refused because this PR is already in the merge queue, so it lands as its own PR. 🤖
| user.buyback_pledges.where(rsi_pledge_id: item[:id].to_s).update_all( | ||
| price: item[:price], | ||
| insurance_months: item[:insurance_months], | ||
| lifetime_insurance: ActiveModel::Type::Boolean.new.cast(item[:lifetime_insurance]) || false, | ||
| details_synced_at: synced_at, | ||
| updated_at: synced_at |
There was a problem hiding this comment.
🟡 Partial detail updates erase stored prices
When a detail request omits price, StoreDetails#run clears the stored price and marks details complete. Future list syncs skip the pledge, leaving its price missing.
Learn more
The detail input requires only id, so callers can update insurance without sending price. This update writes nil for omitted fields and always sets details_synced_at. The list sync's details_pending only returns rows whose timestamp is nil, so an erased price is never retrieved again by the normal detail pass.
Example: Pledge 1 has a stored price of $100 and six months of insurance. A later detail request {id: "1", insuranceMonths: 120} clears the price, and the next list sync excludes pledge 1 from detailsPending.
Recommended fix: Distinguish omitted fields from explicit clearing in StoreDetails#run, updating only keys present in each item. Define a separate explicit mechanism for recording a 404 or a page with no readable price, and do not mark incomplete updates as fully synced.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intended. A detail post is always one complete read of a pledge's RSI page, and details are read once per pledge (a buy-back costs its original price). An item without price means the page had no readable price, or RSI answered 404, and is recorded as read so that pledge isn't fetched on every sync. Nothing else posts to this endpoint, so there is no partial update that could clear a stored price. 🤖
| t.decimal :price, precision: 15, scale: 2 | ||
| t.integer :insurance_months | ||
| t.boolean :lifetime_insurance, null: false, default: false | ||
| t.datetime :details_synced_at |
There was a problem hiding this comment.
🔍 Described currency column is absent
The PR description lists price_currency, but the migration adds no such column. The implementation stores USD without one; reconcile the description with the intended schema.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Right, the description was left over from before prices became USD-only. Updated: the columns are available, price (USD), insurance_months, lifetime_insurance and details_synced_at. 🤖
| const message = await request( | ||
| FleetyardsSyncAction.SYNC_BUYBACK_DETAIL, | ||
| { id }, | ||
| undefined, | ||
| (reply) => reply.id === id, | ||
| ).catch(() => undefined); |
There was a problem hiding this comment.
🔍 Verify detail reply IDs before release
The detail pass matches replies by top-level id. Verify the forthcoming extension echoes that field, including on 404 responses; integration was not tested end to end.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Checked against the merged extension (fleetyards/sync#164): the detail reply is {action, id, ...result} for every HTTP status, 404 included, and the 401 reply carries id too. Only the 400 for an invalid id omits it, and the page never sends one. 🤖
1 participant
Resolves #5471. The decisions and the RSI markup they rest on are in the issue.
What changed
/hangar/buybacksgets a "Buy back on RSI" link that opens in a new tab. Packages, ships, paints and add-ons go to/pledge/buyback/<id>. An upgrade's buy-back is a modal on RSI's list page with no URL of its own, so upgrades link to/account/buy-back-pledges.article.pledge[data-disabled], and the row gets a "Not available" tag.pledge_priceminus the from-ship's, in USD, joined onmodels.rsi_id. It is computed when the list is read and never stored, so it follows our price sync.detailsPending, the non-upgrade pledges with no details yet. The modal first reads the account's store pricing through the sync extension (currency, exchange rate and tax rate, the rates RSI converts the account's prices with). Then it reads each pledge's RSI buy-back page and divides the page price back into RSI's USD figure. Results go to the newPUT /hangar/sync-rsi-buyback-detailsin batches of 25. That is the original purchase price, read once and never refreshed. A page whose currency the pricing does not cover is not stored. A pledge page with no price, or one RSI answers 404 for, is stored without one.buyback_pledges:available,price(USD),insurance_months,lifetime_insurance,details_synced_at.docs/findings/rsi-buyback-page.mdnow covers the detail page, the account pricing and the conversion back to USD, the availability marker, an account with no buy-backs, and RSI's upgrade price API (with why we don't use it).Needs fleetyards/sync#164 released before ship and package prices and insurance show up in production. Until then the page has links, availability and upgrade prices.
Currency: an account priced in another currency is converted back with RSI's own rates. The rates' scale (8800 meaning 0.88) is inferred from one EUR account and checked against six pledges, not documented by RSI. An unscaled rate is refused rather than guessed.
Testing
bin/rails test test/integration/api/v1/hangar_buybacks_test.rb test/models/validation_error_test.rb: 32 passeduseBuybackDetailsSync,components/Hangar, the buy-back parsers andRSIStorePricing: 84 passedpnpm lint:tspasses.pnpm lint:fixreports two errors inuseFilters.spec.ts, which is unchanged frommain.db:migrateis blocked here, soschema.rband the annotations were written by hand and the test database was loaded from them.🤖