User modify API requires a redesign

[JacksonChen666]

https://github.com/gergelypolonkai/synadm/blob/dba8132dd74fe4be1129a78e07dd26806a6331d8/synadm/api.py#L536-L561

None is not considered truthy, so using None as "default" would cause the values to never change. This makes for a poorly functional API that is also very poorly defined (the severe lack of docs is included).

Basically, we have a problem that has nothing to do with this PR, but the PR has the same problems.
We can merge this PR without fixing the issue, but we definitely need to fix the issue which is more than just user types (trying to remove admin doesn't work because of a similar issue (False will not pass an implicit if condition)).

The user modify API and command works together poorly, unable to remove admin rights (too lazy to write that word). This is because it's using implicit if conditions, where values of False and None is used as defined values, but is interpreted as non-defined values.

Example: you can give admin, not remove. you can set user type (#90), you can't revert to default user type.

Originally posted by @JacksonChen666 in #90 (comment)

[JacksonChen666]

(re)implementation ideas:

• accept dict/json instead?
• accept value on if something is set (that's not so nice to work with)
• define a value is considered undefined and check for that (None can't be used, nor False or True or maybe even arbitrary strings, probably should be a special type)

(i previously put it into #90, the wrong place. sorry!)

[JOJ0]

You could change the initial report here: user-type works now :-) (not the most beautiful solution, but works now at least ;-)

[JOJ0]

Ok so before we continuie with refactoring ideas here I'd like to state one design idea that I think kind of was with 'synadm' from when I started coding it. I wanted api methods to be able to work as closely as the api docs describe them as possible. I wanted api methods to support everything that the api supports or at least stay close to that.

For example: When the docs say that it's possible to "omit" a json text from the post data, the api method should be able to do that as well.

If we take the api in question, let's have a look, we see that a lot of the parts of the json are marked as "optional". In that case leaving them out entirely from the post dasta would mean "leave that setting untouched": https://matrix-org.github.io/synapse/latest/admin_api/user_admin_api.html#create-or-modify-account

With that in mind, let's start to collect ideas.

The first thing that comes up is a simple one and would improve the "falsy" problem: Change all the exclusion conditions to use if not None. eg:

        if avatar_url is not None:
            data.update({"avatar_url": avatar_url})

[JOJ0]

I think with that we would finally fix what I was aiming for in the first place:

• define a value is considered undefined and check for that (None can't be used, nor False or True or maybe even arbitrary strings, probably should be a special type)

Only I'm wondering what kind of value that could be? Simply a string? Named 'undefined'? Sounds ugly, but actually is the functionality we want :-)

[JOJ0]

Reopening. Was closed because the two bugs that were introduced due to this clumsy design were actually (quick) fixed. Please @JacksonChen666 go ahead with a more robust reimplementation of the user_modify method. Appreciated! My personal opinion is that a "good enough" solution would be to just use is not None for the optionality checks, which would fix the "falsy" problem at least. For further "specialities" like we have with user_type='null' a way simply by working around with strings can be used. Not too bad I think but yeah if you would find something cleaner, sure, go ahead, cleaner is always better :-)

[JacksonChen666]

now throwing my thoughts into how the new user modify API should be:

• separate APIs methods for individually modifiable things (lock for lock, deactivate for deactivate) even though it actually hits the same admin API in synapse
• of course, new individual commands that use the new individual APIs
• maybe 1 API for all user profile related stuff like display names and PFPs. maybe.

[JOJ0]

now throwing my thoughts into how the new user modify API should be:

• separate APIs methods for individually modifiable things (lock for lock, deactivate for deactivate) even though it actually hits the same admin API in synapse

• of course, new individual commands that use the new individual APIs

• maybe 1 API for all user profile related stuff like display names and PFPs. maybe.

splitting sounds good. it's a messy thing trying to do too much currently

[n-peugnet]

Just adding my use case as it can't be fulfilled by the current design: I need to remove a 3pid from a user, could this be possible with the redesign of the command?

[JacksonChen666]

Just adding my use case as it can't be fulfilled by the current design: I need to remove a 3pid from a user, could this be possible with the redesign of the command?

Not yet, see checklist.

The plan is to eventually have something, but for now, you still have synadm user modify --threepid (which I'm actually not sure how you're supposed to use).

[n-peugnet]

for now, you still have synadm user modify --threepid (which I'm actually not sure how you're supposed to use).

From what I understand of the docs and my tests, this option does not allow to remove/override the existing 3pids, only to add more:

-t, --threepid <threepid>

Add a third-party identifier. [...]