On 05/02/2013 10:01, Christian Schneider wrote:
On 04.02.2013 18:10, Francesco Chicchiriccò wrote:
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.
I am still not completely convinced of the rationale for creating such
new UserWorkflowService (and RoleWorkflowService, please don't forget
that also roles are workflow-enabled).
We will end up with UserService / RoleService for read-only operations
and UserWorkflowService / RoleWorkflowService for write-only
operations: does it make sense?
UserService and RoleService contain the full set of crud operations. So
why would they be readonly?
I am not absolutely sure if separating the interfaces makes sense but I
think it could. One indicator is that the UserService always works with
the userId or userName while the workflow methods normally use taskId as
a key. So I think this indicates that the resource we are handling is
not the user but rather a workflow identified by a task. So naming it
UserWorkflowService sounds intuitive to me.
Ok, could you please detail which methods from the current UserService
and RoleService you would like to move into new UserWorkflowService /
RoleWorkflowService?
Why make this distinction for SyncopeUser and not for any other
entity? We "expose" internal ids for everything.
That is true. It would not make too much sense to make an exemption for
User.
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.
As explained in another e-mail thread, it does not make sense to set a
"generic" status for users: "activate" an user is not the same as
setting its status to "active" (and so on): the difference is given by
the workflow definition that any single project could implement to
suit its own needs.
For these reasons, we should try to find a way to express the operations:
* activate()
* suspend()
* reactivate()
in RESTful terms.
But "putting user status" is not an option, IMO.
I will experiment with introducing methods that take a userId or
userName String. So at least we can cut half of the methods for status
changes.
Is it ok to remove the reactivate and use activate with a null token
instead or is this too different in meaning?
They have different meaning and are mapped to different methods on the
underlying UserWorkflowAdapter, so we should keed them distinct.
Regards.
--
Francesco Chicchiriccò
ASF Member, Apache Syncope PMC chair, Apache Cocoon PMC Member
http://people.apache.org/~ilgrosso/
