Talentpool Autoreview #1459

mbaruh · 2021-03-11T00:13:22Z

~~This is blocked by python-discord/site#451~~

This closes #1423 and #1424

Reorganization

This PR requires the code to be split across multiple files. To avoid unnecessary file moves, I moved the files to a separate recruitment extension now instead of waiting for #1422.

Talentpool listing

The user's name and discriminator are now shown.
It is shown whether the user's automatic review is scheduled or if it was already posted.
Users who left the server are crossed off.

Questionable choices

Since the talentpool is still attached to the watch channel system (will be resolved in #1422), and the information displayed in big-brother and talentpool lists is slightly different (specifically the review info), I split the list_watched_users function into two parts, one organizing the information (prepare_watched_users_data), and one adding whatever necessary and posting it the list (list_watched_users). I then overrode (list_watched_users) in the Talentpool class. This will all probably need to be at least somewhat rewritten when the watch channel is removed.

Auto Reviewing

Once someone's name is added to the talentpool, it will now schedule an automatic review after 30 days.

The review contains details of the nominee, their nominations, the number of messages they have and the channels they're most active in, and statistics about their infractions and previous nominations.

Lastly, the bot will add three emojis to the review: A random ducky (or eyes if none found) to mark as seen, a thumbsup, and thumbsdown for the vote itself.
The code accounts for the possibility of the review being too long for a single message by splitting it where necessary.

The PR also adds the ability to prematurely post an automatic review with:
!talentpool review <nomination_id>

As well as being able to mark a nomination as already reviewed without posting an automatic review (e.g if someone chose to write a review themselves) by using:
!talentpool mark_reviewed <nomination_id>

Questionable choices

This was implemented by writing a Reviewer class, subclassing the Scheduler. This makes sense, to a degree, as its purpose is to schedule coroutine execution at a later date, and allowed me to avoid writing wrapper methods for the scheduler's cancel, cancel_all, and __contains__. This doesn't change the fact that using the scheduling methods directly on the Reviewer object and not through its added methods doesn't make immediate sense. Please judge.
Should those two commands work with the nomination ID, or should they work with the user ID?

This change is done in preparation to having the cog split across multiple files.

Make it a package as well so that the talentpool actually loads.

This commit adds the functionality to automatically review a nominee a set number of days after being nominated. This is implemented by subclassing the Scheduler and formatting a review after 30 days. The review contains details of the nominee, their nominations, the number of messages they have and the channels they're most active in, and statistics about their infractions and previous nominations. Lastly, the bot will add three emojis to the review: eyes to mark as seen, a thumbsup, and thumbsdown for the vote itself. The code accounts for the possibility of the review being too long for a single message but splitting it where necessary.

MarkKoz · 2021-03-12T06:31:56Z

I'm not sold on the current design that subclasses Scheduler. The main reason is that it doesn't provide much benefit over exposing the scheduler as an attribute. You stated yourself that it's unclear that the class is also a scheduler. If it was exposed as an attribute, the code would be more explicit.

The other issue with the design is that the subclass does a lot more than just scheduling. There's basically 1 function, schedule_review, that is directly tied to the scheduler. One may be able to argue for a subclass that only consists of that function. However, the problem is that the scheduler's other methods are still exposed. Therefore, there is still not much benefit. From an interface perspective, ideally the client would be forced to use schedule_review for all scheduling and have no way to access the regular schedule functions.

mbaruh · 2021-03-12T11:15:05Z

Hmm I get what you're saying about not subclassing it and making an attribute instead, but is it really better to apply changes to the scheduler directly from outside? I'd say it's clearer to write the methods that define how to interact with the class.

It didn't make much sense for the Reviewer to subclasses Scheduler. The Scheduler has methods that don't make sense to use on the Reviewer directly. There is now a Scheduler object as an attribute of the Reviewer. Interacting with it is done by adding __contains__, cancel, and cancel_all methods.