Hi syncoper,
I do not even see methods in RoleService that should be moved into a different
ServiceInterface! Workflow controller is used in RoleController but just for
CRUD operations, no direct access, so this is mostly transparent to user.
But I completely agree with Christian, that direct workflow operations
identified by TaskID should be moved out of UserService. They are used in
context of users (currently) but can be used also in other contexts. This is
why I would move the following method to WorkflowService:
@POST
@Path("workflow/tasks/{taskId}/claim")
WorkflowFormTO claimForm(@PathParam("taskId") String taskId);
@POST
@Path("workflow/tasks/{taskId}/execute")
UserTO executeWorkflow(@PathParam("taskId") String taskId, UserTO userTO);
@GET
@Path("workflow/form")
List<WorkflowFormTO> getForms();
@POST
@Path("workflow/form")
UserTO submitForm(WorkflowFormTO form);
Since this next method contains a userId but is related to workflow activity,
I'm not sure what to do with it. I guess is should remain in UserService, to
not have an dependency from WorkflowService to UserService.
@GET
@Path("{userId}/workflow/form")
WorkflowFormTO getFormForUser(@PathParam("userId") Long userId);
Best regards.
Jan
> -----Original Message-----
> From: Francesco Chicchiriccò [mailto:ilgro...@apache.org]
> Sent: Dienstag, 5. Februar 2013 11:03
> To: dev@syncope.apache.org
> Subject: Re: [DISCUSS] User Service
>
> 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/
