chore: split `play` responsibilities & require perms for destructive commands

[afonsojramos]

Closes #148 and #149.

[afonsojramos]

kickstart discussion on Serenity to implement permissions at the subcommand level

I am not sure if this is a limitation of Serenity or Discord itself.

keep the same level of permissions for all play (sub)commands (either all DJ or not)

I do think that some /play commands are too destructive to be allowed by everyone as @rafaeldamasceno pointed out in #148, as such, I think we do, indeed, need to split responsibilities here.

[joao-conde]

I agree they are destructive, hence I would just protect them all. Changing the queue is always dangerous.

[afonsojramos]

But just plainly adding songs to the end of the queue seems like something anyone should be able to do... 🤔 I'm not 100% sure though 🤪 We must find some middle ground... Any suggestions @rafaeldamasceno?

[joao-conde]

I dislike the splay due to naming, overall split from other play commands which do the same, etc...

I agree everyone should be able to do /play and /play jump should be restricted.

Hence, the best solution for me would actually be to REVERSE all these flag/subcommand shenanigan and just give a regular play (if you want to play jump just /play + /skip you lazy dark web users). But this is very drastic so I dont see us all agreeing on this.

[rafaeldamasceno]

I agree everyone should be able to do /play and /play jump should be restricted.

100% in agreement with you here. If an API limitation is why the subcommands can't be restricted then some other things need to be taken in consideration.

I dislike the splay due to naming, overall split from other play commands which do the same, etc...

Well, I also don't love the naming. In fact, since slash commands have suggestions, I'd say we can be explicit in the naming: /superplay is a good option since super is a short word. I disagree, however, that they do the same thing. Especially for /play all, it has a clearly different behavior that cannot be achieved by any other command.

Hence, the best solution for me would actually be to REVERSE all these flag/subcommand shenanigan and just give a regular play (if you want to play jump just /play + /skip you lazy dark web users). But this is very drastic so I dont see us all agreeing on this.

I tend to agree: I never appreciated this subcommand approach, and I stated this clearly. But the fault in this case doesn't rely on the approach so much, but in the way the feature itself interacts with the DJ role and it's implementation.

In any case, I feel like this discussion shouldn't be held here. @afonsojramos @aquelemiguel Where could we discuss this whole feature interaction more in depth? Perhaps a discussion ticket?

[afonsojramos]

I created this PR in order for it to be a discussion so we could better see the changes and discuss based on that. What I should've done was to mark it as draft. 😕

[afonsojramos]

Hence, the best solution for me would actually be to REVERSE all these flag/subcommand shenanigan and just give a regular play (if you want to play jump just /play + /skip you lazy dark web users). But this is very drastic so I dont see us all agreeing on this.

The thing is that we will loose the functionality of some of the commands :(

Another proposal would be to proceed with this recommendation from @joao-conde and revert all the subcommands, BUT, introduce a skipto/forceskipto in order to also provide the jump functionality. However, we would still lose the /play all which I think it is actually useful. But at the same time that can be achieved by requiring for the traditional playlist link.

[afonsojramos]

Okay, I just had an idea that might actually be feasible... What if we manually checked for a user's role? We know the role's name, we just need to check it depending on which subcommand is called. This does not, however, fix the /play instead of /play end ordeal.

[rafaeldamasceno]

What if we manually checked for a user's role? We know the role's name, we just need to check it depending on which subcommand is called.

This is what I was going to suggest. I wanted to do this discussion further in depth because I understand that sometimes you (actual developers of the bot) are trying to align with the Discord API design values. Reworking the flag feature into subcommands or leaving all the play subcommands restricted just because it's what works when looking at the available API prevents a lot of useful functionality or good UX from existing. There are scenarios when you need to subvert the API/slash command design, because it simply wasn't designed to take into account every possible use case.

For this specific case, I would leave all /play or /splay or whatever subcommands unrestricted and manually check that the user has the necessary role for the jump and next, like @afonsojramos just said. Yes, it looks like any user can call them, but it's just two subcommands out of the bunch that do destructive actions, so it doesn't bother me that it looks like any user can call them, but instead face a permission error message. Restricting the commands that a user can definitely not use qualifies as "good enough" usage of the API for me. I wouldn't make the permissions in the bot limited to what the API allows in the command restriction department.

However, I really don't use the DJ role, not even in a big guild. If #146 is going to be closed and if the old unrestricted behavior is brought back as default, I think that people that actually use the DJ role more should jump in and give their feedback. Even so, I think that mine and Afonso's suggestion is good.

As for the /play//play end situation, is it really not possible for /play to exist if there are subcommands? If it isn't, I don't see any way to solve this but to have the subcommands moved to another command.

[joao-conde]

I had the same idea @afonsojramos . But manually checking for the role won't make the commands appear "grayed" out like the current commands for which a user does not have permission. It would instead just send a channel message like "no permission".

But then we would have grayed out commands and other commands which you could call but then would be denied access, seems hella weird.

[rafaeldamasceno]

Yeah, it's weird the behavior is inconsistent, but its the best we can do. As I've said in the previous comment, I think we should try to apply the API and slash command design as much as possible, but to use our own solutions when it doesn't solve our problems.

[aquelemiguel]

@afonsojramos @joao-conde @rafaeldamasceno I've only now read this thread. Let me chime in for a sec.

Firstly, I have no idea why we're discussing this on a PR, this conversation should have been held on #148.

Secondly, obviously we're not reverting the subcommand implementation. If anything, migrate them to a special command like /splay as suggested above.

Lastly, arguably the most destructive command is the base play itself because anyone can enqueue a 100+ song playlist without any restrictions. So obviously, the entire /play subcommand group should be limited. You don't go to the club and play your Spotify playlist, that's the DJ's job.

I'm closing this PR because it's clearly not agreed on and to move extra discussion to the appropriate place. After properly discussed we may reopen this.