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

Add watch reason to big brother header embed & fix small bug in watch command #266

Merged
merged 6 commits into from Jan 12, 2019

Conversation

3 participants
@SebastiaanZ
Copy link
Member

SebastiaanZ commented Jan 10, 2019

Edit:
I've changed the retrieval/caching of watch reasons so it only gets done when a user becomes active. There are a lot of historical entries on our watch list that are no longer active on our server, so there's no need to cache all of them at start-up time.

Summary:
This PR enables staff to see the reason a user is being watched while reading big-brother-logs. The code fetches the reasons out of the infraction table at start-up, caches it and serves it from cache when building the embed. This PR also fixes a small bug in the !bigbrother watch command. This PR closes #254

How it looks in action
If a reason was specified:
Hemlock's Cat with watch reason

If a reason wasn't specified (for backwards compatibility):
Hemlock's cat without a watch reason

The reason is retrieved by looking for the prefix
Since we store the watch reason as a note with a specific prefix, I'm using the prefix to search for the reason. The code than selects the latest reason set by comparing the dates.

A side-effect of this is that if we were to change the prefix, the old watch reasons will no longer be found by the code. I've therefore moved the prefix to the __init__ of the class with a comment not to change it in the future.

Small bug fix
In the old code, if the API-call to insert the user into the watch list failed, the error_message would overwrite the reason parameter before setting the note infraction. This caused the big-brother watch reason to be set to the API error_message. To mitigate this, I've renamed the variable name for the error_message to error_reason. I've also moved the infraction code so that an infraction will only be set if they were actually added to big-brother.

Update to config-default.yml
While the API route for infraction_user_type was given a key in constants.py, it wasn't actually set in config-default.yml. I've added it so I can use it to search the infraction table for the bb watch reason.

update_cache is now async
Because the update_cache method now needs to fetch the watch reasons from the API, I've made it a coroutine. I've updated all the calls to it so it gets properly awaited.

@SebastiaanZ

This comment has been minimized.

Copy link
Member

SebastiaanZ commented Jan 10, 2019

I noticed that the alias we had defined for !bigbrother watch <user> <reason>, the !watch, still had a default argument for reason (None). I removed it to make sure a reason is really mandatory.

@sco1 sco1 added this to Needs review in Bot Tracking via automation Jan 10, 2019

@sco1

This comment has been minimized.

Copy link
Member

sco1 commented Jan 10, 2019

For discussion: though assuming the existence of the watch prefix is a great simplification, it may still be still worth considering a fallback since most of the current watches were probably made before the watch reason was made mandatory.

@SebastiaanZ

This comment has been minimized.

Copy link
Member

SebastiaanZ commented Jan 10, 2019

For discussion: though assuming the existence of the watch prefix is a great simplification, it may still be still worth considering a fallback since most of the current watches were probably made before the watch reason was made mandatory.

What do you mean by that?

The watches for which no reason was specified currently show up as (no reason specified) as a fallback, so to say. I don't think we can handle it in a different way, since there was no reason given for those users. (Although I could be missing something obvious.)

@sco1

This comment has been minimized.

Copy link
Member

sco1 commented Jan 10, 2019

They may have other infractions that give context

@SebastiaanZ

This comment has been minimized.

Copy link
Member

SebastiaanZ commented Jan 10, 2019

That's true, maybe we could add the last infraction along side the watch reason.

Personally, I do like to keep the two separate, though: The watch reason was really intended as a reason for the thing we're looking at and the last infraction may not even be relevant. Unfortunately, the embed doesn't allow for two lines in the footer, otherwise we could've just listed the last infraction and the reason on separate lines.

@SebastiaanZ

This comment has been minimized.

Copy link
Member

SebastiaanZ commented Jan 10, 2019

@sco1 This could be an alternative embed lay-out:

Hemlock's Cat alternative lay-out for headed embed

I'm just worried that if the embed gets too large, it'll hurt readability in bb

@heavysaturn

This comment has been minimized.

Copy link
Member

heavysaturn commented Jan 10, 2019

I don't like the fat embed at all, but I do quite like the one in your original PR. I'm not too worried about a transitional period where old watches don't have reasons. That's a problem that will solve itself, given time. If it's particularly frustrating, we can just rewatch them to sort it out.

@SebastiaanZ

This comment has been minimized.

Copy link
Member

SebastiaanZ commented Jan 10, 2019

Right, something else that came to my mind is that getting the watch reasons for everyone is really not efficient. It's probably better to cache it the first time a user becomes active, so the bot doesn't have to query the API for everyone at start-up. Most of them are no longer active anyway.

Only retrieve/cache watch reason when user becomes active; restore up…
…date_cache to synchronous as it was before
@SebastiaanZ

This comment has been minimized.

Copy link
Member

SebastiaanZ commented Jan 11, 2019

I have kept the original, small embed for now (see the opening comment), since there doesn't seem to an agreement to change it at the moment and I'm leaning towards that option as well.

@heavysaturn
Copy link
Member

heavysaturn left a comment

This looks good to me.

return "(no reason specified)"

@staticmethod
def _parse_infraction_time(infraction: str) -> struct_time:

This comment has been minimized.

@sco1

sco1 Jan 12, 2019

Member

This could probably be added to bot.utils.time at some point, since we have parse_rfc1123 performing the reverse.

Bot Tracking automation moved this from Needs review to Reviewer approved Jan 12, 2019

@sco1

sco1 approved these changes Jan 12, 2019

@sco1 sco1 merged commit dc73cc8 into master Jan 12, 2019

Bot Tracking automation moved this from Reviewer approved to Done Jan 12, 2019

@sco1 sco1 deleted the bigbrother-add-watch-message-to-header-embed branch Jan 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment