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

Add half-life event member updates #204

Merged
merged 12 commits into from
Jan 25, 2023
Merged

Conversation

maurolacy
Copy link
Contributor

@maurolacy maurolacy commented Jan 24, 2023

Closes #203.

Opted for straightforward changes for the fix, instead of a full refactoring. Member.points is unsigned, and adding more complexity to update_members doesn't look like a good idea. In fact, perhaps it would be better to break down update_members into add_members and remove_members (as suggested in the past), and then add a new halve_members in parallel.

TODO:

  • Unit tests. (done)
  • Migration handler update member msgs fixing code (done).

@maurolacy maurolacy self-assigned this Jan 24, 2023
@maurolacy
Copy link
Contributor Author

This bug shows the limits of unit testing, by the way. Everything is properly unit tested, and still this is wrong, and fails in an integrated environment.

Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Thank you for the quick fix.
It looks correct. Added some minor code cleanup comments, but mainly would like an explicit test for the new migrate function that it triggers this correctly.

Fortunately, they are being ignored on the receiving (mixer) end
@maurolacy maurolacy force-pushed the fix/halflife-member-updates branch from 54fd28c to daf5f26 Compare January 25, 2023 08:12
Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Thanks for the changes.
Approved to merge and deploy

if points <= 1 {
return Ok(None);
}
Ok(Some(MemberDiff::new(addr, Some(points), Some(points))))
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good.

@maurolacy maurolacy merged commit cce76df into main Jan 25, 2023
@maurolacy maurolacy deleted the fix/halflife-member-updates branch January 25, 2023 20:57
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.

[tg4-engagement] Half-life process does not update members
2 participants