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

Starboard 2.0 #75

Merged
merged 33 commits into from
May 7, 2023
Merged

Starboard 2.0 #75

merged 33 commits into from
May 7, 2023

Conversation

andrewj-brown
Copy link
Member

@andrewj-brown andrewj-brown commented Mar 20, 2023

Starboard 2.0, implementing a bunch of stuff I originally ceebs'ed, plus novel features that committee discovered a need for:

  • Starboard reaction totals are now calculated as (reacts on original message + reacts on starboard message)
  • These reacts are now enforced one-per-person and don't include the author of the original message
  • Allowed starboarding messages from admin channels (comedy inbound!)
  • Subsequently, changed the message format so that channels display the name, rather than a link (which will be invisible for admin/private channels)
  • Committee now have the ability to "blacklist" a message from being starboarded
  • Committee now have a "cleanup" command that should remove any race-condition-caused duplicate messages
  • Deletions of bot-posted-starboard-messages no longer cause the bot to shit itself
  • Misc bugfixes (timezones; ratelimit in .env.example; replies to deleted messages; empty messages)

Many of these features are only minimally tested. Prior to merging I'll need to get a few other people together in the testing server and make sure it's all fully functional.

Copy link
Member

@JamesDearlove JamesDearlove left a comment

Choose a reason for hiding this comment

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

Not a full review, but here's some comments

uqcsbot/starboard.py Outdated Show resolved Hide resolved
uqcsbot/starboard.py Outdated Show resolved Hide resolved
uqcsbot/starboard.py Outdated Show resolved Hide resolved
uqcsbot/starboard.py Outdated Show resolved Hide resolved
uqcsbot/text.py Show resolved Hide resolved
Copy link
Member

@JamesDearlove JamesDearlove left a comment

Choose a reason for hiding this comment

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

Not a full review, but here's some comments

@andrewj-brown
Copy link
Member Author

alright i've made a few changes, mostly commenting. notably:

  • function-block'ed the thing jimmy mentioned
  • logging of blacklist in #admin-alerts
  • fixed timezone imports
  • insta-delete self-reacts when the bot sees them

Copy link
Member

@JamesDearlove JamesDearlove left a comment

Choose a reason for hiding this comment

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

Some additional problems I've found

uqcsbot/starboard.py Outdated Show resolved Hide resolved
uqcsbot/starboard.py Outdated Show resolved Hide resolved
@andrewj-brown
Copy link
Member Author

you can really tell i've spent so much time testing this pre-PR

@JamesDearlove
Copy link
Member

After conversations last night, I'm gonna mark this as a draft so it's not accidentally merged. Feel free to close it and start afresh @andrewj-brown

@JamesDearlove JamesDearlove marked this pull request as draft April 8, 2023 03:05
@andrewj-brown
Copy link
Member Author

andrewj-brown commented Apr 8, 2023

After conversations last night, I'm gonna mark this as a draft so it's not accidentally merged. Feel free to close it and start afresh @andrewj-brown

I'll leave it up; honestly it would have been a draft from the start if I knew anything about Github's UI

@andrewj-brown andrewj-brown marked this pull request as ready for review April 13, 2023 10:58
Copy link
Member

@49Indium 49Indium left a comment

Choose a reason for hiding this comment

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

LGTM, but I'll wait for another review as well (probably from @JamesDearlove ).
Some of the terminology (such as "big" and "whitelist"/"blacklist") was a little confusing, so a brief set of definitions at the top or slight name changes may be nice.

.env.example Outdated Show resolved Hide resolved
uqcsbot/starboard.py Outdated Show resolved Hide resolved
uqcsbot/starboard.py Outdated Show resolved Hide resolved
uqcsbot/starboard.py Outdated Show resolved Hide resolved
uqcsbot/starboard.py Outdated Show resolved Hide resolved
uqcsbot/starboard.py Show resolved Hide resolved
uqcsbot/starboard.py Outdated Show resolved Hide resolved
uqcsbot/starboard.py Show resolved Hide resolved
uqcsbot/starboard.py Show resolved Hide resolved
uqcsbot/starboard.py Show resolved Hide resolved
Copy link
Member

@49Indium 49Indium left a comment

Choose a reason for hiding this comment

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

LGTM

if modlog is not None:
client.loop.create_task(modlog.send(message))
else:
message += f" ...And also, I couldn't find #{MODLOG_CHANNEL_NAME} to log this properly."
Copy link
Member

Choose a reason for hiding this comment

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

Love this!

Copy link
Member

@JamesDearlove JamesDearlove left a comment

Choose a reason for hiding this comment

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

lgtm. Comments made reading this much easier, nice work

Copy link
Member

@JamesDearlove JamesDearlove left a comment

Choose a reason for hiding this comment

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

image

Nice

@andrewj-brown andrewj-brown merged commit f21ca4c into main May 7, 2023
@andrewj-brown andrewj-brown mentioned this pull request May 9, 2023
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.

3 participants