On 04.02.2013 12:41, Francesco Chicchiriccò wrote:
> On 04/02/2013 12:12, Jan Bernhardt wrote:
>>>
>>>
>>> The workflow-related methods in UserController (and RoleController BTW)
>>> are there because they are involved in user (and role) lifecycle
>>> management.
>>>
>>> Given these things, I wouldn't make any movement.
>> OK, it still doesn't feel right to me, but of course we still can
>> make refactoring's at a later time, when we all feel a greater need
>> to do so.
>
> It is not matter of feeling, it is just incorrect: UserController and
> RoleController make usage of workflow adapters to perform user and
> role operations while the WorkflowController manipulates the workflow
> definition, used to initialize and configure the workflow adapters.
I agree with you that WorkflowController and the workflow methods in
UserController do not belong together. On the other hand I think it may
be a good idea to separate them from the UserService. So would it make
sense to have a UserWorkflowService? This is supported by the fact that
we already have a separate ApprovalRestClient in the client module.
>
>>>> 2. There are quite some many methods capable of handling username and
>>> userId likewise (suspendByUsername, deleteByUsername). As far as I can
>>> see console does not use any of them (*byUsername). From my point of
>>> view, these methods are redundant (except one), since you can always
>>> get
>>> user id by readUser(String username) operation. My proposal would be to
>>> not support these methods in new interface any longer.
>>>
>>> The username-based methods were introduced for making more user-
>>> friendly the invocation of user methods, even from command line
>>> using tools
>>> like curl; the admin console was built before that, and hence it's
>>> using the
>>> former id-based methods.
>>>
>>> I agree that username-based are redundant, but they were introduced to
>>> increase the ease of use, not the efficiency.
>> IMHO I would not add so many convenience functions to core interface,
>> just for tools like curl. Any adaptor of syncope should rather use
>> IDs than (temporary) names. I think these kind of convenience
>> functions would belong to a rich syncope client [SYNCOPE-150], rather
>> than core interface.
>>
>> One problem with username is, that it makes sub-resource access more
>> difficult or increases potential path collisions with other resources.
>>
>> Of course we can keep these methods as they currently are, but I
>> would still like to remove them.
>>
>> Who else has an opinion on this matter?
>
> Adding some bits to this: SYNCOPE-42 and SYNCOPE-91 introduced in
> 1.0.0-incubating such methods that were absent before.
>
> Anyway, I am +-0 to remove such methods in 1.2.0.
Perhaps these convenience methods show that it might be better to have
the user name as a key in the rest service instead of the id.
I am not really sure about this though. When talking with Jan about it
he argued that a user name can change over time.
On the other hand I think the user id we currently have is an internal
thing of our database and may better be hidden to the outside.
One other thing we could consider is using a redirect from the user
based rest resource to the id based resource. So when you do a get on
users/readByUsername/{username} we could redirect to users/{username}.
For the status changing methods like users/activate/{userid} we could
instead use: users/{userId}/status.
You could send put with body active to users/{userId}/status to make a
user active. Or you could get users/{userId}/status to retrieve the
status of a user.
I think it makes sense to think about these things to make the API as
good as possible. On the other hand I think these considerations should
not block the 1.1.0 release.
Best regards
Christian
--
Christian Schneider
http://www.liquid-reality.de
Open Source Architect
http://www.talend.com
