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

Take activity into account in talentpool #2418

Merged
merged 14 commits into from
Apr 8, 2023
Merged

Conversation

wookie184
Copy link
Contributor

Closes #2302

  • Only posts review for users who have been active in the past 7 days
  • Remove users who have been inactive in the past 30 days from the talent pool
  • Make the tp list command sort based on when the review will actually happen, and display
  • Send a message in nomination-discussion if a user is automatically removed from the talentpool.

It's not thoroughly tested yet and I will do some more, although I think it's good enough to open and get reviews.

@wookie184 wookie184 added t: feature New feature or request a: recruitment Related to recruitment: (talentpool) s: needs review Author is waiting for someone to review and approve labels Feb 26, 2023
Copy link
Member

@shtlrs shtlrs left a comment

Choose a reason for hiding this comment

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

This is only a code review, will test it later on.

Other than that, this looks beautiful !

bot/exts/recruitment/talentpool/_cog.py Outdated Show resolved Hide resolved
bot/exts/recruitment/talentpool/_cog.py Outdated Show resolved Hide resolved
bot/exts/recruitment/talentpool/_cog.py Show resolved Hide resolved
if member:
line += f" ({member.name}#{member.discriminator})"
else:
line += " (not in server)"
Copy link
Member

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Given the 💤 idea below, you could use :userleave: from the bot's emojis in this case. Just a thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

bot/exts/recruitment/talentpool/_cog.py Outdated Show resolved Hide resolved
Copy link
Member

@mbaruh mbaruh left a comment

Choose a reason for hiding this comment

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

Great additions, and some great style improvements as well.

bot/exts/recruitment/talentpool/_cog.py Outdated Show resolved Hide resolved
bot/exts/recruitment/talentpool/_cog.py Outdated Show resolved Hide resolved
bot/exts/recruitment/talentpool/_cog.py Show resolved Hide resolved
bot/exts/recruitment/talentpool/_cog.py Show resolved Hide resolved
bot/exts/recruitment/talentpool/_cog.py Outdated Show resolved Hide resolved
if member:
line += f" ({member.name}#{member.discriminator})"
else:
line += " (not in server)"
Copy link
Member

Choose a reason for hiding this comment

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

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.

bot/exts/recruitment/talentpool/_cog.py Outdated Show resolved Hide resolved
Comment on lines 246 to 247
elif is_ready_for_review:
line += " *(ready for review)*"
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made it three groups, "being reviewed", "recent nominations", and "others by autoreview priority", thoughts?

e9aa8f1

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 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)).

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

bot/exts/recruitment/talentpool/_cog.py Show resolved Hide resolved
bot/exts/recruitment/talentpool/_cog.py Show resolved Hide resolved
bot/exts/recruitment/talentpool/_cog.py Outdated Show resolved Hide resolved
bot/exts/recruitment/talentpool/_cog.py Outdated Show resolved Hide resolved
bot/exts/recruitment/talentpool/_cog.py Outdated Show resolved Hide resolved
bot/exts/recruitment/talentpool/_cog.py Show resolved Hide resolved
Copy link
Member

@shtlrs shtlrs left a comment

Choose a reason for hiding this comment

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

There's a bug that needs to be fixed, otherwise no one is ready for review :p

Other than that, it works great when i fix it locally :D

bot/exts/recruitment/talentpool/_cog.py Show resolved Hide resolved
if nomination.reviewed:
continue

log.info("Removing %s from the talent pool due to inactivity", nomination.user_id)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, good find, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@wookie184 wookie184 requested review from mbaruh and shtlrs March 25, 2023 17:58
Copy link
Member

@shtlrs shtlrs left a comment

Choose a reason for hiding this comment

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

:shipit:

@mbaruh mbaruh merged commit e64e710 into main Apr 8, 2023
@mbaruh mbaruh deleted the 2302-activity-in-reviews branch April 8, 2023 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: recruitment Related to recruitment: (talentpool) s: needs review Author is waiting for someone to review and approve t: feature New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Nominations: Take into account activity when reviewing
3 participants