Support applying infractions to users not in the DB via Converter FetchedUser

[manusaurio]

Closes #627

Trying to ban a user uses 3 sources as converter: discord.Member, discord.User and the proxy_user function, in that order. If a user never joined the server, the first two fails, and proxy_user doesn't, but the latter is essentially useless anyway in this case, since the user needs to have its own row in the database for the infraction to take place.

How this change works

Ban-functions now try to convert the user argument with sources in the following order:

1. discord.Member
2. discord.User
3. bot.converters.FetchedUser (returns discord.User or discord.Object)

There are several rules for this in the previous flow:
If the FetchedUser converter is executed, it will fetch from the Discord API. If there's no error during the fetch, the discord.User object will be used to try to post the infraction. If there's an error because the user doesn't exist, the discord.User object attributes will be used to add a new user to the database, then infraction application will be posted again.

If there's any error during the fetch, and said error isn't explicitly that the user doesn't exist, a made-up discord.Object containing the user ID will be used, and if the user needs to be added to the DB, default values will be used to fill the remaining columns:

• avatar_hash: 0
• discriminator: 0
• in_guild: False
• name: Name unknown
• roles: []

Main changes and additions

1. Added post_user(...): adds a new user to the database, uses default values if the passed object doesn't have all the required attributes.
2. Changed post_infraction: tries to post_user(...) if the first try posting the infraction fails because the user doesn't exist.
3. Moved proxy_user to bot.converters
4. Added FetchedUser(Converter) class, which fetches from the Discord API and returns a discord.User or discord.Object if the fetching fails (as long as the error doesn't indicate the user doesn't exist.)

Observations

The flow after discord.Member/discord.User could be different (since we seem to aim to minimize API calls), but longer, and (presumably) more convoluted:

1. Try to use a proxy discord.Object first.
2. Try to apply the infraction with that.
3. If it doesn't exist, then try fetching the user. If it success, try adding it to the database.
4. If the fetch from before fails, then use the original discord.Object anyway and use default values for the rows to add the user to the database.

Examples

[manusaurio]

I know adding to the DB could have been done in the converter itself, but I felt it was a bit odd to make a converter have side effects like that. I think it should limit its job to just... convert. Not sure if I'm wrong on this.

[MarkKoz]

This looks promising so far. It may be worth keeping proxy_user in case a fetch or a POST to the database fails (but probably not if the fetch error indicates a Discord user with that ID does not exist). Note that the watch channels have their own proxy user definition for whatever reason.

[MarkKoz]

Also, there is a fetch_user somewhere in the infraction scheduler IIRC. Not sure if it should be removed if proxy_user is going to stay. After all, if fetch_user failed once (and thus we fell back to proxy_user), it's likely to fail again. On the other hand, it failing would be some weird, likely intermittent, API issue because we'd not fall back if the user doesn't exist.

[manusaurio]

I'll pick this up in a few hours, to refactor it and finish it.

Question: how should proxy_user work? I find strange the way it's done. It says Used when a Member or User object cannot be resolved., but it's still required to have a row for that user in the local DB. So, for this to work, it would be necessary to POST that user, basically with a made up ID. Isn't that a bit dangerous?

On the other hand, there's a PR by @Numerlor that removes both proxy_users and makes a class of it, so we should keep that in mind:
https://github.com/python-discord/bot/pull/651/files#diff-9d66c9dd2c18a8ecd9bc3be1100ae94eR73-R98

[MarkKoz]

The fetch may fail even if the user is already in the database since our API is, of course, separate from Discord's. In this situation the infractions will behave as they currently do - they work as long as the user is in the database. Better than not having anything at all to fall back on. If we want it to really be robust, then fake information could be posted to the API if it's fallen back to a proxy user. It's not a big deal since the ID is the only important field and that is being supplied by the person invoking the command (if it's a username instead then just give up).

[manusaurio]

A couple of issues came up, so now it looks worse.

As you said, @MarkKoz, it would be a good idea to have that discord.Object from proxy_user(id) still working, in case fetching fails, but the error doesn't imply the user doesn't exist. The problem is that I can't (I think) import the proxy_users from the moderation utils to converters (I think there's a circular import issue?) so these commits add a new proxy_user as a helper in the FetchedUser class itself. That's an abomination that should be addressed.

[MarkKoz]

in case fetching fails, but the error doesn't imply the user doesn't exist.

Why not? If it's a NotFound AKA 404 then that's exactly what it implies.

The problem is that I can't (I think) import the proxy_users...

There isn't a need to import it. Make the object inside the FetchedUser converter. Granted, this means FetchedUser isn't always a "fetched" user (i.e. the name isn't a 100% accurate description), but I think that's OK for our purposes. Here's a rough example:

class FetchedUser:
    async def convert():
        ...

        try:
            # If the fetch succeeds just return the user
            return await fetch_user(user_id)
        except discord.HTTPException as e:
            if e.status != 404:
                # There was some weird API issue.
                # Create a mock User object
                user = discord.Object(user_id)
                user.mention = user.id

                ...

                return user
            else:
                # It's a 404 so the user simply doesn't exist.
                raise BadArgument("User does not exist.")

The original proxy_users can likely just be completely removed and replaced by FetchedUser, unless you think there is a case for some other commands (now or in the future) for which it'd be not worth the API call of fetch_user.

[manusaurio]

Why not? If it's a NotFound AKA 404 then that's exactly what it implies.

My sentence should be read as "if there is an error and it doesn't imply the user doesn't exist, then return a proxy," sorry if I worded it poorly, the comma shouldn't be there (in case fetching fails but the error doesn't imply the user doesn't exist)

The original proxy_users can likely just be completely removed and replaced by FetchedUser

That makes it much easier then. But management.py does use proxy_use (to search for infractions.) Should it be removed anyway, and should I make it use FetchedUser instead too? I assume there shouldn't be a problem about the API limits unless the moderators really want to test it.

[MarkKoz]

But management.py does use proxy_use (to search for infractions.)

Oh that is a good case for it. I'd say leave it. You should just move proxy_user into the converters module to avoid any imports and allow the FetchedUser to return the proxy e.g. return await ProxyUser.convert(user_id).

Regarding #651, it's stale so I wouldn't wait for it. I don't think the resulting conflicts will be too bad. If you really want it merged first, you could try poking the author or even ask to take it over yourself if you fancy that.

[MarkKoz]

For the most part, this seems to work.

This fetch_user needs to be addressed somehow (e.g. skip DM if user is a proxy) because otherwise it will raise an error

bot/bot/cogs/moderation/scheduler.py

Line 110 in 3b6c7d8

user = await self.bot.fetch_user(user.id)

Watch channels have their own proxy_user which should be removed. The commands there should also use FetchedUser

bot/bot/cogs/watchchannels/watchchannel.py

Line 28 in 3b6c7d8

def proxy_user(user_id: str) -> Object:

[MarkKoz]

Nice work! I took some liberties to make the converter subclass UserConverter and also clean up some of the aliases.