Fix: Work Item Tracking Process Group Controls management (#1496)

azuredevops/internal/acceptancetests/resource_workitemtrackingprocess_group_test.go

@@ -371,3 +371,70 @@ func groupImportStateIdFunc(resourceName string) resource.ImportStateIdFunc {
 		return fmt.Sprintf("%s/%s/%s/%s/%s", processId, witRefName, pageId, sectionId, groupId), nil
 	}
 }
+
+func TestAccWorkitemtrackingprocessGroup_UpdateControls(t *testing.T) {
+	workItemTypeName := testutils.GenerateWorkItemTypeName()
+	processName := testutils.GenerateResourceName()
+	tfNode := "azuredevops_workitemtrackingprocess_group.test"
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:          func() { testutils.PreCheck(t, nil) },
+		ProviderFactories: testutils.GetProviderFactories(),
+		CheckDestroy:      testutils.CheckProcessDestroyed,
+		Steps: []resource.TestStep{
+			{
+				Config: groupWithControls(workItemTypeName, processName, 1),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(tfNode, "control.#", "1"),
+					resource.TestCheckResourceAttr(tfNode, "control.0.id", "System.Title"),
+				),
+			},
+			{
+				Config: groupWithControls(workItemTypeName, processName, 2),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(tfNode, "control.#", "2"),
+					resource.TestCheckResourceAttr(tfNode, "control.0.id", "System.Title"),
+					resource.TestCheckResourceAttr(tfNode, "control.1.id", "System.Description"),
+				),
+			},
+			{
+				Config: groupWithControls(workItemTypeName, processName, 1),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(tfNode, "control.#", "1"),
+					resource.TestCheckResourceAttr(tfNode, "control.0.id", "System.Title"),
+				),
+			},
+		},
+	})
+}
+
+func groupWithControls(workItemTypeName string, processName string, count int) string {
+	workItemType := basicWorkItemType(workItemTypeName, processName)
+	controls := ""
+	if count >= 1 {
+		controls += `
+  control {
+    id    = "System.Title"
+    label = "Title"
+  }`
+	}
+	if count >= 2 {
+		controls += `
+  control {
+    id    = "System.Description"
+    label = "Description"
+  }`
+	}
+	return fmt.Sprintf(`
+%s
+
+resource "azuredevops_workitemtrackingprocess_group" "test" {
+  process_id                    = azuredevops_workitemtrackingprocess_process.test.id
+  work_item_type_reference_name = azuredevops_workitemtrackingprocess_workitemtype.test.reference_name
+  page_id                       = azuredevops_workitemtrackingprocess_workitemtype.test.pages[0].id
+  section_id                    = azuredevops_workitemtrackingprocess_workitemtype.test.pages[0].sections[0].id
+  label                         = "Test Group"
+  %s
+}
+`, workItemType, controls)
+}

azuredevops/internal/service/workitemtrackingprocess/resource_group.go

@@ -222,37 +222,7 @@ func createResourceGroup(ctx context.Context, d *schema.ResourceData, m any) dia
 	}
 
 	// Add controls to the group if specified
