When someone first joins a room, they see some commands (`mailbox`,
`owner`, ..) and they know they are getters and setters, but they have
no good example as to how to use them.
Is it `!pm mailbox SOMETHING` or `!pm mailbox=SOMETHING` or something
else?
It's better if the introduction text gives you the full command you need
to get started (e.g. `!pm mailbox SOME_MAILBOX`), instead of a partial
command that you don't know how to use (e.g. `!pm mailbox` - this is
merely a getter and will not set your mailbox to `SOME_MAILBOX`).
Starting from this, I thought it would be a good idea to make all
option getters tell you how the commands are to be used. If you send
`!pm mailbox` and it tells you "not yet set", it should also tell you
how to actually set it (e.g. `!pm mailbox VALUE`).
If're an admin and mess up the `users` list, you won't see "owner"
commands.
If you're just a regular room user (not an admin, not an owner), you'll
only see the `help` command in the `help` message.
Both of these situations may make you wonder:
- is that all there is?
- earlier I saw more commands, so what's going on?
Adding "and accessible to you" hopefully clears things up, or at least
it tries to make the help message more correct.
This can be improved in the future, to show some additional information
about each mailbox like:
- "how many users are in that room"
- "which users are in that room"
- "who is the owner of the mailbox"
This can all be done later though.
Checking using `settings.Allowed` is odd. Not all commands are related
to setting configuration settings. Admin commands are coming in the
future, for which this is certainly not the case.
We now do access checks early on (during command processing), so command
handlers can be clean of access checks. If we're inside of a command
handler, the user is privileged to run it.
Retrieving the settings object for each and every option was wasteful
I don't like how we're doing custom formatting for optionMailbox in many
different places. This should be redone.
Before:
> * **`!pm help`** - Show this help message
> * **`!pm stop`** - Disable bridge for the room and clear all configuration
> * **`!pm mailbox`** - Get or set mailbox of the room
> * **`!pm owner`** - Get or set owner of the room
> * **`!pm nosender`** - Get or set `nosender` of the room (`true` - hide email sender; `false` - show email sender)
> * **`!pm nosubject`** - Get or set `nosubject` of the room (`true` - hide email subject; `false` - show email subject)
> * **`!pm nohtml`** - Get or set `nohtml` of the room (`true` - ignore HTML in email; `false` - parse HTML in emails)
> * **`!pm nothreads`** - Get or set `nothreads` of the room (`true` - ignore email threads; `false` - convert email threads into matrix threads)
> * **`!pm nofiles`** - Get or set `nofiles` of the room (`true` - ignore email attachments; `false` - upload email attachments
After:
> * **`!pm help`** - Show this help message
> * **`!pm stop`** - Disable bridge for the room and clear all configuration
> * **`!pm mailbox`** (currently `something@example.com`) - Get or set mailbox of the room
> * **`!pm owner`** (currently `@someone:example.com`) - Get or set owner of the room
> * **`!pm nosender`** (currently `false`) - Get or set `nosender` of the room (`true` - hide email sender; `false` - show email sender)
> * **`!pm nosubject`** (currently `true`) - Get or set `nosubject` of the room (`true` - hide email subject; `false` - show email subject)
> * **`!pm nohtml`** (currently `false`) - Get or set `nohtml` of the room (`true` - ignore HTML in email; `false` - parse HTML in emails)
> * **`!pm nothreads`** (currently `false`) - Get or set `nothreads` of the room (`true` - ignore email threads; `false` - convert email threads into matrix threads)
> * **`!pm nofiles`** (currently `false`) - Get or set `nofiles` of the room (`true` - ignore email attachments; `false` - upload email attachments)
In various places, we build messages using `Sprintf` before passing them
to `Notice()`.
If we let `Notice()` do string formatting, we run the chance of having
it try to format our already-preformatted text. If our text includes
format references (e.g. `%s`), it would cause a problem (`%s(MISSING)`).
One way to trigger it is to change the bot prefix from `!pm` to `%pm`.
Doing so, `sendHelp()` would create some help message which contains
`%pm` references. `Notice()` would then try to process them as well,
leading to a bunch of `%!p(MISSING)m` in the final text.
It previously said "the following commands" were supported
and it was only listing subcommands (help, stop, ..)
without any indication of how the user should construct the full command
(`PREFIX SUB_COMMAND`).
For perfect clarity, we now list full commands in the help message. Example:
> The following commands are supported:
>
> - !pm help - Show this help message
> - !pm stop - Disable bridge for the room and clear all configuration
> - !pm mailbox - Get or set mailbox of the room
> - !pm owner - Get or set owner of the room
> - !pm nosender - Get or set nosender of the room (true - hide email sender; false - show email sender)
> - !pm nosubject - Get or set nosubject of the room (true - hide email subject; false - show email subject)
The new help message is prefix-aware, instead of hardcodign `!pm`.
If the bot is running with a custom prefix (not `!pm`), this is even
more helpful, as it lets people know what the prefix is. Reading
documentation elsewhere and seeing `!pm STUFF` will no longer confuse
anyone.
With this change, we also make use of the existing `Notice()` function,
so we don't need to duplicate some code related to sending notices.
This also adds `Mailbox()` and `Owner()` getters for completeness.
Wording has been changed a bit to avoid saying "that room". It sounds
better if it's "this room" or just "the room".
