JOJ0
Add server notice support
Implemented features (for now):
- Send to single matrix IDs or to all mxids matching a regular expression
- Uses pagination when sending to multiple users (batches of 100 by default, tunable by CLI option)
- Specify notice content in command arguments or in files
- Supports both unformatted and unformatted+formatted message content
- Asks for confirmation (including message content and sample mxids the notice will be sent to) by default before sending - can be disabled by flag
Thanks for those last changes, looking good and we are close to ready-to-merge. Some tiny formatting and help text notes though:
The META of the "Unformatted message" is PLAIN but the confirm_prompt tells "Unformatted message", this could be streamlined. I suggest using "Plaintext message" for the prompt (Or rather "Plain text message", not sure??)
Since FORMATTED only supports html, the brackets around HTML could be removed in the help text:
FORMATTED - HTML-formatted content of the notice. If not set, PLAIN will be used.
I suggest you remove the -a/--from-argument entirely, I think it's misleading what it does. I think your intention was that this boolean option has two states: Take from file or do the default: Take the cli args. I think we are better off without it. When --from-file is simply a flag, it is more clear what it's about.
Set show_defaults=True for the --paginate option. Also I wonder if streamlining with other synadm commands could be a good idea here. We have --limit in other places for pagination but I'm not entirely sure if it's exactly the same purpose as with this command. Have a look at synadm room list --limit for example to see what I'm talking about and help decide if it should be streamlined. Thanks!
Something is not working when paginate and regex is used together:
(synadm) ~/git/a-0_synadm (notice) $ synadm notice send '.*user' "plain message for text users" '<strong>formatted msg</strong>' -r --paginate 1
Recipients (list may be incomplete):
Unformatted message:
---
plain message for text users
---
Formatted message:
---
<strong>formatted msg</strong>
---
Send now? [y/N]:
It's fine when a matirx ID or just localpart is given and --to-regex is not present:
(synadm) ~/git/a-0_synadm (notice) $ synadm notice send 'testuser1' "plain message for text users" '<strong>formatted msg</strong>' --paginate 1
#Recipients (list may be incomplete):
- @testuser1:peek-a-boo.at
Unformatted message:
---
plain message for text users
---
Formatted message:
---
<strong>formatted msg</strong>
---
Send now? [y/N]:
Found some formatting issues in notice.py. Trailing spaces and line continuation issues. You might want to run flake8 over this file as well., Thanks!
I just realise that we not display what the API returns. Please display it at the very end of the cli method using helper.output(), it will automagically handle the printing to console according to the selected output format:
From API docs:
Once the notice has been sent, the API will return the following response:
{
"event_id": "<event_id>"
}
Also I realise that we don't fully support what this API is capable of. This is ok for this PR and can be advanced later! The checkbox on the README shouldn't be marked though, but somehow it should be noted that the notice api is "partly" implemented, or to a great deal implemented/almost/dont know. It's almost completely supported but there is room for improvement, so to say. The goal is to give other contributors the information that they could work on it still.
Eg.
You can optionally include the following additional parameters:
type: the type of event. Defaults to m.room.message.
state_key: Setting this will result in a state event being sent.
https://matrix-org.github.io/synapse/latest/admin_api/server_notices.html
Thanks for those last changes, looking good and we are close to ready-to-merge. Some tiny formatting and help text notes though:
The META of the "Unformatted message" is PLAIN but the confirm_prompt tells "Unformatted message", this could be streamlined. I suggest using "Plaintext message" for the prompt (Or rather "Plain text message", not sure??)
Since FORMATTED only supports html, the brackets around HTML could be removed in the help text:
FORMATTED - HTML-formatted content of the notice. If not set, PLAIN will be used.
Done. Will be committed soon.
I suggest you remove the -a/--from-argument entirely, I think it's misleading what it does. I think your intention was that this boolean option has two states: Take from file or do the default: Take the cli args. I think we are better off without it. When --from-file is simply a flag, it is more clear what it's about.
Done. Will be committed soon.
Set show_defaults=True for the --paginate option. Also I wonder if streamlining with other synadm commands could be a good idea here. We have --limit in other places for pagination but I'm not entirely sure if it's exactly the same purpose as with this command. Have a look at
synadm room list --limitfor example to see what I'm talking about and help decide if it should be streamlined. Thanks!
show_defaults is done and will be committed soon. A limit is not pagination though, it only has similar effects at the beginning. Pagination does not limit anything, it just specifies how many users receive the notice at once, before synadm queries the next (paginate large) batch of users. Of course, in the first query it limits how many will be returned, but with pagination you also use the next_token (an offset so to say) to query the next consecutive batch.
So limit would be a false wording here, pagination is the correct term for this case. In room_list, limit is the correct wording.
Something is not working when paginate and regex is used together:
(synadm) ~/git/a-0_synadm (notice) $ synadm notice send '.*user' "plain message for text users" '<strong>formatted msg</strong>' -r --paginate 1 Recipients (list may be incomplete): Unformatted message: --- plain message for text users --- Formatted message: --- <strong>formatted msg</strong> --- Send now? [y/N]:It's fine when a matirx ID or just localpart is given and --to-regex is not present:
(synadm) ~/git/a-0_synadm (notice) $ synadm notice send 'testuser1' "plain message for text users" '<strong>formatted msg</strong>' --paginate 1 #Recipients (list may be incomplete): - @testuser1:peek-a-boo.at Unformatted message: --- plain message for text users --- Formatted message: --- <strong>formatted msg</strong> --- Send now? [y/N]:
The example receipients is just a "tiny bous feature" that is supposed to assist the sender, it only matches the users in the first batch and if the regex applies, shows them in a list. If no user matches the regex in the first batch (which is likely if you set paginate to 1), the list will be empty. I considered querying all users until the list has 10 entries or all users have been queried, but that would have inflated the CLI-side code so I stuck with the easier option.
To ease this issue, I could imagine not using paginate in this dry run, but a fixed constant like 100, to make it more likely that some matrix id will be matched. paginate is a performance option anyways and should not accect the actual functionality of the command, so using a constant here sounds reasonable to me. What do you think?
I dont understand. In my first example exactly one user named testuser1 should be shown with paginate set to 1. That not what's expected?
Update: Ah ok paginate is happening before the regex?
Now I understand your suggestion with 100
Hmm not sure what to here...
Give it a try with the constant and I'll see how it feels then from a user perspective!
I set it to 100 now, you are probably not going to feel anything now, it will feel alright.
The point where this constant 100 feels awkward is when your server has a larger (e.g. >200) number of users and you apply a fairly restrictive regex. That way there may not be any match in the first 100, even though there are matches in later pages of users.
From a user perspective it would definitely make the most sense to search for regex matches until you have 10 matches or you searched all users. We would need to accept a larger notice_send_cmd then though, since this means quite some more logic. I currently have some time on my hands, so I'll do a demo-implementation and see how bad it actually would be.
As promised, here it is, and it has become a bigger function. However, I must admit I like this version a lot more as it feels more solid.
The user now has the guarantee that only the users in the "dry run list" will receive the notice - and the user can specify the upper bound of that list via CLI option. So if you really want to, you can check all 200 or 300 mxids yourself before sending.
The list ends with - ... if there is at least one mxid not mentioned in the list (limit too small) and if this last line is missing you know these are the only mxids.
So I'd like to keep this.
Yes this feels right now! Very nice! ... is clearly showing that the list is incomplete! Great!
--match-list-length is a rather longish name but at least it's descriptive. I don't have a better idea. I'll think about it. Maybe you wanna put short forms for this option as well as for --paginate? -l and -p for example?
To make flake8 happy with the click.options definitions you would want to use this line-continuation syntax (line-break must be directly after the opening bracket!):
click.option(
"--from-file", "-f", default=False, show_default=True,
is_flag=True, help="""Interpret arguments as file paths instead of notice
content and read content from those files""")
In api.py you also have an akward looking cointinuation syntax. I would prefer it like that and think it's nicely readable then:
outputs.append(
self.query(
"post", "v1/send_server_notice", data=data)
)
)
Or even like that, whatever you like better:
outputs.append(
self.query(
"post",
"v1/send_server_notice", data=data)
)
)
HTH
I first didn't like the idea of using -l because it very much sounded like --limit to me, but to be honest it's still a nice shorthand for a List Length, and "limit" is also half-true'ish. And you shouldn't use unknown options without looking at the help page anyways ;) So -l is the way to go I guess.
My typing errors around "awkward .... syntax" are actually very spot on the topic! 🤣
I approved the last changes now but might start a new review and change request.
Since the regex part became quite vital when testing the send command, I decided to include the few changes of the generate_mxid regex here instead of a separate pull request. (commit a700ec4 )
A noticable difference however is that the "replace @ and :" part is gone now (was your goal to convert @user: to user to @user:retrieved.hs.tld?). We can change that part back if you wish, to me it made most sense to match on None, then full valid mxid, then valid localpart, then return None since nothing is matched.
Imo (but this is very opinionated) you should not accept "too wrong inputs" as it becomes unclear to the user what actually happens. And I don't feel like the ability to pass @username instead of username is an actual improvement. Either mxid, or localpart, or regex. But no character manipulation to "make it fit". Just my opinion though, maybe you have different thoughts?
Since the regex part became quite vital when testing the send command, I decided to include the few changes of the
generate_mxidregex here instead of a separate pull request. (commit a700ec4 )
That's ok and appreciated!
A noticable difference however is that the "replace @ and :" part is gone now (was your goal to convert
@user:touserto@user:retrieved.hs.tld?). We can change that part back if you wish, to me it made most sense to match on None, then full valid mxid, then valid localpart, then return None since nothing is matched.Imo (but this is very opinionated) you should not accept "too wrong inputs" as it becomes unclear to the user what actually happens. And I don't feel like the ability to pass @username instead of username is an actual improvement. Either mxid, or localpart, or regex. But no character manipulation to "make it fit". Just my opinion though, maybe you have different thoughts?
Actually this behaviour was intentional. @user or user: was just accepted because it is clearly not an mixd.
I am fine with your decision but other problems pop up when I try it out now. It seems it is sent to Synapse now if I put @user or @user:
(synadm) ~/git/a-0_synadm (notice) $ synadm user details @testuser1
WARNING Synapse returned status code 400
errcode: M_INVALID_PARAM
error: Expected UserID string to start with '@'
(synadm) ~/git/a-0_synadm (notice) $ synadm user details @testuser1:
WARNING Synapse returned status code 400
errcode: M_INVALID_PARAM
error: Expected UserID string to start with '@'
(synadm) ~/git/a-0_synadm (notice) $ synadm user details testuser1:
WARNING Synapse returned status code 400
errcode: M_INVALID_PARAM
error: Expected UserID string to start with '@'
Shouldn't something like this be catched before and not even sent to Synapse? It's misleading: It tells the user he must put in a full mxid, which is not true!
Maybe that's the reason we just kicked out : and @ back when we implemented this? Can't remember but I don't like the behaviour now.
Either you just keep the basic logic of that condition block or you now test through all the other commands that rely on that regex function's behaviour. Each and everything that's handling something with usernames is relying on that.
I suggest you change back the second elif branch to stay else, so we keep close to the original behaviour.
What we could do that can't hurt: In the upgraded regex, please put a ^ sign at the very beginning: ^@[....
I think this works well:
if user_id is None:
return None
elif re.match(r"^@[-./=\w]+:[-\[\].:\w]+$", user_id):
return user_id
else:
localpart = re.sub("[@:]", "", user_id)
mxid = "@{}:{}".format(localpart, self.retrieve_homeserver_name())
return mxid
I just tested with some synadm user ... commands. It's important that Synapse receives what we want it to receive, so its error messages are not misleading!
For example: This is a nice response, Synapse is interpreting as a full mxid and sends to Synapse, which tells us that it's not the local serverpart. In that case we have a nice "playing together" of regex (plus replacing) magic and Synapse's original error messages.
(synadm) ~/git/a-0_synadm (notice) $ synadm user details '@testuser1:[1234::1234::1234::1234::1234::1234::1234]:5678'
WARNING Synapse returned status code 400
errcode: M_UNKNOWN
error: Can only look up local users
(synadm) ~/git/a-0_synadm (notice) $
Sure, it won't always play that well, but what I don't think is good is the new behaviour I discovered above when @user or user: is sent to Synapse which gives a misleading answer.
I am fine with your decision but other problems pop up when I try it out now. It seems it is sent to Synapse now if I put @user or @user:
Shouldn't something like this be catched before and not even sent to Synapse? It's misleading: It tells the user he must put in a full mxid, which is not true!
Well, the issue is that generate_mxid before and now could return None, but that case is generally not handled in synadm (except for my code :P), with my changes it would return None for something like @user , which is where the problems start.
I think this works well:
if user_id is None:
return None
elif re.match(r"^@[-./=\w]+:[-\[\].:\w]+$", user_id):
return user_id
else:
localpart = re.sub("[@:]", "", user_id)
mxid = "@{}:{}".format(localpart, self.retrieve_homeserver_name())
return mxid
This is close to what we had before, with the major problem why we started discussing changes: If someone passes ".*" as TO (in notice send), but forgot to set the regex flag, it is neither None, nor a valid mxid, so it lands in your else case and is force-built into some mxid which doesn't make any sense - and subsequently processed by synapse because generate_mxid didn't return None. That is what I wanted to avoid...
What we could do that can't hurt: In the upgraded regex, please put a ^ sign at the very beginning: ^@[....
This is not necessary when using re.match, we could still do it to improve code readability. See https://docs.python.org/3/library/re.html#re.match
Ok thanks for the reminder where we are coming from. For the user commands, this is how it reacts:
~/git/a-0_synadm (notice) $ synadm user details '.*'
WARNING Synapse returned status code 404
errcode: M_NOT_FOUND
error: User not found
~/git/a-0_synadm (notice) $
~/git/a-0_synadm (notice) $
~/git/a-0_synadm (notice) $
~/git/a-0_synadm (notice) $ synadm user details '....'
WARNING Synapse returned status code 404
errcode: M_NOT_FOUND
error: User not found
~/git/a-0_synadm (notice) $ synadm user details '@.*'
WARNING Synapse returned status code 404
errcode: M_NOT_FOUND
error: User not found
~/git/a-0_synadm (notice) $ synadm user details '@.*:bla.de'
WARNING Synapse returned status code 404
errcode: M_NOT_FOUND
error: User not found
~/git/a-0_synadm (notice) $
Not too bad and IMHO absolutely ok like that!
Let me see what would happen with the notice send command.....
Well, not too bad either:
(synadm) ~/git/a-0_synadm (notice) $ synadm notice send '.*' "plain message for text users" '<strong>formatted msg</strong>'
Recipients:
- @.*:peek-a-boo.at
Plaintext message:
---
plain message for text users
---
Formatted message:
---
<strong>formatted msg</strong>
---
Send now? [y/N]: y
WARNING Synapse returned status code 404
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $ synadm notice send '.....' "plain message for text users" '<strong>formatted msg</strong>'
Recipients:
- @.....:peek-a-boo.at
Plaintext message:
---
plain message for text users
---
Formatted message:
---
<strong>formatted msg</strong>
---
Send now? [y/N]: y
WARNING Synapse returned status code 404
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $ synadm notice send '@.*' "plain message for text users" '<strong>formatted msg</strong>'
Recipients:
- @.*:peek-a-boo.at
Plaintext message:
---
plain message for text users
---
Formatted message:
---
<strong>formatted msg</strong>
---
Send now? [y/N]: y
WARNING Synapse returned status code 404
(synadm) ~/git/a-0_synadm (notice) $
ok it's a 404, not tooo nice of a response. Can you please test how it reacts with your proposal?
Too me it looks like silently ignoring stuff. This is what I get with your extra elif branch:
(synadm) ~/git/a-0_synadm (notice) $ synadm notice send '@.*' "plain message for text users" '<strong>formatted msg</strong>'
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $ synadm notice send '.*' "plain message for text users" '<strong>formatted msg</strong>'
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $ synadm notice send '' "plain message for text users" '<strong>formatted msg</strong>'
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $ synadm notice send blabla "plain message for text users" '<strong>formatted msg</strong>'
Recipients:
- @blabla:peek-a-boo.at
Plaintext message:
---
plain message for text users
---
Formatted message:
---
<strong>formatted msg</strong>
---
Send now? [y/N]: y
WARNING Synapse returned status code 404
(synadm) ~/git/a-0_synadm (notice) $
It is not perfect either I think.....could be better....any error in this case is better than silently ignoring (from user perspective)
I agree silently ignoring is not too great, I could add a custom error output in case generate_mxid returns None (that check is done already, it's just adding one print(...) statement).
However I would generally argue we should not send mxids to synapse of which we could already know they don't exist (or contain arbitrary characters like [ or *), since we can control how our program behaves but passing such stuff to synapse may cause unwanted side effects
What we could do that can't hurt: In the upgraded regex, please put a ^ sign at the very beginning: ^@[....
This is not necessary when using re.match, we could still do it to improve code readability. See https://docs.python.org/3/library/re.html#re.match
Yes definitely! Just for readability. You mentioned the improvement of using re.match over re.search. :+1:
I agree silently ignoring is not too great, I could add a custom error output in case
generate_mxidreturnsNone(that check is done already, it's just adding oneprint(...)statement).
That sounds right!
However I would generally argue we should not send mxids to synapse of which we could already know they don't exist (or contain arbitrary characters like
[or*), since we can control how our program behaves but passing such stuff to synapse may cause unwanted side effects
That's certainly correct and would be the best of situations, but still Synapse is aware of handling errors itself and giving useful error responses. Not always, but often. Getting 404's is not very helpful to a user but anything else could be....if it's not misleading. We won't be able to catch everything and sometimes have to rely on Synapse too :-)
But yes I totaly agree that reacting on gereate_mxid returning None is worth a try! Go for it and then let's test again!
I was testing your last change yesterday and it still didnt feel well rounded. Sometimes we are getting an unknown recepient error and sometmes not. It feels random, thus the following idea coming up in a band rehearsal break ;-)
Try removing all the magic: No substitution of @ and :, as you already suggested previously, remove check for forgotten --regex.
Just an idea and didnt think it through thoroughly!!! Test how it behaves with user details command as well! Post some testing examples.
HTH
I put some thought and testing into this and came up with the following:
def generate_mxid(self, user_id):
""" Checks whether the given user ID is an MXID already or else
generates it from the passed string and the homeserver name fetched
via the retrieve_homeserver_name method.
Args:
user_id (string): User ID given by user as command argument.
Returns:
string: the fully qualified Matrix User ID (MXID) or None if the
user_id parameter is None or no regex matched.
"""
if user_id is None:
self.log.debug("Missing input in generate_mxid().")
return None
elif re.match(r"^@[-./=\w]+:[-\[\].:\w]+$", user_id):
self.log.debug("A proper MXID was passed.")
return user_id
elif re.match(r"^@?[-./=\w]+:?$", user_id):
self.log.debug("A proper localpart was passed, generating MXID "
"for local homeserver.")
localpart = re.sub("[@:]", "", user_id)
mxid = "@{}:{}".format(localpart, self.retrieve_homeserver_name())
return mxid
else:
self.log.error("Neither an MXID nor a proper localpart was "
"passed.")
return None
Also I realized that the only way to not screw things up with core changes like this is to be very explicit, log properly what's happening and most importantly throw an error when things definitely won't work out for the caller!
Hope that helps! Please do a lot of testing and post odd things!
One thing that is odd but now I think I can live with since the first error message gives us a clue where the problem is:
(synadm) ~/git/a-0_synadm (notice) $ synadm user details 'testuser::'
ERROR Neither an MXID nor a proper localpart was passed.
WARNING Synapse returned status code 400
errcode: M_INVALID_PARAM
error: Expected UserID string to start with '@'
(synadm) ~/git/a-0_synadm (notice) $
In the case of the notice send command this is behaving very well since we're reacting on the None returned already:
(synadm) ~/git/a-0_synadm (notice) $ synadm notice send 'test::' "plain msg"
ERROR Neither an MXID nor a proper localpart was passed.
The recipient you specified is invalid.
(synadm) ~/git/a-0_synadm (notice) $
So actually this should be easy to fix within all the commands that handle users! I'll take of that myself later. Don't worry.
FYI: To see the log.debug messages while testing you need to pass -vv to the main synadm command.
Also note that I adapted the Returns: docstring to fit the new behaviour.
I put some thought and testing into this and came up with the following:
def generate_mxid(self, user_id): """ Checks whether the given user ID is an MXID already or else generates it from the passed string and the homeserver name fetched via the retrieve_homeserver_name method. Args: user_id (string): User ID given by user as command argument. Returns: string: the fully qualified Matrix User ID (MXID) or None if the user_id parameter is None or no regex matched. """ if user_id is None: self.log.debug("Missing input in generate_mxid().") return None elif re.match(r"^@[-./=\w]+:[-\[\].:\w]+$", user_id): self.log.debug("A proper MXID was passed.") return user_id elif re.match(r"^@?[-./=\w]+:?$", user_id): self.log.debug("A proper localpart was passed, generating MXID " "for local homeserver.") localpart = re.sub("[@:]", "", user_id) mxid = "@{}:{}".format(localpart, self.retrieve_homeserver_name()) return mxid else: self.log.error("Neither an MXID nor a proper localpart was " "passed.") return NoneAlso I realized that the only way to not screw things up with core changes like this is to be very explicit, log properly what's happening and most importantly throw an error when things definitely won't work out for the caller!
Hope that helps! Please do a lot of testing and post odd things!
One thing that is odd but now I think I can live with since the first error message gives us a clue where the problem is:
(synadm) ~/git/a-0_synadm (notice) $ synadm user details 'testuser::' ERROR Neither an MXID nor a proper localpart was passed. WARNING Synapse returned status code 400 errcode: M_INVALID_PARAM error: Expected UserID string to start with '@' (synadm) ~/git/a-0_synadm (notice) $In the case of the notice send command this is behaving very well since we're reacting on the None returned already:
(synadm) ~/git/a-0_synadm (notice) $ synadm notice send 'test::' "plain msg" ERROR Neither an MXID nor a proper localpart was passed. The recipient you specified is invalid. (synadm) ~/git/a-0_synadm (notice) $So actually this should be easy to fix within all the commands that handle users! I'll take of that myself later. Don't worry.
FYI: To see the log.debug messages while testing you need to pass
-vvto the mainsynadmcommand.Also note that I adapted the Returns: docstring to fit the new behaviour.
Seems to work great with the inputs I tested, I'll commit this version of generate_mxid. I also think handling the None output of this function is very necessary anyways, so it's good that we stumbled across it now
Still something in your cojnfirm_prompt functrion needs to be improved. It just doesn't feel correct with some crap inputs that don't match anything. Something like: No users matching the regex pattern.
(synadm) ~/git/a-0_synadm (notice) $ synadm notice send '@testuser.*' "plain msg" -r
Recipients:
- @testuser1:peek-a-boo.at
- @testuser2:peek-a-boo.at
Plaintext message:
---
plain msg
---
Formatted message:
---
plain msg
---
Send now? [y/N]: n
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $
(synadm) ~/git/a-0_synadm (notice) $ synadm notice send 'testuser.*' "plain msg" -r
Recipients:
Plaintext message:
---
plain msg
---
Formatted message:
---
plain msg
---
Send now? [y/N]:
(synadm) ~/git/a-0_synadm (notice) $
Also when trying to send to nothing it just doesn't error out and just does nothing.
(synadm) ~/git/a-0_synadm (notice) $ synadm notice send 'testuser.*' "plain msg" -r
Recipients:
Plaintext message:
---
plain msg
---
Formatted message:
---
plain msg
---
Send now? [y/N]: y
(synadm) ~/git/a-0_synadm (notice) $
It's not clear to the user what's happening....
Still something in your cojnfirm_prompt functrion needs to be improved. It just doesn't feel correct with some crap inputs that don't match anything. Something like: No users matching the regex pattern.
(synadm) ~/git/a-0_synadm (notice) $ synadm notice send '@testuser.*' "plain msg" -r Recipients: - @testuser1:peek-a-boo.at - @testuser2:peek-a-boo.at Plaintext message: --- plain msg --- Formatted message: --- plain msg --- Send now? [y/N]: n (synadm) ~/git/a-0_synadm (notice) $ (synadm) ~/git/a-0_synadm (notice) $ (synadm) ~/git/a-0_synadm (notice) $ (synadm) ~/git/a-0_synadm (notice) $ synadm notice send 'testuser.*' "plain msg" -r Recipients: Plaintext message: --- plain msg --- Formatted message: --- plain msg --- Send now? [y/N]: (synadm) ~/git/a-0_synadm (notice) $
Hmmm that is true, but what would you expect? Output "No user matched the regular expression" and quit?
Also when trying to send to nothing it just doesn't error out and just does nothing.
(synadm) ~/git/a-0_synadm (notice) $ synadm notice send 'testuser.*' "plain msg" -r Recipients: Plaintext message: --- plain msg --- Formatted message: --- plain msg --- Send now? [y/N]: y (synadm) ~/git/a-0_synadm (notice) $It's not clear to the user what's happening....
I mean for the --batch version, that is absolutely what's supposed to happen. Send to all matching users and quit afterwards. As for the UI, I agree the empty receipients list doesn't look like a list in the console, so it may not be clear that it actually says "no recipient matched"...
Maybe I should just add "no recipient matched" below Recipients: if no mxid was printed during iterating? That would make the output a lot more clear without major changes in how the code works and without inflating the code (as would be necessary if it was to be checked whether any recipient matched before starting to build the output string)
Hi @a-0-dev, thanks for that tiny last commit! It does add to usability a lot! in the meantime I've merged to master and fixed and improved some stuff to get ready for releasing this feature. Please have a look and do some testing if you can. If no further issues arise I think I could manage to release it by the end of the week.
These are my changes., review and comments welcome: https://github.com/JOJ0/synadm/compare/0a43837..master
Seems to work just as I would expect, thanks! I'm sorry for bringing up two more topics though :)
- I'm not happy with my own wording of the regex option any more. "--to-regex" very much sounds like synadm would convert something to a regex, which is very misleading (at least it doesn't make any sense, so the user would probably think twice and come to the right conclusion). Maybe rename it to "--regex-to" before the release so it's not changed later on? It sounds a little off/wrong, but the only way to understand it is that TO is a regex - so imho a little more clear and thus better. What do you think?
- I built this in a very scalable way for a reason, it's supposed to be able to send notices to thousands / 10.000s of users without any issue. I identified a potential bottleneck when I first saw the console output of event IDs now (which I like a lot!). It showed me that the
outputslist (api.py L1177) will have as many entries as there are recipients - AND all of it will be printed to the console. Sure, not every server is matrix.org, but I think this may cause issues... What's your opinion? Not collecting the outputs at all? Only collecting some? Another option?
It would be cool to figure out point 1 before the release, point 2 isn't nearly as critical imo. Apart from that, I'd be happy to see the next version of synadm now :)
1 ) I would radically shorten it to just --regex. We shouldn't forget that a user when looking at --help would instantly see the description next do it.
Also in the html rendered docs it would be like that.
What I would also like about that,is in a commandline where the full option is in use, it's just quick to type (I personally often use long options when using commands I'm not very familiar with, It helps reading shell history as well).