Take activity into account in talentpool #2418
Conversation
wookie184
added
t: feature
| if member: | ||
| line += f" ({member.name}#{member.discriminator})" | ||
| else: | ||
| line += " (not in server)" |
There was a problem hiding this comment.
I liked the way we crossed off the name before, it's more catchy that way.
Can't we combine them both ? e.g cross off the name and say that they're not part of our server anymore ?
There was a problem hiding this comment.
I generally prefer actual text since it's explicit and doesn't require any knowledge of the actual meaning. Since users without activity in the past 30 days will be removed anyway, there should be fewer users not on the server on the list.
There was a problem hiding this comment.
I generally agree, but I wanted to avoid making lines too long. Don't feel strongly about this though.
Should probably match the "(reviewed)" part in terms of styling, or vice versa.
There was a problem hiding this comment.
Given the 💤 idea below, you could use :userleave: from the bot's emojis in this case. Just a thought.
There was a problem hiding this comment.
I've left it as is for now so it's sort of symetrical with the ({member.name}#{member.discriminator}) that other's have, although i'm happy to change it.
| if member: | ||
| line += f" ({member.name}#{member.discriminator})" | ||
| else: | ||
| line += " (not in server)" |
There was a problem hiding this comment.
I generally agree, but I wanted to avoid making lines too long. Don't feel strongly about this though.
Should probably match the "(reviewed)" part in terms of styling, or vice versa.
| elif is_ready_for_review: | ||
| line += " *(ready for review)*" |
There was a problem hiding this comment.
I'm not sure I like the implication that newer nominations are not ready for review. The min amount of days is a detail for the auto-reviewer. If a moderator has a good reason to put someone for review early, then they should. I'm guessing this isn't what you meant, but it's how it reads once this becomes a user-facing text.
Regardless, since we're now auto-removing inactive nominations, the activity criterion in is_nomination_ready_for_review seems unnecessary as long as the count in messages_per_user is produced with the same amount of days as the inactivity criterion. Whether they are in the server and whether they were already reviewed are already included information in the embed. So the only extra bit of info the highlighted addition provides is whether the min amount of time passed.
What can be done here (tell what you think) is to instead have two subtitles in the embed: Recent Nominations and Nominations Ready for Auto-Review. This would also help avoid overloading each line with information.
There was a problem hiding this comment.
I'm not sure I like the implication that newer nominations are not ready for review. The min amount of days is a detail for the auto-reviewer. If a moderator has a good reason to put someone for review early, then they should. I'm guessing this isn't what you meant, but it's how it reads once this becomes a user-facing text.
Yeah that's fair
Regardless, since we're now auto-removing inactive nominations, the activity criterion in is_nomination_ready_for_review seems unnecessary as long as the count in messages_per_user is produced with the same amount of days as the inactivity criterion.
The days value isn't the same though (7d for inactivity and 30d for removal). I think it makes sense for them to be separate too, I don't think we need to review anyone who hasn't been active in the past 7d, but that's definitely too short for complete removal.
What can be done here (tell what you think) is to instead have two subtitles in the embed: Recent Nominations and Nominations Ready for Auto-Review. This would also help avoid overloading each line with information.
I like this idea, i'll try it out. Do you think we'd need an indication for those without recent activity, (maybe a 💤 emoji or something? Not 100% clear but the best I can think of), or not?
There was a problem hiding this comment.
The days value isn't the same though
Ah right
Do you think we'd need an indication for those without recent activity
A 💤 could work, though that has the issue you mentioned above of using implicit meanings. I guess it will be fine if it's documented in the help embed.
There was a problem hiding this comment.
I made it three groups, "being reviewed", "recent nominations", and "others by autoreview priority", thoughts?
There was a problem hiding this comment.
If that's the case then we no longer need the (reviewed) part. Also recent nominations and others by autoreview priority doesn't tell me what it means for the nomination to be recent (which I think what you were trying to accomplish with the (ready for review)). That's why I suggested Nominations Ready for Auto-Review, because it implies that recent nominations won't be auto-reviewed.
There was a problem hiding this comment.
If that's the case then we no longer need the
(reviewed)part.
I made it so it only appears for the newest/oldest subcommands which don't do any grouping.
Also
recent nominationsandothers by autoreview prioritydoesn't tell me what it means for the nomination to be recent (which I think what you were trying to accomplish with the(ready for review)).
It's explained in the command help, although the actual conditions are quite complicated so it's hard to get all information across in a simple way.
There's not a specific use for listing nominations, which makes it harder to design a useful output. I wanted to be able to get an idea of who might be put up for review next, I don't really mind exactly how that's done.
That's why I suggested
Nominations Ready for Auto-Review, because it implies that recent nominations won't be auto-reviewed.
The "other" section doesn't necessarily mean ready for review though, e.g. if the user is not on the server or doesn't have recent activity.
There was a problem hiding this comment.
I just think that once you don't say what it means for a nomination to be recent, it's unclear why it should be in a separate section. It will probably be at the bottom of the priority list anyway.
Either way, it's not a blocker. You could ask the mods what they think a useful output would be.
| if nomination.reviewed: | ||
| continue | ||
|
|
||
| log.info("Removing %s from the talent pool due to inactivity", nomination.user_id) |
There was a problem hiding this comment.
Nitpick: don't we mostly use f-strings now for string formatting ? It also keeps it consistent with how you're formatting other strings in your changes
There was a problem hiding this comment.
For logging I think we usually use this style across the bot, https://blog.pilosus.org/posts/2020/01/24/python-f-strings-in-logging/ has some reasons and I think there's also potential security issues if you're passing untrusted input (not relevant in this case)
| # ... and is for a user that has been active recently | ||
| self.is_user_active_enough(user_message_count) and | ||
| # ... and is currently a member of the server | ||
| await get_or_fetch_member(guild, nomination.id) is not None |
There was a problem hiding this comment.
This should be nomination.user_id :P
I was testing but the predicate was always returning False and I'm like: "wat, but this should fit" ahahah
There was a problem hiding this comment.
Ahh, good find, thanks!
Closes #2302
It's not thoroughly tested yet and I will do some more, although I think it's good enough to open and get reviews.