feat: DiscordRoleFilter #68
Conversation
Implementation for new settings: visible_roles can_add_roles can_remove_roles
|
Please find this pull request over some work I have done for my corporation that maybe of interest to others. Thanks very much for the good work with this plugin. |
| * | ||
| * @var string DEFAULT_CAN_REMOVE_ROLES | ||
| */ | ||
| public const DEFAULT_CAN_REMOVE_ROLES = '@@everyrole:RoleName Example2 RemoveMe'; |
There was a problem hiding this comment.
instead providing hardcoded roles list as value, wouldn't it be better to put this as field tooltip into connector settings ?
There was a problem hiding this comment.
IIRC the design goal is not to break peoples systems during an upgrade.
An empty value does not make the overall module (seat-discord-connector) work as most users might expect, the empty value will disable functionality.
Not sure tooltip information alone helps. I'm sure the tooltip information can be improved, I didn't spend much time on it (since I know what it does lol).
This is about a fresh install or an upgrade to this version and then opening the discord settings and ensuring the new fields are populated with defaults that continue to maintain old behavior where possible. In the case of this new feature that should be possible, hence this value is in code as well as documentation (README).
If you only put information in documentation and tooltips many people won't read/notice during upgrade and the first time they save the settings after upgrade (which could be months) their connector will stop working like it did before pressing Discord Settings -> Save. Because it will save an empty string value by default, which will change module behaviour.
How many support issues might this cause, due to not reading documentation around the uprade ?
The @@everyrole is an illegal role name on Discord, so was picked for its special meaning.
That role names in discord can contain spaces.
That the use of * it also a valid role name in discord.
Discord has a @everyone with special meaning, so it wasn't a far stretch to use @@everyrole
I'm trying to convey to someone not reading the seat-discord-connector documentation and making a guess how to configure seat-discord-connector with 2 or more roles and when roles contain spaces all at the same time. i.e. space is not the role name delimiter, space is allowed and considered part of the role name. So ideally the SeAT administrators best guess on what to do will be correct first time.
|
|
||
| } | ||
|
|
||
| ?> |
There was a problem hiding this comment.
php closing tag is non longer expected
There was a problem hiding this comment.
habbit, PHP is my 15th language
|
|
||
| } | ||
|
|
||
| ?> |
There was a problem hiding this comment.
php closing tag is non longer expected
There was a problem hiding this comment.
habbit, PHP is my 15th language
| */ | ||
| public function __construct(string $spec = '', string $explodeOn = ':') | ||
| { | ||
| $this->filter_specs = explode($explodeOn, $spec); |
There was a problem hiding this comment.
maybe collection could be useful here ?
https://laravel.com/docs/6.x/collections
There was a problem hiding this comment.
does the configuration system allow storage and default conversion to collections ?
so the primary usage and input data types/format is directly from the string types in the settings dialogs
or does PHP now allow overloading of the constructor so an additional ctor can added
There was a problem hiding this comment.
collection will allow you to manipulate array of things, including way to exclude data from final result.
ie: https://laravel.com/docs/6.x/collections#method-filter
Maybe but I don't have experience there. We only use discord and it would require auditing all other connectors for conventions, then work validating it all. Not something I'm looking to do.
Yes it went into production at the time of the PR and has resolved our role management issues. No further changes were needed. I probably did remove some trailing The Codacy Static Code Analysis seems to be unhappy about *Test.php files added, I'm not fussed about fixing concerns in unittests. |
Implementation for new settings:
visible_roles
can_add_roles
can_remove_roles