Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PUT Task Variable Admin API is missing #2515

Closed
mauriziovitale opened this Issue Feb 15, 2019 · 17 comments

Comments

Projects
None yet
5 participants
@mauriziovitale
Copy link

mauriziovitale commented Feb 15, 2019

We should have an API that allows an admin to update a task variable. Right now this endpoint is missing

@salaboy salaboy added this to the 7.1.0.M1 milestone Feb 18, 2019

@erdemedeiros erdemedeiros added the api label Feb 19, 2019

@CTI777

This comment has been minimized.

Copy link
Contributor

CTI777 commented Feb 26, 2019

@erdemedeiros @salaboy
We have in RB TaskVariableController, but do not have at all TaskVariableAdminController.
Should we consider what else may be needed?

@erdemedeiros

This comment has been minimized.

Copy link
Member

erdemedeiros commented Feb 26, 2019

@CTI777 yes, please, add it to TaskVariableAdminController.

@CTI777

This comment has been minimized.

Copy link
Contributor

CTI777 commented Feb 27, 2019

@erdemedeiros @salaboy
I have a question regarding RB changes
We have a POST method setVariables (in TaskVariableControllerImpl) which works like set and update...
Update task variables (PUT) should work only in admin area.
How should I continue:

  1. should we allow update in non admin area?
  2. If we will add update method to admin area can I check existing variable using getVariables method? It seems that approach used for admin update process variables is not applicable here…
@salaboy

This comment has been minimized.

Copy link
Member

salaboy commented Feb 27, 2019

as we discussed in the standup I would do as follow:

  1. User (the activiti API internally it should check that the user sending the request is the assignee of the task to perform these actions, if the user sending the request is not the assignee these methods should fail):
    • POST newVariable (only creates a new variable if it doesn't exist, if it exist it should fail)
    • PUT update Variable (only update the value of an existing variable)
  2. Admin (can change any task, no matter if he/she is the assignee or candidate)
    • POST newVariable (only creates a new variable if it doesn't exist, if it exist it should fail)
    • PUT updateVariable (only update the value of an existing variable)

We should remove the method setVariable to be absolutely clear on what the behaviour is.

@CTI777 is it clearer now?

@CTI777

This comment has been minimized.

Copy link
Contributor

CTI777 commented Feb 27, 2019

To conclude (for user and admin area):
get: variables
post: newVariables
put: updateVariables

Question: to check existing task variables can I use getVariables?

@CTI777

This comment has been minimized.

Copy link
Contributor

CTI777 commented Feb 27, 2019

@salaboy
If user want to update / create several task variables, and one variable fail (e.g. try to create already existing var): should operation for all variables fail?

@salaboy

This comment has been minimized.

Copy link
Member

salaboy commented Feb 27, 2019

@CTI777 I am having problems with the plural.. I wonder if we just do newVariable and updateVariable.
Is there any case where we want to update multiple variables or create multiple variables at the same time?
@mauriziovitale can you please clarify?

@mauriziovitale

This comment has been minimized.

Copy link
Author

mauriziovitale commented Feb 27, 2019

@salaboy right now we don't have any specific requirements related to the multiple variables. So we can start with newVariable and updateVariable

@salaboy

This comment has been minimized.

Copy link
Member

salaboy commented Feb 28, 2019

@mauriziovitale awesome! @CTI777 let's try to do that then.. ping me in gitter if you have questions.

@ryandawsonuk

This comment has been minimized.

Copy link
Member

ryandawsonuk commented Feb 28, 2019

Just to confirm current status - there are currently two PRs for the core level on this and we're just starting to look at the cloud level? It looks to me like we're just starting on the cloud level because I don't see any branch on activiti-cloud-runtime-bundle-service and it seems it was just agreed in the last comments what should be done for cloud.

@CTI777

This comment has been minimized.

Copy link
Contributor

CTI777 commented Feb 28, 2019

@ryandawsonuk
I have made currently 2 PR's: API, Activiti. The next will be RB, which is almost ready... I will do it next hours...

@CTI777

This comment has been minimized.

Copy link
Contributor

CTI777 commented Feb 28, 2019

Have made PR for RB.
I still have several questions:

  1. Who can get task variables (implementation in Activiti) - currently no checks for assignee.
    If we will add check for assignee some tests may fail...
  2. It seems that Type of variable is not supported in set/update variable
  3. I do not change task variables Payload: SetTaskVariablesPayload.
    Maybe it is better to call it
    NewTaskVariablePayload
    UpdateTaskVariablePayload
    But it seems that it would be better to refactor it (the changes will affect tests, some other modules)
  4. SetTaskVariableCmdExecutor this is used currently for New Task Variable. To add same for Update Task Variable - will cause problems, because they have same payload type
@erdemedeiros

This comment has been minimized.

Copy link
Member

erdemedeiros commented Feb 28, 2019

@salaboy, @ryandawsonuk and I have some concerns about

We should remove the method setVariable to be absolutely clear on what the behaviour is.

This means a breaking change in the API, so I don't think we should do it for 7.0.x. Probably we should keep existing APIs methods (both Java and REST) as they are and only introduce the two new entries createVariable and updateVariable. Note that in terms of naming I prefer createVariable to newVariable.

@salaboy

This comment has been minimized.

Copy link
Member

salaboy commented Feb 28, 2019

@erdemedeiros totally agree.. so let's not remove the old payloads and methods.. just add new ones with the new payloads as suggested by @CTI777

@CTI777

This comment has been minimized.

Copy link
Contributor

CTI777 commented Feb 28, 2019

@salaboy @erdemedeiros
What about createVariable vs newVariable?
Should I make createVariable method?

@salaboy

This comment has been minimized.

Copy link
Member

salaboy commented Feb 28, 2019

@CTI777 createVariable please.

@CTI777

This comment has been minimized.

Copy link
Contributor

CTI777 commented Feb 28, 2019

@salaboy Ok, will try to complete things today evening...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.