-	if v, ok := d.GetOk("control"); ok {
-		controlList := v.([]interface{})
-		controls := make([]workitemtrackingprocess.Control, len(controlList))
-		for i, c := range controlList {
-			controlMap := c.(map[string]interface{})
-			control := workitemtrackingprocess.Control{
-				Id:       converter.String(controlMap["id"].(string)),
-				Visible:  converter.Bool(controlMap["visible"].(bool)),
-				ReadOnly: converter.Bool(controlMap["read_only"].(bool)),
-			}
-			if label, ok := controlMap["label"].(string); ok {
-				control.Label = converter.String(label)
-			}
-			// Use the list index as the order
-			control.Order = converter.Int(i)
-			if metadata, ok := controlMap["metadata"].(string); ok {
-				control.Metadata = converter.String(metadata)
-			}
-			if watermark, ok := controlMap["watermark"].(string); ok {
-				control.Watermark = converter.String(watermark)
-			}
-			if isContribution, ok := controlMap["is_contribution"].(bool); ok {
-				control.IsContribution = converter.Bool(isContribution)
-			}
-			if contribution, ok := controlMap["contribution"].([]interface{}); ok && len(contribution) > 0 {
-				control.Contribution = expandContribution(contribution)
-			}
-			controls[i] = control
-		}
-		group.Controls = &controls
-	}
+	group.Controls = expandControls(d)
 
 	args := workitemtrackingprocess.AddGroupArgs{
 		ProcessId:  converter.UUID(d.Get("process_id").(string)),
@@ -278,11 +248,15 @@ func createResourceGroup(ctx context.Context, d *schema.ResourceData, m any) dia
 
 	d.SetId(*createdGroup.Id)
 
-	if group.Controls == nil || len(*group.Controls) == 0 {
+	expectedControlCount := 0
+	if group.Controls != nil {
+		expectedControlCount = len(*group.Controls)
+	}
+
+	if expectedControlCount == 0 {
 		return readResourceGroup(ctx, d, m)
 	}
 
-	expectedControlCount := len(*group.Controls)
 	// NOTE: Adding contribution controls (and potentially other controls) have eventual
 	// consistency issues, so we need to retry reading the resource until all expected
 	// controls are present. We use ContinuousTargetOccurence to ensure the count is
@@ -350,12 +324,13 @@ func readResourceGroupWithError(ctx context.Context, d *schema.ResourceData, m a
 	d.Set("visible", foundGroup.Visible)
 
 	// Read controls if present
+	controls := make([]map[string]interface{}, 0)
 	if foundGroup.Controls != nil && len(*foundGroup.Controls) > 0 {
 		groupControls := *foundGroup.Controls
 		slices.SortStableFunc(groupControls, func(a, b workitemtrackingprocess.Control) int {
 			return cmp.Compare(converter.ToInt(a.Order, math.MaxInt), converter.ToInt(b.Order, math.MaxInt))
 		})
-		controls := make([]map[string]interface{}, len(groupControls))
+		controls = make([]map[string]interface{}, len(groupControls))
 		for i, c := range groupControls {
 			control := map[string]interface{}{
 				"visible":    c.Visible,
@@ -389,9 +364,9 @@ func readResourceGroupWithError(ctx context.Context, d *schema.ResourceData, m a
 			}
 			controls[i] = control
 		}
-		if err := d.Set("control", controls); err != nil {
-			return err
-		}
+	}
+	if err := d.Set("control", controls); err != nil {
+		return err
 	}
 	return nil
 }
@@ -406,8 +381,9 @@ func updateResourceGroup(ctx context.Context, d *schema.ResourceData, m any) dia
 	sectionId := d.Get("section_id").(string)
 
 	updateGroup := &workitemtrackingprocess.Group{
-		Label:   converter.String(d.Get("label").(string)),
-		Visible: converter.Bool(d.Get("visible").(bool)),
+		Label:    converter.String(d.Get("label").(string)),
+		Visible:  converter.Bool(d.Get("visible").(bool)),
+		Controls: expandControls(d),
 	}
 	//nolint:staticcheck // SA1019: d.GetOkExists is deprecated but required to distinguish between unset and zero value
 	if v, ok := d.GetOkExists("order"); ok {
@@ -433,28 +409,56 @@ func updateResourceGroup(ctx context.Context, d *schema.ResourceData, m any) dia
 		if err != nil {
 			return diag.Errorf(" Moving group. Error %+v", err)
 		}
+	} else {
+		args := workitemtrackingprocess.UpdateGroupArgs{
+			ProcessId:  converter.UUID(processId),
+			WitRefName: converter.String(witRefName),
+			PageId:     converter.String(pageId),
+			SectionId:  converter.String(sectionId),
+			GroupId:    &groupId,
+			Group:      updateGroup,
+		}
 
-		return readResourceGroup(ctx, d, m)
+		_, err := clients.WorkItemTrackingProcessClient.UpdateGroup(ctx, args)
+		if err != nil {
+			return diag.Errorf(" Update group. Error %+v", err)
+		}
 	}
 
-	args := workitemtrackingprocess.UpdateGroupArgs{
-		ProcessId:  converter.UUID(processId),
-		WitRefName: converter.String(witRefName),
-		PageId:     converter.String(pageId),
-		SectionId:  converter.String(sectionId),
-		GroupId:    &groupId,
-		Group:      updateGroup,
+	expectedControlCount := 0
+	if updateGroup.Controls != nil {
+		expectedControlCount = len(*updateGroup.Controls)
 	}
 
-	err := utils.RetryOnUnexpectedException(ctx, d.Timeout(schema.TimeoutUpdate), func() error {
-		_, err := clients.WorkItemTrackingProcessClient.UpdateGroup(ctx, args)
-		return err
-	})
-	if err != nil {
-		return diag.Errorf(" Update group. Error %+v", err)
+	// NOTE: We only wait if controls are present in the configuration.
+	// If the user removed the control block, we don't wait (as per the logic in expandControls returning nil).
+	if expectedControlCount == 0 {
+		return readResourceGroup(ctx, d, m)
 	}
 
-	return readResourceGroup(ctx, d, m)
+	stateConf := &retry.StateChangeConf{
+		Pending:                   []string{"waiting"},
+		Target:                    []string{"ready", "error"},
+		Timeout:                   d.Timeout(schema.TimeoutUpdate),
+		MinTimeout:                retryMinTimeout,
+		ContinuousTargetOccurence: 4,
+		Refresh: func() (interface{}, string, error) {
+			if err := readResourceGroupWithError(ctx, d, m); err != nil {
+				return nil, "error", err
+			}
+			actualControlCount := 0
+			if v, ok := d.GetOk("control"); ok {
+				actualControlCount = len(v.([]any))
+			}
+			log.Printf("[DEBUG] Expected control count: %d, Actual control count: %d", expectedControlCount, actualControlCount)
+			if actualControlCount < expectedControlCount {
+				return nil, "waiting", nil
+			}
+			return d, "ready", nil
+		},
+	}
+	_, err := stateConf.WaitForStateContext(ctx)
+	return diag.FromErr(err)
 }
 
 func deleteResourceGroup(ctx context.Context, d *schema.ResourceData, m any) diag.Diagnostics {
@@ -528,3 +532,38 @@ func findGroupInSection(section *workitemtrackingprocess.Section, groupId string
 	}
 	return nil
 }
+
+func expandControls(d *schema.ResourceData) *[]workitemtrackingprocess.Control {
+	if v, ok := d.GetOk("control"); ok {
+		controlList := v.([]interface{})
+		controls := make([]workitemtrackingprocess.Control, len(controlList))
+		for i, c := range controlList {
+			controlMap := c.(map[string]interface{})
+			control := workitemtrackingprocess.Control{
+				Id:       converter.String(controlMap["id"].(string)),
+				Visible:  converter.Bool(controlMap["visible"].(bool)),
+				ReadOnly: converter.Bool(controlMap["read_only"].(bool)),
+			}
+			if label, ok := controlMap["label"].(string); ok {
+				control.Label = converter.String(label)
+			}
+			// Use the list index as the order
+			control.Order = converter.Int(i)
+			if metadata, ok := controlMap["metadata"].(string); ok {
+				control.Metadata = converter.String(metadata)
+			}
+			if watermark, ok := controlMap["watermark"].(string); ok {
+				control.Watermark = converter.String(watermark)
+			}
+			if isContribution, ok := controlMap["is_contribution"].(bool); ok {
+				control.IsContribution = converter.Bool(isContribution)
+			}
+			if contribution, ok := controlMap["contribution"].([]interface{}); ok && len(contribution) > 0 {
+				control.Contribution = expandContribution(contribution)
+			}
+			controls[i] = control
+		}
+		return &controls
+	}
+	return nil
+}

azuredevops/internal/service/workitemtrackingprocess/resource_group_test.go

@@ -313,6 +313,7 @@ func TestGroup_Read_Successful(t *testing.T) {
 		"order":                         strconv.Itoa(order),
 		"visible":                       strconv.FormatBool(visible),
 		"id":                            groupId,
+		"control.#":                     "0",
 	}
 	diffOptions := []cmp.Option{
 		cmpopts.EquateEmpty(),
@@ -322,6 +323,77 @@ func TestGroup_Read_Successful(t *testing.T) {
 	}
 }
 
+func TestGroup_Update_SuccessfulWithControls(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	mockClient := azdosdkmocks.NewMockWorkitemtrackingprocessClient(ctrl)
+	clients := &client.AggregatedClient{WorkItemTrackingProcessClient: mockClient, Ctx: context.Background()}
+
+	processId := uuid.New()
+	witRefName := "MyNewAgileProcess.MyWorkItemType"
+	pageId := "page-1"
+	sectionId := "section-1"
+	groupId := "group-1"
+	label := "My Group"
+	order := 1
+	visible := true
+
+	controlId := "System.Title"
+	controlLabel := "Title"
+
+	returnGroup := &workitemtrackingprocess.Group{
+		Id:      &groupId,
+		Label:   &label,
+		Order:   &order,
+		Visible: &visible,
+		Controls: &[]workitemtrackingprocess.Control{
+			{
+				Id:    &controlId,
+				Label: &controlLabel,
+			},
+		},
+	}
+
+	// Mocking MoveGroupToPage because TestResourceDataRaw makes everything "changed"
+	mockClient.EXPECT().MoveGroupToPage(clients.Ctx, gomock.Any()).DoAndReturn(
+		func(ctx context.Context, args workitemtrackingprocess.MoveGroupToPageArgs) (*workitemtrackingprocess.Group, error) {
+			assert.NotNil(t, args.Group.Controls)
+			assert.Len(t, *args.Group.Controls, 1)
+			assert.Equal(t, controlId, *(*args.Group.Controls)[0].Id)
+			return returnGroup, nil
+		},
+	).Times(1)
+
+	returnWorkItemType := createProcessWorkItemTypeWithGroup(witRefName, pageId, sectionId, *returnGroup)
+
+	mockClient.EXPECT().GetProcessWorkItemType(clients.Ctx, gomock.Any()).DoAndReturn(
+		func(ctx context.Context, args workitemtrackingprocess.GetProcessWorkItemTypeArgs) (*workitemtrackingprocess.ProcessWorkItemType, error) {
+			return returnWorkItemType, nil
+		},
+	).Times(4)
+
+	d := getGroupResourceData(t, map[string]any{
+		"process_id":                    processId.String(),
+		"work_item_type_reference_name": witRefName,
+		"page_id":                       pageId,
+		"section_id":                    sectionId,
+		"label":                         label,
+		"order":                         order,
+		"visible":                       visible,
+		"control": []any{
+			map[string]any{
+				"id":    controlId,
+				"label": controlLabel,
+			},
+		},
+	})
+	d.SetId(groupId)
+
+	diags := updateResourceGroup(context.Background(), d, clients)
+	assert.Empty(t, diags)
+}
+
 func TestGroup_FindGroupById(t *testing.T) {
 	groupId := "target-group"
 	pageId1 := "page-1"
@@ -657,6 +729,7 @@ func TestGroup_Update_Successful(t *testing.T) {
 		"order":                         strconv.Itoa(order),
 		"visible":                       strconv.FormatBool(visible),
 		"id":                            groupId,
+		"control.#":                     "0",
 	}
 	diffOptions := []cmp.Option{
 		cmpopts.EquateEmpty(),