Fixes #26714: Migrate WorkflowService to use ZIO #6320
Conversation
ab4ea46 to
3cc9c9d
Compare
|
Commit modified |
3cc9c9d to
46b3ef0
Compare
|
Commit modified |
46b3ef0 to
91a7590
Compare
|
Commit modified |
91a7590 to
81f85b2
Compare
|
Commit modified |
clarktsiory
left a comment
There was a problem hiding this comment.
Most changes look okay, there are some changes that need to be corrected before merging
| savedChangeRequest.toBox match { | ||
| case Full(id) => |
There was a problem hiding this comment.
the error-handling here could be migrated using chainError, fold/either, etc.
| workflowLevelService.getForNodeGroup(CurrentUser.actor, change) match { | ||
| case eb: EmptyBox => | ||
| val msg = s"Error when getting the validation workflow for changes in directive '${change.newGroup.name}'" | ||
| logger.warn(msg, eb) | ||
| JsRaw(s"alert('${msg}')") | ||
|
|
||
| case Full(workflowService) => | ||
| workflowLevelService | ||
| .getForNodeGroup(CurrentUser.actor, change) | ||
| .chainError(s"Error when getting the validation workflow for changes in directive '${change.newGroup.name}'") | ||
| .either | ||
| .runNow match { | ||
| case Left(err) => | ||
| logger.warn(err.fullMsg) | ||
| JsRaw(s"alert('${err.fullMsg}')") | ||
|
|
||
| case Right(workflowService) => |
There was a problem hiding this comment.
The JsRaw alert should not output the full message (containing the details of the exception).
The previous msg should be kept instead
| workflowLevelService.getForGlobalParam(CurrentUser.actor, change) match { | ||
| case eb: EmptyBox => | ||
| val msg = "An error occured when trying to find the validation workflow to use for that change." | ||
| logger.error(msg, eb) | ||
| JsRaw(s"alert('${msg}')") // JsRaw ok, const | ||
| workflowLevelService | ||
| .getForGlobalParam(CurrentUser.actor, change) | ||
| .chainError("An error occured when trying to find the validation workflow to use for that change.") | ||
| .either | ||
| .runNow match { | ||
| case Left(err) => | ||
| logger.error(err.fullMsg) | ||
| JsRaw(s"alert('${err.fullMsg}')") // JsRaw ok, const |
There was a problem hiding this comment.
The previous msg should be kept instead
| def save(changeRequest: ChangeRequest)(implicit cc: ChangeContext): IOResult[ChangeRequest] = { | ||
| implicit val qc: QueryContext = cc.toQuery | ||
|
|
||
| workflowEnabled().toBox.foreach { |
There was a problem hiding this comment.
As soon as the toBox is called, the save that is returning an IOResult is no longer lazy, so the method lacks transparency (we should follow the principle of "referential transparency" by enforcing that a method that returns an IOResult should always be lazy).
So, we should avoid the .toBox.foreach { ... }.toIO here and use the transformations on ZIO
| savedChangeRequest | ||
| .chainError("An error occurred while updating the parameter") | ||
| .fold( | ||
| err => { | ||
| logger.error(err.fullMsg) | ||
| formTracker.addFormError(error(err.fullMsg)) | ||
| onFailure | ||
| }, | ||
| res => res | ||
| ) | ||
| .runNow |
There was a problem hiding this comment.
A fold that does not apply any transformation on the "success" part should be a catchAll instead
|
PR updated with a new commit |
6fea465 to
bd14500
Compare
|
Commit modified |
|
PR rebased |
bd14500 to
9ad63dc
Compare
9ad63dc to
26e197f
Compare
|
Commit modified |
clarktsiory
left a comment
There was a problem hiding this comment.
LGTM ! Tested and the snippets are still rendering correctly, all issues regarding having liftweb code within ZIO effects seem to have been fixed.
|
OK, merging this PR |
42e3e6e
into
Normation:branches/rudder/9.0
https://issues.rudder.io/issues/26714