Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[1849] fix for the Bonds variant #11376

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

philcampeau
Copy link
Collaborator

Fixes #11138

(hopefully)

Before clicking "Create"

  • Branch is derived from the latest master
  • Add the pins or archive_alpha_games label if this change will break existing games
  • Code passes linter with docker compose exec rack rubocop -a
  • Tests pass cleanly with docker compose exec rack rake

Implementation Notes

Explanation of Change

We keep getting blocking step errors when using the Bonds variant, and it's always happening after the game enters phase 8, which is when the bonds are enabled.

It's difficult to test this fix, as the error doesn't happen in my local test environment, but I think the fix here may help. I changed the round_state definition, and modified the related code.

Screenshots

Any Assumptions / Hacks

Comment on lines 23 to 24
issued_bond: nil,
redeemed_bond: nil,
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're only using boolean values/tests elsewhere, so you might as well make this boolean values here.

Suggested change
issued_bond: nil,
redeemed_bond: nil,
issued_bond: false,
redeemed_bond: false,

Copy link
Collaborator

@ollybh ollybh left a comment

Choose a reason for hiding this comment

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

@philcampeau Can you explain what you're changing here? There doesn't look to be anything wrong with this change (apart from my minor quibble about using boolean values), but how will this fix the bug?

@philcampeau philcampeau requested a review from ollybh November 24, 2024 20:16
@philcampeau
Copy link
Collaborator Author

@philcampeau Can you explain what you're changing here? There doesn't look to be anything wrong with this change (apart from my minor quibble about using boolean values), but how will this fix the bug?

To be honest, I don't know if it will fix the issue or not, but I can't figure out what the issue is, and I noticed I did this differently than every other instance on the site, so I figured I might as well try this to see if it helps.

Copy link
Collaborator

@ollybh ollybh left a comment

Choose a reason for hiding this comment

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

I can't see how this is going to fix the reported issue, but I can't see any problems with this patch.

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

Successfully merging this pull request may close these issues.

[1849 variants] server caught in a refreshing loop
2 participants