diff --git a/src/aggregator-ruleng/WorkItemRelationWrapperCollection.cs b/src/aggregator-ruleng/WorkItemRelationWrapperCollection.cs index 39d46ef..f533863 100644 --- a/src/aggregator-ruleng/WorkItemRelationWrapperCollection.cs +++ b/src/aggregator-ruleng/WorkItemRelationWrapperCollection.cs @@ -49,8 +49,6 @@ private void AddRelation(WorkItemRelationWrapper item) : null } }); - - _pivotWorkItem.IsDirty = true; } private bool RemoveRelation(WorkItemRelationWrapper item) @@ -68,7 +66,6 @@ private bool RemoveRelation(WorkItemRelationWrapper item) Path = "/relations/-", Value = _original.IndexOf(item) }); - _pivotWorkItem.IsDirty = true; return true; } diff --git a/src/aggregator-ruleng/WorkItemStore.cs b/src/aggregator-ruleng/WorkItemStore.cs index 639d2ad..278da97 100644 --- a/src/aggregator-ruleng/WorkItemStore.cs +++ b/src/aggregator-ruleng/WorkItemStore.cs @@ -132,7 +132,6 @@ private static bool ChangeRecycleStatus(WorkItemWrapper workItem, RecycleStatus workItem.RecycleStatus = toRecycleStatus; var updated = previousStatus != workItem.RecycleStatus; - workItem.IsDirty = updated || workItem.IsDirty; return updated; } @@ -156,8 +155,7 @@ private void ImpersonateChanges() foreach (var workItem in changedWorkItems) { // emit JsonPatchOperation for this field even if the value is unchanged - workItem.ChangedBy = null; - workItem.ChangedBy = _triggerIdentity; + workItem.ForceSetFieldValue(CoreFieldRefNames.ChangedBy, _triggerIdentity); } } diff --git a/src/aggregator-ruleng/WorkItemWrapper.cs b/src/aggregator-ruleng/WorkItemWrapper.cs index 9b7e39f..656f4d3 100644 --- a/src/aggregator-ruleng/WorkItemWrapper.cs +++ b/src/aggregator-ruleng/WorkItemWrapper.cs @@ -12,6 +12,7 @@ public class WorkItemWrapper { private readonly EngineContext _context; private readonly WorkItem _item; + private Dictionary _originalFieldValues; internal WorkItemWrapper(EngineContext context, WorkItem item) { @@ -344,7 +345,9 @@ public double Watermark public bool IsNew => Id is TemporaryWorkItemId; - public bool IsDirty { get; internal set; } + public bool IsDirty => + RecycleStatus != RecycleStatus.NoChange || + Changes.Any(op => op.Operation != Operation.Test); internal RecycleStatus RecycleStatus { get; set; } = RecycleStatus.NoChange; @@ -356,80 +359,49 @@ public object this[string field] set => SetFieldValue(field, value); } - private void SetFieldValue(string field, object value) + private void SetFieldValue(string field, object value, bool force = false) { if (IsReadOnly) { throw new InvalidOperationException("Work item is read-only."); } - if (_item.Fields.ContainsKey(field)) - { - if (_item.Fields[field]?.Equals(value) ?? false) - { - // if new value does not differ from existing value, just ignore change - return; - } + string path = "/fields/" + field; - SetExistingFieldValue(field, value); - } - else - { - SetFieldValueFirstTime(field, value); - } - - IsDirty = true; - } + _originalFieldValues ??= _item.Fields.ToDictionary(kvp => kvp.Key, kvp => kvp.Value); + _originalFieldValues.TryGetValue(field, out var originalValue); - void SetExistingFieldValue(string field, object value) - { - _item.Fields[field] = value; - // do we have a previous op for this field? - var op = Changes.FirstOrDefault(op => op.Path == "/fields/" + field); - if (value == null) + JsonPatchOperation newOp = (originalValue, value) switch { - if (op != null) + (null, null) when force is false => null, + (_, null) => new JsonPatchOperation { - op.Operation = Operation.Remove; - op.Value = null; - } - else + Operation = Operation.Remove, + Path = path, + }, + (null, { }) => new JsonPatchOperation { - Changes.Add(new JsonPatchOperation() - { - Operation = Operation.Remove, - Path = "/fields/" + field, - Value = null - }); - } - } - else - { - if (op != null) - { - op.Value = TranslateValue(value); - } - else - { - Changes.Add(new JsonPatchOperation() + Operation = Operation.Add, + Path = path, + Value = TranslateValue(value), + }, + ({ }, { }) => !force && originalValue.Equals(value) + ? null + : new JsonPatchOperation { Operation = Operation.Replace, - Path = "/fields/" + field, - Value = TranslateValue(value) - }); - } - }//if null value - } + Path = path, + Value = TranslateValue(value), + } + }; - void SetFieldValueFirstTime(string field, object value) - { - _item.Fields.Add(field, value); - Changes.Add(new JsonPatchOperation() + _item.Fields[field] = value; + + Changes.RemoveAll(op => op.Path == path); + if (newOp != null) { - Operation = value == null ? Operation.Remove : Operation.Add, - Path = "/fields/" + field, - Value = TranslateValue(value) - }); + Changes.Add(newOp); + } } private static object TranslateValue(object value) @@ -505,6 +477,11 @@ internal void RemapIdReferences(IDictionary realIds) } } } + + internal void ForceSetFieldValue(string field, object value) + { + SetFieldValue(field, value, force: true); + } } internal enum RecycleStatus diff --git a/src/unittests-ruleng/WorkItemWrapperTests.cs b/src/unittests-ruleng/WorkItemWrapperTests.cs index 7d96db8..6dcbf15 100644 --- a/src/unittests-ruleng/WorkItemWrapperTests.cs +++ b/src/unittests-ruleng/WorkItemWrapperTests.cs @@ -122,6 +122,7 @@ public void AssignDestinationFieldAndSourceFieldIsMissing() { { "System.WorkItemType", "Task" }, { "System.Title", "The Child" }, + { CustomField, "Some value" }, }, Url = $"{clientsContext.WorkItemsBaseUrl}/{destWorkItemId}" }; @@ -166,6 +167,7 @@ public void AssignDestinationFieldAndSourceFieldIsNull() { { "System.WorkItemType", "Task" }, { "System.Title", "The Child" }, + { CustomField, "Some value" }, }, Url = $"{clientsContext.WorkItemsBaseUrl}/{destWorkItemId}" }; @@ -246,7 +248,29 @@ public void FieldHasNoValueAndIsNulled() }; var wrapper = new WorkItemWrapper(context, workItem); + Assert.False(wrapper.IsDirty); wrapper[CustomField] = null; + Assert.False(wrapper.IsDirty); + } + + [Fact] + public void FieldHasNoValueAndIsNulledByForce() + { + const string CustomField = "My.Custom"; + int workItemId = 42; + var workItem = new WorkItem + { + Id = workItemId, + Fields = new Dictionary + { + { "System.WorkItemType", "Task" }, + { "System.Title", "The Child" }, + }, + Url = $"{clientsContext.WorkItemsBaseUrl}/{workItemId}" + }; + var wrapper = new WorkItemWrapper(context, workItem); + + wrapper.ForceSetFieldValue(CustomField, null); // first is the /test op Assert.Equal(2, wrapper.Changes.Count); @@ -332,6 +356,32 @@ public void AssignSameValueShouldNotInvalidateIsDirty_Success(object testValue) Assert.False(wrapper.IsDirty); } + [Fact] + public void AssignSameValueByForceShouldResultInChange() + { + WorkItem workItem = new WorkItem + { + Id = 11, + Fields = new Dictionary + { + { "System.WorkItemType", "Bug" }, + { "System.Title", "Hello" }, + }, + Url = $"{clientsContext.WorkItemsBaseUrl}/11" + }; + + var wrapper = new WorkItemWrapper(context, workItem); + + wrapper.ForceSetFieldValue("System.Title", "Hello"); + + // first is the /test op + Assert.Equal(2, wrapper.Changes.Count); + var actual = wrapper.Changes[1]; + Assert.Equal(Operation.Replace, actual.Operation); + Assert.Equal("/fields/System.Title", actual.Path); + Assert.Equal("Hello", actual.Value); + } + [Fact] public void ChangingAFieldWithEnableRevisionCheckOnAddsTestOperation() @@ -397,8 +447,10 @@ public void ChangingAFieldWithEnableRevisionCheckOffHasNoTestOperation() Assert.Equal("Replaced title", actual.Value); } - [Fact] - public void ChangingAPulledFieldTwiceHasASingleReplaceOperation() + [Theory] + [InlineData("Replaced Title")] + [InlineData(null)] + public void ChangingAPulledFieldTwiceHasASingleReplaceOperation(string firstValue) { var logger = Substitute.For(); var ruleSettings = new RuleSettings { EnableRevisionCheck = false }; @@ -418,7 +470,7 @@ public void ChangingAPulledFieldTwiceHasASingleReplaceOperation() }; var wrapper = new WorkItemWrapper(context, workItem); - wrapper.Title = "Replaced title"; + wrapper.Title = firstValue; wrapper.Title = "Replaced title - again"; Assert.Single(wrapper.Changes); @@ -428,8 +480,10 @@ public void ChangingAPulledFieldTwiceHasASingleReplaceOperation() Assert.Equal("Replaced title - again", actual.Value); } - [Fact] - public void ChangingANewFieldTwiceHasASingleAddOperation() + [Theory] + [InlineData("New Reason")] + [InlineData(null)] + public void ChangingANewFieldTwiceHasASingleAddOperation(string firstValue) { var logger = Substitute.For(); var ruleSettings = new RuleSettings { EnableRevisionCheck = false }; @@ -449,7 +503,7 @@ public void ChangingANewFieldTwiceHasASingleAddOperation() }; var wrapper = new WorkItemWrapper(context, workItem); - wrapper.Reason = "New reason"; + wrapper.Reason = firstValue; wrapper.Reason = "New reason - again"; Assert.Single(wrapper.Changes);