#10849 Liquid syntax support for CorrelateTask #12021

lampersky · 2022-07-16T12:39:44Z

fixes #10849

this is how it works:

correlateTaskLiquid.mp4

/cc @Skrypt

hishamco · 2022-07-16T13:27:35Z

Seems there's a breaking changes

lampersky · 2022-07-16T13:43:52Z

Seems there's a breaking changes

@hishamco no, there is not

here you have recipe from OC before that PR, and you can test it with this PR, and see that, this feature has backward compatibility

Recipe.zip

src/OrchardCore.Modules/OrchardCore.Workflows/Activities/CorrelateTask.cs

github-actions · 2024-01-29T13:27:47Z

This pull request has merge conflicts. Please resolve those before requesting a review.

Piedone

I sincerely apologize for us taking so much time here. I checked out the PR, going over old ones, and this would be quite useful.

How about other activities, see this comment? #10849 (comment)

Is triage still needed here, @Skrypt? And do you also want to review this?

src/OrchardCore.Modules/OrchardCore.Workflows/Activities/CorrelateTask.cs

src/OrchardCore.Modules/OrchardCore.Workflows/Views/Items/CorrelateTask.Fields.Edit.cshtml

Skrypt · 2024-03-14T12:33:46Z

@Piedone maybe we should take a look more often to PR marked with "need triage" on thursday meetings. I've been trying to make @sebastienros to take a look at those by marking these with that tag. I think I had no comment to add on the PR and it just needed triage.

Piedone · 2024-03-14T13:46:35Z

Sure, please bring them up. I think we only need to triage PRs on the meeting when there's something to discuss with the wider audience (this PR is like that, then?), i.e. you as a reviewer aren't sure.

Please keep to these @lampersky:

Apply code suggestions directly so the reviewer doesn't have to eyeball the changes. These resolve themselves after applying them, that's fine.
Don't resolve other conversations so it's easier to track for the reviewer. Then, the reviewer will resolve them.
Feel free to mark conversations that you addressed to keep track of them with an emoji or otherwise, just don't resolve them.
Please keep conversations happening in line comment in those convos, otherwise communication will be a mess. If you have trouble finding them, see this video.
Please click "Re-request review" in the top-right corner for each reviewer when you're ready for another round of review, so they know that you're done.

…elateTask.Fields.Edit.cshtml Co-authored-by: Zoltán Lehóczky <[email protected]>

Piedone

@MikeAlhayek @sebastienros could you please check this during the next triage too?

Piedone · 2024-04-08T00:28:16Z

src/OrchardCore.Modules/OrchardCore.Workflows/Activities/CorrelateTask.cs

+            var value = Syntax switch {
+                "Liquid" => await _expressionEvaluator.EvaluateAsync(Value, workflowContext, null),
+                "JavaScript" => (await _scriptEvaluator.EvaluateAsync(Value, workflowContext, null))?.Trim(),
+                _ => null 


Suggested change

_ => null

_ => throw new NotSupportedException($"The syntax {Syntax} isn't supported for CorrelateTask.")

src/OrchardCore.Modules/OrchardCore.Workflows/Views/Items/CorrelateTask.Fields.Edit.cshtml

src/OrchardCore.Modules/OrchardCore.Workflows/Activities/CorrelateTask.cs

MikeAlhayek · 2024-04-08T02:55:33Z

src/OrchardCore.Modules/OrchardCore.Workflows/Activities/CorrelateTask.cs

@@ -28,14 +30,25 @@ public WorkflowExpression<string> Value
            set => SetProperty(value);
        }

