#193 The cmdlet Add-IshBackgroundTask is extended with InputDataTempl… #195

ArianArt · 2024-06-21T14:22:47Z

…ate and HashId.

ddemeyer

End of Friday review to attempt an unblock. Chose this issue over the older one ;-) as I think this one is more important.

Remember, ISHRemote works across all, so should remain backward compatible.

ddemeyer · 2024-06-21T14:30:10Z

Source/ISHRemote/Trisoft.ISHRemote/Cmdlets/BackgroundTask/AddIshBackgroundTask.Tests.ps1

@@ -18,6 +18,8 @@ Describe "Add-IshBackgroundTask" -Tags "Create" {
 		$global:ishBackgroundTaskCmdlet = Add-IshFolder -IShSession $ishSession -ParentFolderId $folderIdTestRootOriginal -FolderType $folderTypeTestRootOriginal -FolderName $cmdletName -OwnedBy $ownedByTestRootOriginal -ReadAccess $readAccessTestRootOriginal
 		$ishFolderTopic = Add-IshFolder -IshSession $ishSession -ParentFolderId($global:ishBackgroundTaskCmdlet.IshFolderRef) -FolderType ISHModule -FolderName "Topic" -OwnedBy $ownedByTestRootOriginal -ReadAccess $readAccessTestRootOriginal

+        $inputMetadataField = Set-IshRequestedMetadataField -IshSession $ishSession -Level Task -Name INPUTDATAID


Move variable down to a new BeforeAll block in section Context "Add-IshBackgroundTask IshObjectsGroup Pipeline IshObject with InputDataTemplate" { ... please also rename $inputMetadataField to $requestedMetadata

ddemeyer · 2024-06-21T14:32:39Z

Source/ISHRemote/Trisoft.ISHRemote/Cmdlets/BackgroundTask/AddIshBackgroundTask.Tests.ps1

@@ -128,6 +130,69 @@ Describe "Add-IshBackgroundTask" -Tags "Create" {
 			$ishSession.MetadataBatchSize = $savedMetadataBatchSize
 		}
 	}
+
+    Context "Add-IshBackgroundTask IshObjectsGroup Pipeline IshObject with InputDataTemplate" {
+        if(([Version]$ishSession.ServerVersion).Major -ge 15 -or (([Version]$ishSession.ServerVersion).Major -ge 14 -and ([Version]$ishSession.ServerVersion).Revision -ge 4)) {


Just like in #175 ...

Pesters tests need to have if clauses WITHIN the Pester It scope, not around.... see https://pester.dev/docs/usage/discovery-and-run

I corrected it :-) I mistakenly thought that this condition could cover the test context

ddemeyer · 2024-06-21T14:39:58Z

Source/ISHRemote/Trisoft.ISHRemote/Cmdlets/BackgroundTask/AddIshBackgroundTask.Tests.ps1

+        if(([Version]$ishSession.ServerVersion).Major -ge 15 -or (([Version]$ishSession.ServerVersion).Major -ge 14 -and ([Version]$ishSession.ServerVersion).Revision -ge 4)) {
+            It "Pipeline IshObject with LogicalId IshObjectsWithLngRef" {
+                $backgroundTask = $ishObjects | Add-IshBackgroundTask -IshSession $ishSession -EventType "TESTBACKGROUNDTASK" -InputDataTemplate IshObjectsWithLngRef |
+                Get-IshBackgroundTask -IshSession $ishSession -RequestedMetadata $inputMetadataField


Purpose of a superfluous Get-IshBackgroundTask ... as the Add-IshBackgroundTask should already return a good value to the pipeline.... At least add a comment for that you want to retrieve system metadata which typically is not returned

ddemeyer · 2024-06-21T14:42:31Z

Source/ISHRemote/Trisoft.ISHRemote/Cmdlets/BackgroundTask/AddIshBackgroundTask.cs

 using Trisoft.ISHRemote.BackgroundTask25ServiceReference;
 using Trisoft.ISHRemote.EventMonitor25ServiceReference;
 using System.Text;
+using System.Xml.Linq;
+using Microsoft.IdentityModel.Tokens;


Purpose of this reference?

It's for IsNullOrEmpty. -> EventDescription.IsNullOrEmpty()

ddemeyer · 2024-06-21T14:46:14Z

Source/ISHRemote/Trisoft.ISHRemote/Cmdlets/BackgroundTask/AddIshBackgroundTask.cs

@@ -46,13 +50,30 @@ namespace Trisoft.ISHRemote.Cmdlets.BackgroundTask
    /// New-IshSession -WsBaseUrl "https://example.com/ISHWS/" -PSCredential "Admin"


New sample should not write
New-IshSession -WsBaseUrl "https://example.com/ISHWS/" -PSCredential "Admin"
but
New-IshSession -WsBaseUrl "https://example.com/ISHWS/"
Same for other samples in this file. Refactoring other files is a separate story, at least make the new samples good

ddemeyer · 2024-06-21T15:25:40Z

Source/ISHRemote/Trisoft.ISHRemote/Cmdlets/BackgroundTask/AddIshBackgroundTask.cs

                    // Create BackgroundTask
-                    WriteDebug($"Create BackgroundTask EventType[{EventType}] RawInputData.length[{RawInputData.Length}] StartAfter[{StartAfter}]");
+                    WriteDebug($"Create BackgroundTask EventType[{EventType}] RawInputData.length[{inputData}] StartAfter[{StartAfter}]");


RawInputData.Length[{inputData}

should be backgroundTaskInputData given line 314

should be with .Length as we don't want to print 10000 bytes of text here
The before read WriteDebug($"Create BackgroundTask EventType[{EventType}] RawInputData.length[{RawInputData.Length}] StartAfter[{StartAfter}]");

Resolved with the following
WriteDebug(ParameterSetName == "IshObjectsGroup" ?
$"Create BackgroundTask EventType[{EventType}] InputData.length[{inputData.Length}] StartAfter[{StartAfter}]" :
$"Create BackgroundTask EventType[{EventType}] RawInputData.length[{RawInputData.Length}] StartAfter[{StartAfter}]");

ddemeyer · 2024-06-21T15:27:03Z

Source/ISHRemote/Trisoft.ISHRemote/Cmdlets/BackgroundTask/AddIshBackgroundTask.cs

-                    // Create BackgroundTask 
-                    WriteDebug($"Create BackgroundTask EventType[{EventType}] RawInputData.length[{RawInputData.Length}]");
+                    // Create BackgroundTask
+                    WriteDebug($"Create BackgroundTask EventType[{EventType}] RawInputData.length[{inputData}] StartAfter[{StartAfter}]");


Why print StartAfter value, when two lines up you checked it does not have a value... please do similar correction as higher.
Orginal was WriteDebug($"Create BackgroundTask EventType[{EventType}] RawInputData.length[{RawInputData.Length}]");

ddemeyer · 2024-06-21T15:32:28Z

Source/ISHRemote/Trisoft.ISHRemote/Cmdlets/TrisoftCmdlet.cs

 using Trisoft.ISHRemote.Folder25ServiceReference;
-using System.Runtime.InteropServices;
+using Trisoft.ISHRemote.Interfaces;


Surprised why removing a function, which I assume was not used (DevideListInBatchesByLogicalId) results in 3 extra using statements

It's not extra, the using section is ordered

ddemeyer · 2024-06-21T15:37:45Z

Source/ISHRemote/Trisoft.ISHRemote/Cmdlets/BackgroundTask/AddIshBackgroundTask.cs

        #region Private fields
        private readonly List<IshObject> _retrievedIshObjects = new List<IshObject>();
        private readonly DateTime _modifiedSince = DateTime.Today.AddDays(-1);
-        private readonly BackgroundTask25ServiceReference.eUserFilter _userFilter = EnumConverter.ToUserFilter<BackgroundTask25ServiceReference.eUserFilter>(Enumerations.UserFilter.Current);
+        private readonly eUserFilter _userFilter = EnumConverter.ToUserFilter<eUserFilter>(Enumerations.UserFilter.Current);


I expected an _inputDataTemplate here, like this EnumConverter.ToUserFi... line. Because you made InputDataTemplate a non-mandatory parameter, so if not specified it defaults to...?
Add a default for IshObjectsWithLngRef because any customer that already has a script that reads Add-IshBackgroundTask -EventType "SMARTTAG" should not break

ddemeyer · 2024-06-21T15:40:06Z

Source/ISHRemote/Trisoft.ISHRemote/Cmdlets/BackgroundTask/AddIshBackgroundTask.cs

@@ -46,13 +50,30 @@ namespace Trisoft.ISHRemote.Cmdlets.BackgroundTask
    /// New-IshSession -WsBaseUrl "https://example.com/ISHWS/" -PSCredential "Admin"
    /// $ishBackgroundTask = Get-IshFolder -FolderPath "General\Myfolder" -FolderTypeFilter @("ISHModule", "ISHMasterDoc", "ISHLibrary") -Recurse |
    ///                      Get-IshFolderContent -VersionFilter Latest -LanguagesFilter en |
-    ///                      Add-IshBackgroundTask -EventType "SMARTTAG"
+    ///                      Add-IshBackgroundTask -EventType "SMARTTAG" -InputDataTemplate IshObjectsWithLngRef


Remove the explicit added -InputDataTemplate IshObjectsWithLngRef. And enrich the help and this sample description to indicate that the default value of this parameter is `IshObjectsWithLngRef .

HildeVermeiren · 2024-06-24T12:16:22Z

Source/ISHRemote/Trisoft.ISHRemote/Cmdlets/BackgroundTask/AddIshBackgroundTask.cs

+        /// <para type="description">The InputDataTemplate (e.g. IshObjectWithLngRef) indicates whether a list of ishObjects or one ishObject is submitted as input data to the background task.</para>
+        /// </summary>
+        [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = false, ParameterSetName = "IshObjectsGroup")]
+        public InputDataTemplate InputDataTemplate { get; set; } = InputDataTemplate.IshObjectsWithIshRef;


The default should be "IshObjectsWithLngRef"

HildeVermeiren · 2024-06-24T12:33:18Z

Source/ISHRemote/Trisoft.ISHRemote/Cmdlets/BackgroundTask/AddIshBackgroundTask.cs

-                    var createBackgroundTaskResponse = IshSession.BackgroundTask25.CreateBackgroundTask(newBackgroundTaskRequest);
-                    var progressId = createBackgroundTaskResponse.progressId;
+                    var createBackgroundTaskStartAfterResponse = IshSession.BackgroundTask25.CreateBackgroundTask(newBackgroundTaskRequest);
+                    var progressId = createBackgroundTaskStartAfterResponse.progressId;
                    progressIds.Add(progressId);


I don't understand why we add this single value progressId to a list. Event with the DocumenObj25 call we are returning 1 taskId and 1 progressId. So can we remove this list and the code "if (progressIds.Count > 1)"
and for the following part an "FilterOperator.equal" and "valuetype.value" are slightly better:
"new IshMetadataFilterField(FieldElements.BackgroundTaskProgressId,
Enumerations.Level.Task, Enumerations.FilterOperator.In, progressIds.First().ToString(),
Enumerations.ValueType.Element)"

…tests.

OlegTokar · 2024-06-25T14:52:01Z

Source/ISHRemote/Trisoft.ISHRemote/Cmdlets/BackgroundTask/AddIshBackgroundTask.cs

+                metadataFilter.AddField(new IshMetadataFilterField(FieldElements.BackgroundTaskProgressId,
+                    Level.Task, FilterOperator.In, progressId.ToString(),
+                    Enumerations.ValueType.Value));
+


Redundant new line (2 in a row)

OlegTokar · 2024-06-25T16:29:33Z

Source/ISHRemote/Trisoft.ISHRemote/Cmdlets/BackgroundTask/AddIshBackgroundTask.cs

@@ -35,32 +39,49 @@ namespace Trisoft.ISHRemote.Cmdlets.BackgroundTask
    /// </summary>


Do we still need to mention DocumentObj25.RaiseEventByIshLngRefs in the description?

I removed it and corrected the whole description.

OlegTokar · 2024-06-26T05:22:39Z

Source/ISHRemote/Trisoft.ISHRemote/Cmdlets/BackgroundTask/AddIshBackgroundTask.cs

+    /// $ishBackgroundTask = Get-IshDocumentObj -LogicalId "GUID-ABCD-1234" |
+    ///                      Add-IshBackgroundTask -EventType "SYNCHRONIZEMETRICS" -InputDataTemplate IshObjectWithIshRef
+    /// </code>
+    /// <para>Add BackgroundTask with event type "SYNCHRONIZEMETRICS" for the latest-version of a single content object of type topic; located under the "General\MyFolder\Topics" path. Trigger an event of synchronizing a topic from CM to the metrics.</para> 


We do not use folder path "General\MyFolder\Topics" in the example

All review items are applied

#193 The cmdlet Add-IshBackgroundTask is extended with InputDataTempl…

5b4d931

…ate and HashId.

ArianArt requested review from ddemeyer, HildeVermeiren and OlegTokar June 21, 2024 14:22

ddemeyer previously requested changes Jun 21, 2024

View reviewed changes

#193 Cmdlet code examples and pester tests are corrected.

b1ab80c

HildeVermeiren requested changes Jun 24, 2024

View reviewed changes

Arian Taghva Manesh added 5 commits June 24, 2024 15:42

#193 The default value for the InputDataTemplate is corrected.

c0f8fa8

#193 The outdated tests are removed from AddIshBackgroundTask pester …

d2e251c

…tests.

#193 Failed pester tests are resolved

35f681d

#193 Failed pester test is resolved

209c741

#193 Failed test is resolved

d8123ff

HildeVermeiren approved these changes Jun 25, 2024

View reviewed changes

OlegTokar reviewed Jun 25, 2024

View reviewed changes

ArianArt requested a review from ddemeyer June 25, 2024 15:02

OlegTokar reviewed Jun 25, 2024

View reviewed changes

OlegTokar reviewed Jun 26, 2024

View reviewed changes

#193 Add-IshBackgroundTask cmdlet description is corrected

69bd517

ArianArt requested review from OlegTokar and HildeVermeiren June 26, 2024 08:42

HildeVermeiren approved these changes Jun 26, 2024

View reviewed changes

OlegTokar approved these changes Jun 26, 2024

View reviewed changes

#193 Pester tests aligned with issue #191 changes.

2b15f66

ArianArt requested review from ddemeyer and removed request for ddemeyer July 1, 2024 06:27

ArianArt removed the request for review from ddemeyer July 1, 2024 06:31

ArianArt merged commit ff0dd43 into master Jul 1, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#193 The cmdlet Add-IshBackgroundTask is extended with InputDataTempl… #195

#193 The cmdlet Add-IshBackgroundTask is extended with InputDataTempl… #195

ArianArt commented Jun 21, 2024

ddemeyer left a comment

ddemeyer Jun 21, 2024

ddemeyer Jun 21, 2024

ArianArt Jun 23, 2024

ddemeyer Jun 21, 2024

ddemeyer Jun 21, 2024

ArianArt Jun 21, 2024

ddemeyer Jun 21, 2024

ddemeyer Jun 21, 2024

ArianArt Jun 23, 2024

ddemeyer Jun 21, 2024

ddemeyer Jun 21, 2024

ArianArt Jun 21, 2024

ddemeyer Jun 21, 2024

ddemeyer Jun 21, 2024

HildeVermeiren Jun 24, 2024

HildeVermeiren Jun 24, 2024

OlegTokar Jun 25, 2024

OlegTokar Jun 25, 2024

ArianArt Jun 26, 2024

OlegTokar Jun 26, 2024

		@@ -46,13 +50,30 @@ namespace Trisoft.ISHRemote.Cmdlets.BackgroundTask
		/// New-IshSession -WsBaseUrl "https://example.com/ISHWS/" -PSCredential "Admin"

		@@ -35,32 +39,49 @@ namespace Trisoft.ISHRemote.Cmdlets.BackgroundTask
		/// </summary>

#193 The cmdlet Add-IshBackgroundTask is extended with InputDataTempl… #195

#193 The cmdlet Add-IshBackgroundTask is extended with InputDataTempl… #195

Conversation

ArianArt commented Jun 21, 2024

ddemeyer 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment