Skip to content

[18India] Fix hand share purchase price to use IPO price#12714

Open
ddaybell wants to merge 1 commit into
tobymao:masterfrom
ddaybell:18india-fix-hand-share-price
Open

[18India] Fix hand share purchase price to use IPO price#12714
ddaybell wants to merge 1 commit into
tobymao:masterfrom
ddaybell:18india-fix-hand-share-price

Conversation

@ddaybell

Copy link
Copy Markdown
Contributor

Summary

  • can_buy_from_hand? and can_buy_company? affordability checks for hand share certificates now use IPO/par price explicitly via a new hand_price(company) helper, rather than company.value which can reflect market price when the underlying share's ownership changes
  • hand_price computes par_price * num_shares for :share and :president type certificates; falls back to company.value for other types or if par_price is unavailable

Test plan

  • All 185 existing fixture tests pass
  • Verify a player can buy a hand share certificate at IPO price when the corporation's market price has moved above or below the original IPO price

Closes #11114

?? Generated with Claude Code

…rice

Affordability checks for hand share certificates now explicitly compute
the IPO price from par_price * num_shares rather than relying on
company.value, which can reflect market price when share ownership
changes. Closes tobymao#11114.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
perwestling
perwestling previously approved these changes Jun 14, 2026

@perwestling perwestling left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: #12714 - [18India] Fix hand share purchase price to use IPO price

Reviewed with AI assistance by Mistral Vibe


✅ Strengths

  • Fixes documented issue (#11114)
  • Minimal, focused change (13 lines added, 2 removed)
  • Correct root cause: was using company.value (market price) instead of IPO/par price for hand certificates
  • All 18India fixtures pass (43/43)
  • Clean helper method (hand_price) with proper fallbacks

💡 Strong Recommendation

Since 18India has existing fixtures, please add a test case verifying:

  • Hand share certificates use IPO price, not market price
  • Fallback to company.value for other certificate types
  • Behavior is correct when market price differs from IPO price

❓ Questions

  • Have you verified the manual test case mentioned in the PR description?

✅ Verdict: Approve

The fix is correct and addresses a real bug. Adding a test is strongly recommended but not blocking.

end
end

def hand_price(company)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strongly recommend: Add state-based fixture test

Since 18India has existing fixtures, please add a test that verifies:

  1. A player can buy a hand share certificate at IPO price when corporation's market price is above IPO price
  2. A player can buy a hand share certificate at IPO price when corporation's market price is below IPO price
  3. Non-share/president certificates still use company.value

This prevents regression of the price calculation logic.

@perwestling perwestling dismissed their stale review June 15, 2026 19:31

Remove AI assisted approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[18India] Purchase from hand amount checking is wrong

2 participants