Merge pull request #1641 from influxdata/bugfix/enable-disable

Fix enable / disable bug

CHANGELOG.md

@@ -13,6 +13,10 @@
 
 ## v1.3.3.1 [2017-06-21]
 ### Bug Fixes
+1. [#1641](https://github.com/influxdata/chronograf/pull/1641): Fix enable / disable being out of sync on Kapacitor Rules Page
+
+### Features
+### UI Improvements
 1. [#1642](https://github.com/influxdata/chronograf/pull/1642): Do not prefix basepath to external link for news feed
 
 ## v1.3.3.0 [2017-06-19]

kapacitor/client.go

@@ -171,16 +171,25 @@ func (c *Client) AllStatus(ctx context.Context) (map[string]string, error) {
 
 // Status returns the status of a task in kapacitor
 func (c *Client) Status(ctx context.Context, href string) (string, error) {
-	kapa, err := c.kapaClient(c.URL, c.Username, c.Password)
+	s, err := c.status(ctx, href)
-	if err != nil {
-		return "", err
-	}
-	task, err := kapa.Task(client.Link{Href: href}, nil)
 	if err != nil {
 		return "", err
 	}
 
-	return task.Status.String(), nil
+	return s.String(), nil
+}
+
+func (c *Client) status(ctx context.Context, href string) (client.TaskStatus, error) {
+	kapa, err := c.kapaClient(c.URL, c.Username, c.Password)
+	if err != nil {
+		return 0, err
+	}
+	task, err := kapa.Task(client.Link{Href: href}, nil)
+	if err != nil {
+		return 0, err
+	}
+
+	return task.Status, nil
 }
 
 // All returns all tasks in kapacitor
@@ -259,6 +268,11 @@ func (c *Client) Update(ctx context.Context, href string, rule chronograf.AlertR
 		return nil, err
 	}
 
+	prevStatus, err := c.status(ctx, href)
+	if err != nil {
+		return nil, err
+	}
+
 	// We need to disable the kapacitor task followed by enabling it during update.
 	opts := client.UpdateTaskOptions{
 		TICKscript: string(script),
@@ -277,10 +291,12 @@ func (c *Client) Update(ctx context.Context, href string, rule chronograf.AlertR
 		return nil, err
 	}
 
-	// Now enable the task.
+	// Now enable the task if previously enabled
+	if prevStatus == client.Enabled {
 		if _, err := c.Enable(ctx, href); err != nil {
 			return nil, err
 		}
+	}
 
 	return &Task{
 		ID:         task.ID,

kapacitor/client_test.go

@@ -13,7 +13,12 @@ import (
 type MockKapa struct {
 	ResTask     client.Task
 	ResTasks    []client.Task
-	Error    error
+	TaskError   error
+	UpdateError error
+	CreateError error
+	ListError   error
+	DeleteError error
+	LastStatus  client.TaskStatus
 
 	*client.CreateTaskOptions
 	client.Link
@@ -24,31 +29,34 @@ type MockKapa struct {
 
 func (m *MockKapa) CreateTask(opt client.CreateTaskOptions) (client.Task, error) {
 	m.CreateTaskOptions = &opt
-	return m.ResTask, m.Error
+	return m.ResTask, m.CreateError
 }
 
 func (m *MockKapa) Task(link client.Link, opt *client.TaskOptions) (client.Task, error) {
 	m.Link = link
 	m.TaskOptions = opt
-	return m.ResTask, m.Error
+	return m.ResTask, m.TaskError
 }
 
 func (m *MockKapa) ListTasks(opt *client.ListTasksOptions) ([]client.Task, error) {
 	m.ListTasksOptions = opt
-	return m.ResTasks, m.Error
+	return m.ResTasks, m.ListError
 }
 
 func (m *MockKapa) UpdateTask(link client.Link, opt client.UpdateTaskOptions) (client.Task, error) {
 	m.Link = link
+	m.LastStatus = opt.Status
+
 	if m.UpdateTaskOptions == nil {
 		m.UpdateTaskOptions = &opt
 	}
-	return m.ResTask, m.Error
+
+	return m.ResTask, m.UpdateError
 }
 
 func (m *MockKapa) DeleteTask(link client.Link) error {
 	m.Link = link
-	return m.Error
+	return m.DeleteError
 }
 
 type MockID struct {
@@ -150,7 +158,7 @@ func TestClient_AllStatus(t *testing.T) {
 	for _, tt := range tests {
 		kapa.ResTask = tt.resTask
 		kapa.ResTasks = tt.resTasks
-		kapa.Error = tt.resError
+		kapa.ListError = tt.resError
 
 		t.Run(tt.name, func(t *testing.T) {
 			c := &Client{
@@ -426,7 +434,7 @@ trigger
 	for _, tt := range tests {
 		kapa.ResTask = tt.resTask
 		kapa.ResTasks = tt.resTasks
-		kapa.Error = tt.resError
+		kapa.ListError = tt.resError
 		t.Run(tt.name, func(t *testing.T) {
 			c := &Client{
 				URL:        tt.fields.URL,
@@ -710,7 +718,7 @@ trigger
 	for _, tt := range tests {
 		kapa.ResTask = tt.resTask
 		kapa.ResTasks = tt.resTasks
-		kapa.Error = tt.resError
+		kapa.TaskError = tt.resError
 		t.Run(tt.name, func(t *testing.T) {
 			c := &Client{
 				URL:        tt.fields.URL,
@@ -854,7 +862,7 @@ func TestClient_updateStatus(t *testing.T) {
 	}
 	for _, tt := range tests {
 		kapa.ResTask = tt.resTask
-		kapa.Error = tt.resError
+		kapa.UpdateError = tt.resError
 		kapa.UpdateTaskOptions = nil
 		t.Run(tt.name, func(t *testing.T) {
 			c := &Client{
@@ -904,6 +912,7 @@ func TestClient_Update(t *testing.T) {
 		resError          error
 		wantErr           bool
 		updateTaskOptions *client.UpdateTaskOptions
+		wantStatus        client.TaskStatus
 	}{
 		{
 			name: "update alert rule error",
@@ -937,6 +946,7 @@ func TestClient_Update(t *testing.T) {
 				},
 			},
 			wantErr:    true,
+			wantStatus: client.Disabled,
 		},
 		{
 			name: "update alert rule",
@@ -984,11 +994,60 @@ func TestClient_Update(t *testing.T) {
 					Name: "howdy",
 				},
 			},
+			wantStatus: client.Enabled,
+		},
+		{
+			name: "stays disabled when already disabled",
+			fields: fields{
+				kapaClient: func(url, username, password string) (KapaClient, error) {
+					return kapa, nil
+				},
+				Ticker: &Alert{},
+			},
+			args: args{
+				ctx:  context.Background(),
+				href: "/kapacitor/v1/tasks/howdy",
+				rule: chronograf.AlertRule{
+					ID: "howdy",
+					Query: &chronograf.QueryConfig{
+						Database:        "db",
+						RetentionPolicy: "rp",
+					},
+				},
+			},
+			resTask: client.Task{
+				ID:     "howdy",
+				Status: client.Disabled,
+				Link: client.Link{
+					Href: "/kapacitor/v1/tasks/howdy",
+				},
+			},
+			updateTaskOptions: &client.UpdateTaskOptions{
+				TICKscript: "",
+				Type:       client.StreamTask,
+				Status:     client.Disabled,
+				DBRPs: []client.DBRP{
+					{
+						Database:        "db",
+						RetentionPolicy: "rp",
+					},
+				},
+			},
+			want: &Task{
+				ID:         "howdy",
+				Href:       "/kapacitor/v1/tasks/howdy",
+				HrefOutput: "/kapacitor/v1/tasks/howdy/output",
+				Rule: chronograf.AlertRule{
+					ID:   "howdy",
+					Name: "howdy",
+				},
+			},
+			wantStatus: client.Disabled,
 		},
 	}
 	for _, tt := range tests {
 		kapa.ResTask = tt.resTask
-		kapa.Error = tt.resError
+		kapa.UpdateError = tt.resError
 		t.Run(tt.name, func(t *testing.T) {
 			c := &Client{
 				URL:        tt.fields.URL,
@@ -1009,6 +1068,9 @@ func TestClient_Update(t *testing.T) {
 			if !reflect.DeepEqual(kapa.UpdateTaskOptions, tt.updateTaskOptions) {
 				t.Errorf("Client.Update() = %v, want %v", kapa.UpdateTaskOptions, tt.updateTaskOptions)
 			}
+			if tt.wantStatus != kapa.LastStatus {
+				t.Errorf("Client.Update() = %v, want %v", kapa.LastStatus, tt.wantStatus)
+			}
 		})
 	}
 }
@@ -1126,7 +1188,7 @@ func TestClient_Create(t *testing.T) {
 	}
 	for _, tt := range tests {
 		kapa.ResTask = tt.resTask
-		kapa.Error = tt.resError
+		kapa.CreateError = tt.resError
 		t.Run(tt.name, func(t *testing.T) {
 			c := &Client{
 				URL:        tt.fields.URL,