Add cj pin and cj unpin

[D0rs4n]

Lets Code Jam participants pin messages in their own team channels. Check done via the Code Jam MGMT api.
Closes #53

[ChrisLovering]

In my opinion, if this command is ran outside of the code jam team category, it should silently fail. Sir-Robin has no reason to respond to this command outside of that context.

If it is inside the code jam category, then you can assume the user running the command has permission to pin the message, as no other users will be in those channels.

If we really wanted to be safe, we can still check that the user invoking the command is a participant, but it's not 100% needed with the above solution.

[D0rs4n]

In my opinion, if this command is ran outside of the code jam team category, it should silently fail. Sir-Robin has no reason to respond to this command outside of that context.

If it is inside the code jam category, then you can assume the user running the command has permission to pin the message, as no other users will be in those channels.

If we really wanted to be safe, we can still check that the user invoking the command is a participant, but it's not 100% needed with the above solution.

As for you first suggestion I agree, it should silently fail. However, I don't think we should rely on categories, those categories are created as we go, hence we don't add them as environment variables, so we would have to rely to their names, which feels like something that could easily break. (I think we discussed a similar issue when creating those categories). That's why I wanted to stick with the API.

[brad90four]

LGTM content-wise, have not tested locally

[ChrisLovering]

In my opinion, if this command is ran outside of the code jam team category, it should silently fail. Sir-Robin has no reason to respond to this command outside of that context.
If it is inside the code jam category, then you can assume the user running the command has permission to pin the message, as no other users will be in those channels.
If we really wanted to be safe, we can still check that the user invoking the command is a participant, but it's not 100% needed with the above solution.

As for you first suggestion I agree, it should silently fail. However, I don't think we should rely on categories, those categories are created as we go, hence we don't add them as environment variables, so we would have to rely to their names, which feels like something that could easily break. (I think we discussed a similar issue when creating those categories). That's why I wanted to stick with the API.

Using the API seems fine to me.

However, we are still giving error messages to users in other channels. EG if the user isn't a participant or if they didn't reply to a message.

IMO sir-robin should never respond to this command if it is outside of a codejam channel.

Also, on the code side of things, if you have a huge if statement, and then a tiny else statement, it is usually best to do the else statement first and return early.

EG for this it would be

if not isinstance(referenced_message, discord.Message):
    ctx.send( ... )
    return

...
the rest of the code
...

[D0rs4n]

In my opinion, if this command is ran outside of the code jam team category, it should silently fail. Sir-Robin has no reason to respond to this command outside of that context.
If it is inside the code jam category, then you can assume the user running the command has permission to pin the message, as no other users will be in those channels.
If we really wanted to be safe, we can still check that the user invoking the command is a participant, but it's not 100% needed with the above solution.

As for you first suggestion I agree, it should silently fail. However, I don't think we should rely on categories, those categories are created as we go, hence we don't add them as environment variables, so we would have to rely to their names, which feels like something that could easily break. (I think we discussed a similar issue when creating those categories). That's why I wanted to stick with the API.

Using the API seems fine to me.

However, we are still giving error messages to users in other channels. EG if the user isn't a participant or if they didn't reply to a message.

IMO sir-robin should never respond to this command if it is outside of a codejam channel.

Also, on the code side of things, if you have a huge if statement, and then a tiny else statement, it is usually best to do the else statement first and return early.

EG for this it would be

if not isinstance(referenced_message, discord.Message):
    ctx.send( ... )
    return

...
the rest of the code
...

Resolved in db082ac

Significant changes have been made

[janine9vn]

Looks good! I have a request/comment about allowing pin/unpins from message links. I can be swayed either way though.

Also a small note about reverting an earlier decision about which role to lock commands to.

[janine9vn]

Very nice!

[Kronifer]

looks good

[brad90four]

Looks good, just the one thing on the conditional error message (I don't think it should have a dangling comma)

[D0rs4n]

These two commands have an authorization issue and maaaybe another one too:

1. The Events Lead cannot use `&cj pin` or `&cj unpin` (poor Kat)

2. Any helper part of the Events Team can abuse `&cj pin` and `&cj unpin` to (un)pin any messages in channels Robin has permission to do so. I have no idea what permissions Robin will be given in the actual server during normal operations, but let's verify this won't be an issue (if not block this route entirely).

1.:The Events Lead can use the command, since it's enabled for the whole Events Team. (which includes the Events Lead)
2.: No the staff cannot abuse this, since it's limited to the Code Jam categories, hence the category, if invoked outside of the Code Jam categories, it'll silently fail.

[ichard26]

Re 1. right makes sense. Sorry about that. Re 2. I'm talking about a staff member who is on the Events Team invoking &cj pin in a CJ team channel to pin a message outside of the CJ categories. A bit of an edge case I'll admit, but I thought it was worth mentioning.

I'll dismiss my review so a merge is still possible since 2 is definitely an edge case.

I am an idiot re. 1

[D0rs4n]

2 is valid, Stalling the PR while figuring it out.

Significant changes have been made.

[ichard26]

I'm sorry, but some more changes are needed.

Technically the Events Team and Admins can (un)pin a message from a CJ team channel that IS NOT the channel the command was invoked in, but honestly I don't think this matters much and isn't a permission bypass (just confusing). So I suggest ignoring this unless someone feels very strongly. I'm kind of scared of making any more changes than necessary 😅

[ChrisLovering]

Both the pin and unpin command have a lot of duplicated code. IMO this tends to mean that it should be abstracted to a helper func, where the two commands just call this function with pin or unpin and the helper func does all this work in a single place. (see the help channel implementation.)

There also seems to be a very common pattern in sir-robin where we get a team, do some API error handling and then do something with the team.

IMO this fetching of the team and error handling should be moved to a helper function, to further remove this code duplication.
Extra points if you return the team in a data class, so we're not doing dict slicing everywhere.

[D0rs4n]

Resolved all. 🤞
Except for the helper to handle team related MGMT requests, that'll be included in a separate PR.

[ichard26]

Wheee this looks so much nicer. Can confirm it works and has no permission issues. Great work @D0rs4n, and thank you @ChrisLovering for bringing some sensibility back into this mess :)

[D0rs4n]

Wheee this looks so much nicer. Can confirm it works and has no permission issues. Great work @D0rs4n, and thank you @ChrisLovering for bringing some sensibility back into this mess :)

Thank you for your thorough review! :)