User modify command threepid handling fixes #136
Conversation
as well as rewriting help of `--threepid` opton.
Actually Click errors out if `--threpid` is passed without arguments _or_ if it is not exactly two arguments. No need for this empty tuple check. It will never be true.
to prevent WARNING when user wants to clear by passing `--threepid '' ''`.
|
One more uglyness remains with this quick and dirty implementation. If the user passes some valid arg-pairs alongside one empty-string-pair, the sanity check still complains: Is that bad? Or ist that actually good (because bad arguments where passed anyway)?. |
|
@JacksonChen666 I know there is some rather dirty fixes in this PR. Some nitpicking on my help texts appreciated! Or maybe you have some better ideas? Or we decide that it is good enough for now? @n-peugnet if you have a minute: Can you install from this branch and test it out in terms of "usability"? |
synadm/cli/user.py
Outdated
| value (eg. --threepid email <user@example.org>). This option can also | ||
| be stated multiple times, i.e. a user can have multiple threepids | ||
| configured.""") | ||
| help="""Add a third-party identifier (email address or phone number). |
There was a problem hiding this comment.
So does it add 3pids or replace them in the end?
There was a problem hiding this comment.
Ah yeah, thanks for that catch! It replaces for existing accounts. That was the reason I was rewriting this help text in the first place. The problem with wording right now is that this user modify command is used to create as well as alter user accounts. Suggestions how to write this concisely?
Maybe start like that:
Set a third party identifier.....
I just tested it. It works as expected, but it is a little bit strange to use. Maybe a |
synadm/cli/user.py
Outdated
| help="""Add a third-party identifier (email address or phone number). | ||
| Threepids are used for several things: For use when logging in, as an | ||
| alternative to the user id; in the case of email, as an alternative contact | ||
| to help with account recovery; as well as to receive notifications of |
There was a problem hiding this comment.
should be comma not semicolon!
Yes I also thought about a separate clear threepid option. The thing with that is that it's a weird option to have when creating an account. Well, actually, it would just be ignored in that case. All these weirdnesses are some of the reasons for the redesign approaches you already noticed ;-) |
- State usage information first thing; Description of what it actually is, state last. - Remove information of clearing in perparation of separate --clear-threepids option. - Explicitely state that existing users will get all threepids replaced!
- Removes all threepids. - Takes precedence over all passed --threepid options. - It's not separately catched and warned that it doesn't make sense to provide this together with --threepid. Decision made to keep code less populated (it _is_ already too much...). - Since api.user_modify is also already huge, decide to keep "threepid clearing" code as-is (passing empty strings in tuple-tuple clear. Period.) - Fix format of hard to read helper.api.user_modify() statement.
We don't only modify users with this command we also create them - fix all accourences of "modify" to additionally state "create".
|
I finally decided to add a separate option I also gave the help texts another rewrite and reordering. Hope I got everything to be told into it finally. Also some fixes in wording of the "user interaction" messages. The command always stated that "something is modified" even though it could also be just freshly "created" Anyway there is room for improvement / redesign in all this. We know...we know...known issue... :-) |
Fixes #135
Improve the
--threepidoption of theuser modifycommand.--clear-threepidsNote: If a user has the idea/follows the logic that resetting according to the API docs (https://matrix-org.github.io/synapse/latest/admin_api/user_admin_api.html#create-or-modify-account) should also work by setting it to "empty", ie.
--threepid '' '', that will also work, but it is not documented in--help!--helpfor this option.--threepidoptions, sets only the last of the argument pairs the user had provided on the cli (ignoring all others).