-
-
Notifications
You must be signed in to change notification settings - Fork 714
Patreon #1917
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
Patreon #1917
Conversation
Add function to send the current supporters Add a command to call this function
I would also like a review from @ChrisLovering - cannot work out how to request a review from you though :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the hardwork you've put into this PR!
A few small things in my preliminary review.
Revert "Rename some variables" This reverts commit 2f5aeb0. Fix variable names (again) And again
2f5aeb0
to
68b382a
Compare
Changes made
Please remove my request for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't intend to leave another review but I am.
@commands.Cog.listener() | ||
async def on_member_update(self, before: discord.Member, after: discord.Member) -> None: | ||
"""Send a message when someone receives a patreon role.""" | ||
old_patreon_tier = get_patreon_tier(before) | ||
new_patreon_tier = get_patreon_tier(after) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has an inherent dependency on how the patreon bot works. If the patreon bot decided to take away and giveth the new role in two seperate api requests, this would cease to work. Or, if the patreon bot were to take the old role away and then give the new role, this would also cease to work.
This all hinges on the patreon bot making a single api request, and would lead to funky behaviour if their bot used more than one request--and I believe it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be an issue. Whatever order things happen there will still only be one member_update that involves the tier increasing, which is all that this checks.
The only slightly unintended case would be if a user moves down a tier (e.g. from 3 to 2), and the bot does this by removing 3 and then adding 2. In this case the message would be sent, although it still wouldn't be incorrect (Just saying they are now a tier 2 patron).
Fixing this would probably require waiting for other role changes to see if they change things, although this could be fairly complicated so I think it would be best to see how what we have currently works out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few points, mostly about style so not requesting changes so as to not block the PR with this review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't able to test the role assignments on real users, but seem to work fine in manual testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving as my only comment won't affect us atm, so I'll leave it to you to decide whether to action it before merging.
I've also taken the liberty of adding the 3 patreon roles to the test server and updating the config.yml in notion.
Thanks, I definitely did not forget those were things that would need to be done :P |
Thanks @wookie184 for finishing this off for me! |
Closes #1765
First off, this is my first pull request so please give me general pull request feedback if you have any :-)
Some decisions have been made on discord. A search of
bot#1765
should find you all the conversations.Some changes to make to test:
There are 2 new commands:
!patrons
and!patreon
. These send the list of patrons and some patreon info respectively.Logging
I have not included an logging. Please provide feedback about logging.