+        public string Syntax
+        {
+            get => GetProperty(() => "JavaScript");


Why the property name here is Synax and the value is JavaScript? Shouldn't the default value be set in the display driver instead?

For preexisting Tasks we need "JavaScript" here, so they'll continue to run without any user interaction.

If enum is used, then we don't need this

src/OrchardCore.Modules/OrchardCore.Workflows/Activities/CorrelateTask.cs

src/OrchardCore.Modules/OrchardCore.Workflows/Drivers/CorrelateTaskDisplayDriver.cs

src/OrchardCore.Modules/OrchardCore.Workflows/ViewModels/CorrelateTaskViewModel.cs

src/OrchardCore.Modules/OrchardCore.Workflows/Views/Items/CorrelateTask.Fields.Edit.cshtml

… into lampersky/fix10849

Piedone · 2024-04-10T16:19:52Z

When you're done addressing all feedback of a review, click "Re-request review" in the top-right corner for each reviewer when you're ready for another round of review, so they know that you're done.

MikeAlhayek · 2024-04-11T05:23:49Z

src/OrchardCore.Modules/OrchardCore.Workflows/Activities/CorrelateTask.cs

@@ -10,12 +11,16 @@ namespace OrchardCore.Workflows.Activities
    public class CorrelateTask : TaskActivity<CorrelateTask>
    {
        private readonly IWorkflowScriptEvaluator _scriptEvaluator;
+        private readonly IWorkflowExpressionEvaluator _expressionEvaluator;


Suggested change

private readonly IWorkflowExpressionEvaluator _expressionEvaluator;

private readonly IWorkflowExpressionEvaluator _expressionEvaluator;

MikeAlhayek · 2024-04-11T05:24:14Z

src/OrchardCore.Modules/OrchardCore.Workflows/Activities/CorrelateTask.cs

        protected readonly IStringLocalizer S;

-        public CorrelateTask(IWorkflowScriptEvaluator scriptEvaluator, IStringLocalizer<CorrelateTask> localizer)
+        public CorrelateTask(IWorkflowScriptEvaluator scriptEvaluator,


Suggested change

public CorrelateTask(IWorkflowScriptEvaluator scriptEvaluator,

public CorrelateTask(

IWorkflowScriptEvaluator scriptEvaluator,

MikeAlhayek · 2024-04-11T05:28:05Z

src/OrchardCore.Modules/OrchardCore.Workflows/Views/Items/CorrelateTask.Fields.Edit.cshtml

+        document.getElementById('@Html.IdFor(x => x.Syntax)').addEventListener("change", (e) => {
+            const syntax = e.target.value;
+
+            if (syntax == '@WorkflowScriptSyntax.JavaScript') {


nameof() if faster than .ToString()

Suggested change

if (syntax == '@WorkflowScriptSyntax.JavaScript') {

if (syntax == '@nameof(WorkflowScriptSyntax.JavaScript)') {

MikeAlhayek · 2024-04-11T05:28:58Z

src/OrchardCore.Modules/OrchardCore.Workflows/Views/Items/CorrelateTask.Fields.Edit.cshtml

+<div class="form-group mb-3" asp-validation-class-for="Syntax">
+    <label asp-for="Syntax" class="form-label">@T["Syntax"]</label>
+    <select asp-for="Syntax" class="form-select">
+        <option value="@WorkflowScriptSyntax.JavaScript">@T["JavaScript"]</option>


Suggested change

<option value="@WorkflowScriptSyntax.JavaScript">@T["JavaScript"]</option>

<option value="@nameof(WorkflowScriptSyntax.JavaScript)">@T["JavaScript"]</option>

MikeAlhayek · 2024-04-11T05:29:20Z

src/OrchardCore.Modules/OrchardCore.Workflows/Views/Items/CorrelateTask.Fields.Edit.cshtml

+    <label asp-for="Syntax" class="form-label">@T["Syntax"]</label>
+    <select asp-for="Syntax" class="form-select">
+        <option value="@WorkflowScriptSyntax.JavaScript">@T["JavaScript"]</option>
+        <option value="@WorkflowScriptSyntax.Liquid">@T["Liquid"]</option>


Suggested change

<option value="@WorkflowScriptSyntax.Liquid">@T["Liquid"]</option>

<option value="@nameof(WorkflowScriptSyntax.Liquid)">@T["Liquid"]</option>

MikeAlhayek · 2024-04-11T05:30:04Z

src/OrchardCore/OrchardCore.Workflows.Abstractions/Models/WorkflowScriptSyntax.cs

@@ -0,0 +1,8 @@
+namespace OrchardCore.Workflows.Models


Can we please use file-scope name spaces?

MikeAlhayek · 2024-04-11T05:30:14Z

src/OrchardCore/OrchardCore.Workflows.Abstractions/Helpers/WorkflowScriptSyntaxExtensions.cs

+using System;
+using OrchardCore.Workflows.Models;
+
+namespace OrchardCore.Workflows.Helpers;


Suggested change

namespace OrchardCore.Workflows.Helpers;

namespace OrchardCore.Workflows.Helpers;

MikeAlhayek · 2024-04-11T05:32:03Z

src/OrchardCore.Modules/OrchardCore.Workflows/Views/Items/CorrelateTask.Fields.Edit.cshtml

+                document.getElementById('liquid_hint').classList.add('d-none');
+                document.getElementById('javascript_hint').classList.remove('d-none');
+            } else {
+                editor.setOption('mode', 'liquid');


Is this a valid value for the editor?

MikeAlhayek · 2024-04-11T05:32:28Z

src/OrchardCore.Modules/OrchardCore.Workflows/Views/Items/CorrelateTask.Fields.Edit.cshtml

+<div class="form-group mb-3" asp-validation-class-for="Syntax">
+    <label asp-for="Syntax" class="form-label">@T["Syntax"]</label>
+    <select asp-for="Syntax" class="form-select">
+        <option value="@WorkflowScriptSyntax.JavaScript">@T["JavaScript"]</option>


Suggested change

<option value="@WorkflowScriptSyntax.JavaScript">@T["JavaScript"]</option>

<option value="@nameof(WorkflowScriptSyntax.JavaScript)">@T["JavaScript"]</option>

MikeAlhayek · 2024-04-11T05:32:45Z

src/OrchardCore.Modules/OrchardCore.Workflows/Views/Items/CorrelateTask.Fields.Edit.cshtml

+    <label asp-for="Syntax" class="form-label">@T["Syntax"]</label>
+    <select asp-for="Syntax" class="form-select">
+        <option value="@WorkflowScriptSyntax.JavaScript">@T["JavaScript"]</option>
+        <option value="@WorkflowScriptSyntax.Liquid">@T["Liquid"]</option>


Suggested change

<option value="@WorkflowScriptSyntax.Liquid">@T["Liquid"]</option>

<option value="@nameof(WorkflowScriptSyntax.Liquid)">@T["Liquid"]</option>

MikeAlhayek · 2024-04-11T05:58:40Z

src/OrchardCore.Modules/OrchardCore.Workflows/Views/Items/CorrelateTask.Fields.Edit.cshtml

+        <option value="@WorkflowScriptSyntax.Liquid">@T["Liquid"]</option>
+    </select>
+    <span asp-validation-for="Syntax"></span>
+    <span class="hint">@T["Choose between Liquid or JavaScript syntax."]</span>


I don't think this is useful and can we removed

Suggested change

<span class="hint">@T["Choose between Liquid or JavaScript syntax."]</span>

MikeAlhayek · 2024-04-11T06:00:38Z

src/OrchardCore/OrchardCore.Workflows.Abstractions/Models/WorkflowScriptSyntax.cs

+    public enum WorkflowScriptSyntax
+    {
+        JavaScript,
+        Liquid


Suggested change

Liquid

Liquid,

this will be the first enum with such convention, I've just checked and all enums in the project are without comma after last item, so I will leave it as is

github-actions · 2024-06-21T02:00:10Z

It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up.

Piedone · 2024-06-21T12:57:46Z

@MikeAlhayek this is waiting for your review.

src/OrchardCore.Modules/OrchardCore.Workflows/Views/Items/CorrelateTask.Fields.Edit.cshtml

OrchardCMS#10849 Liquid syntax support for CorrelateTask

4961e20

lampersky requested a review from sfmskywalker as a code owner July 16, 2022 12:39

hishamco reviewed Jul 16, 2022

View reviewed changes

src/OrchardCore.Modules/OrchardCore.Workflows/Activities/CorrelateTask.cs Outdated Show resolved Hide resolved

Skrypt added the needs triage label Jul 19, 2022

github-actions bot added the merge conflict label Jan 29, 2024

Merge branch 'main' into lampersky/fix10849

0b930f7

github-actions bot removed the merge conflict label Jan 31, 2024

Piedone requested changes Mar 13, 2024

View reviewed changes

lampersky and others added 4 commits April 7, 2024 15:06

Update src/OrchardCore.Modules/OrchardCore.Workflows/Views/Items/Corr…

6e9d36c

…elateTask.Fields.Edit.cshtml Co-authored-by: Zoltán Lehóczky <[email protected]>

Update src/OrchardCore.Modules/OrchardCore.Workflows/Views/Items/Corr…

4db7a71

…elateTask.Fields.Edit.cshtml Co-authored-by: Zoltán Lehóczky <[email protected]>

pr feedback addressed

27315ec

clean

31bc8ab

lampersky requested a review from Piedone April 7, 2024 15:07

Piedone approved these changes Apr 8, 2024

View reviewed changes

MikeAlhayek requested changes Apr 8, 2024

View reviewed changes

lampersky added 3 commits April 8, 2024 17:02

pr feedback addressed

86f97a3

pr feedback addressed

6ca6c6e

Merge branch 'main' into lampersky/fix10849

1c3d434

lampersky requested a review from MikeAlhayek April 8, 2024 15:16

lampersky added 2 commits April 9, 2024 09:48

pr feedback addressed

352df94

Merge branch 'lampersky/fix10849' of github.com:lampersky/OrchardCore…

9b6e33f

… into lampersky/fix10849

enum

4599762

lampersky requested a review from hishamco April 10, 2024 21:25

MikeAlhayek reviewed Apr 11, 2024

View reviewed changes

MikeAlhayek requested changes Apr 11, 2024

View reviewed changes

pr feedback addressed

87700c2

lampersky requested a review from MikeAlhayek April 11, 2024 08:22

Piedone removed the needs triage label Apr 21, 2024

github-actions bot added the stale label Jun 21, 2024

MikeAlhayek approved these changes Jun 21, 2024

View reviewed changes

src/OrchardCore.Modules/OrchardCore.Workflows/Views/Items/CorrelateTask.Fields.Edit.cshtml Show resolved Hide resolved

Merge branch 'main' into lampersky/fix10849

6bc00eb

MikeAlhayek enabled auto-merge (squash) June 21, 2024 17:15

MikeAlhayek merged commit f95dc8a into OrchardCMS:main Jun 21, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#10849 Liquid syntax support for CorrelateTask #12021

#10849 Liquid syntax support for CorrelateTask #12021

lampersky commented Jul 16, 2022

hishamco commented Jul 16, 2022

lampersky commented Jul 16, 2022

github-actions bot commented Jan 29, 2024

Piedone left a comment

Skrypt commented Mar 14, 2024

Piedone commented Mar 14, 2024

Piedone left a comment

Piedone Apr 8, 2024

MikeAlhayek Apr 8, 2024

Piedone Apr 8, 2024

MikeAlhayek Apr 8, 2024

Piedone commented Apr 10, 2024

MikeAlhayek Apr 11, 2024

MikeAlhayek Apr 11, 2024

MikeAlhayek Apr 11, 2024

MikeAlhayek Apr 11, 2024

MikeAlhayek Apr 11, 2024

MikeAlhayek Apr 11, 2024

MikeAlhayek Apr 11, 2024

MikeAlhayek Apr 11, 2024

lampersky Apr 11, 2024

MikeAlhayek Apr 11, 2024

MikeAlhayek Apr 11, 2024

MikeAlhayek Apr 11, 2024

MikeAlhayek Apr 11, 2024

lampersky Apr 11, 2024

github-actions bot commented Jun 21, 2024

Piedone commented Jun 21, 2024

	_ => null
	_ => throw new NotSupportedException($"The syntax {Syntax} isn't supported for CorrelateTask.")

	private readonly IWorkflowExpressionEvaluator _expressionEvaluator;
	private readonly IWorkflowExpressionEvaluator _expressionEvaluator;

	public CorrelateTask(IWorkflowScriptEvaluator scriptEvaluator,
	public CorrelateTask(
	IWorkflowScriptEvaluator scriptEvaluator,

	if (syntax == '@WorkflowScriptSyntax.JavaScript') {
	if (syntax == '@nameof(WorkflowScriptSyntax.JavaScript)') {

	<option value="@WorkflowScriptSyntax.JavaScript">@T["JavaScript"]</option>
	<option value="@nameof(WorkflowScriptSyntax.JavaScript)">@T["JavaScript"]</option>

	<option value="@WorkflowScriptSyntax.Liquid">@T["Liquid"]</option>
	<option value="@nameof(WorkflowScriptSyntax.Liquid)">@T["Liquid"]</option>

	namespace OrchardCore.Workflows.Helpers;
	namespace OrchardCore.Workflows.Helpers;

#10849 Liquid syntax support for CorrelateTask #12021

#10849 Liquid syntax support for CorrelateTask #12021

Conversation

lampersky commented Jul 16, 2022

hishamco commented Jul 16, 2022

lampersky commented Jul 16, 2022

github-actions bot commented Jan 29, 2024

Piedone left a comment

Choose a reason for hiding this comment

Skrypt commented Mar 14, 2024

Piedone commented Mar 14, 2024

Piedone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piedone commented Apr 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 21, 2024

Piedone commented Jun 21, 2024