Skip to content

Commit 44529e0

Browse files
crspellercrspeller
authored
Fix issues with search initalization. (#356)
* Fix showing AI enhanced serach button when disabled * Fix other instances of the same issue * Refactor and fix * Use mockary * Style * Simplify
1 parent f21877f commit 44529e0

File tree

11 files changed

+723
-113
lines changed

11 files changed

+723
-113
lines changed

‎.mockery.yml‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,4 +17,10 @@ packages:
1717
dir: mmapi/mocks
1818
filename: client_mock.go
1919
interfaces:
20-
Client:
20+
Client:
21+
github.com/mattermost/mattermost-plugin-ai/embeddings:
22+
config:
23+
dir: embeddings/mocks
24+
filename: embedding_search_mock.go
25+
interfaces:
26+
EmbeddingSearch:

‎api/api.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,7 @@ func (a *API) handleGetAIBots(c *gin.Context) {
291291
}
292292

293293
// Check if search is enabled
294-
searchEnabled := a.searchService != nil
294+
searchEnabled := a.searchService.Enabled()
295295

296296
c.JSON(http.StatusOK, AIBotsResponse{
297297
Bots: bots,

‎api/api_search.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ func (a *API) handleRunSearch(c *gin.Context) {
2424
userID := c.GetHeader("Mattermost-User-Id")
2525
bot := c.MustGet(ContextBotKey).(*bots.Bot)
2626

27-
if a.searchService == nil {
27+
if !a.searchService.Enabled() {
2828
c.AbortWithError(http.StatusBadRequest, fmt.Errorf("search functionality is not configured"))
2929
return
3030
}
@@ -53,7 +53,7 @@ func (a *API) handleSearchQuery(c *gin.Context) {
5353
userID := c.GetHeader("Mattermost-User-Id")
5454
bot := c.MustGet(ContextBotKey).(*bots.Bot)
5555

56-
if a.searchService == nil {
56+
if !a.searchService.Enabled() {
5757
c.AbortWithError(http.StatusBadRequest, fmt.Errorf("search functionality is not configured"))
5858
return
5959
}

‎api/api_search_test.go‎

Lines changed: 226 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,226 @@
1+
// Copyright (c) 2023-present Mattermost, Inc. All Rights Reserved.
2+
// See LICENSE.txt for license information.
3+
4+
package api
5+
6+
import (
7+
"bytes"
8+
"encoding/json"
9+
"errors"
10+
"io"
11+
"net/http"
12+
"net/http/httptest"
13+
"testing"
14+
15+
"github.com/gin-gonic/gin"
16+
"github.com/mattermost/mattermost-plugin-ai/embeddings"
17+
"github.com/mattermost/mattermost-plugin-ai/embeddings/mocks"
18+
"github.com/mattermost/mattermost-plugin-ai/llm"
19+
mmapimocks "github.com/mattermost/mattermost-plugin-ai/mmapi/mocks"
20+
"github.com/mattermost/mattermost-plugin-ai/search"
21+
"github.com/mattermost/mattermost/server/public/plugin"
22+
"github.com/stretchr/testify/mock"
23+
"github.com/stretchr/testify/require"
24+
)
25+
26+
func TestHandleRunSearch(t *testing.T) {
27+
gin.SetMode(gin.ReleaseMode)
28+
gin.DefaultWriter = io.Discard
29+
30+
tests := []struct {
31+
name string
32+
searchService *search.Search
33+
setupMock func(t *testing.T) *search.Search
34+
requestBody SearchRequest
35+
expectedStatus int
36+
expectError bool
37+
}{
38+
{
39+
name: "search fails - DM error, service enabled",
40+
setupMock: func(t *testing.T) *search.Search {
41+
mockClient := mmapimocks.NewMockClient(t)
42+
mockClient.On("DM", mock.Anything, mock.Anything, mock.Anything).Return(errors.New("DM failed"))
43+
return search.New(mocks.NewMockEmbeddingSearch(t), mockClient, nil, nil, nil)
44+
},
45+
requestBody: SearchRequest{
46+
Query: "test query",
47+
TeamID: "team123",
48+
ChannelID: "channel123",
49+
MaxResults: 10,
50+
},
51+
expectedStatus: http.StatusInternalServerError,
52+
expectError: true,
53+
},
54+
{
55+
name: "search fails - service disabled",
56+
searchService: search.New(nil, nil, nil, nil, nil),
57+
requestBody: SearchRequest{
58+
Query: "test query",
59+
TeamID: "team123",
60+
ChannelID: "channel123",
61+
MaxResults: 10,
62+
},
63+
expectedStatus: http.StatusBadRequest,
64+
expectError: true,
65+
},
66+
{
67+
name: "search fails - no service",
68+
searchService: nil,
69+
requestBody: SearchRequest{
70+
Query: "test query",
71+
TeamID: "team123",
72+
ChannelID: "channel123",
73+
MaxResults: 10,
74+
},
75+
expectedStatus: http.StatusBadRequest,
76+
expectError: true,
77+
},
78+
{
79+
name: "search fails - empty query",
80+
searchService: search.New(mocks.NewMockEmbeddingSearch(t), nil, nil, nil, nil),
81+
requestBody: SearchRequest{
82+
Query: "",
83+
TeamID: "team123",
84+
ChannelID: "channel123",
85+
MaxResults: 10,
86+
},
87+
expectedStatus: http.StatusBadRequest,
88+
expectError: true,
89+
},
90+
}
91+
92+
for _, test := range tests {
93+
t.Run(test.name, func(t *testing.T) {
94+
e := SetupTestEnvironment(t)
95+
defer e.Cleanup(t)
96+
97+
// Override the search service for this test
98+
if test.setupMock != nil {
99+
e.api.searchService = test.setupMock(t)
100+
} else {
101+
e.api.searchService = test.searchService
102+
}
103+
104+
// Setup a test bot
105+
e.setupTestBot(llm.BotConfig{
106+
Name: "test-bot",
107+
DisplayName: "Test Bot",
108+
})
109+
110+
// Setup mock expectations
111+
e.mockAPI.On("LogError", mock.Anything).Maybe()
112+
113+
// Create request body
114+
bodyBytes, err := json.Marshal(test.requestBody)
115+
require.NoError(t, err)
116+
117+
// Create request
118+
request := httptest.NewRequest(http.MethodPost, "/search/run?botUsername=test-bot", bytes.NewReader(bodyBytes))
119+
request.Header.Add("Mattermost-User-ID", "userid")
120+
request.Header.Set("Content-Type", "application/json")
121+
122+
// Execute request
123+
recorder := httptest.NewRecorder()
124+
e.api.ServeHTTP(&plugin.Context{}, recorder, request)
125+
126+
// Verify status code
127+
resp := recorder.Result()
128+
require.Equal(t, test.expectedStatus, resp.StatusCode)
129+
})
130+
}
131+
}
132+
133+
func TestHandleSearchQuery(t *testing.T) {
134+
gin.SetMode(gin.ReleaseMode)
135+
gin.DefaultWriter = io.Discard
136+
137+
tests := []struct {
138+
name string
139+
setupMock func(t *testing.T) *search.Search
140+
searchService *search.Search
141+
requestBody SearchRequest
142+
expectedStatus int
143+
expectError bool
144+
}{
145+
{
146+
name: "search query succeeds - service enabled",
147+
setupMock: func(t *testing.T) *search.Search {
148+
mockEmbedding := mocks.NewMockEmbeddingSearch(t)
149+
mockEmbedding.On("Search", mock.Anything, "test query", mock.Anything).Return([]embeddings.SearchResult{}, nil)
150+
return search.New(mockEmbedding, nil, nil, nil, nil)
151+
},
152+
requestBody: SearchRequest{
153+
Query: "test query",
154+
TeamID: "team123",
155+
ChannelID: "channel123",
156+
MaxResults: 10,
157+
},
158+
expectedStatus: http.StatusOK,
159+
expectError: false,
160+
},
161+
{
162+
name: "search query fails - service disabled",
163+
searchService: search.New(nil, nil, nil, nil, nil),
164+
requestBody: SearchRequest{
165+
Query: "test query",
166+
TeamID: "team123",
167+
ChannelID: "channel123",
168+
MaxResults: 10,
169+
},
170+
expectedStatus: http.StatusBadRequest,
171+
expectError: true,
172+
},
173+
{
174+
name: "search query fails - no service",
175+
searchService: nil,
176+
requestBody: SearchRequest{
177+
Query: "test query",
178+
TeamID: "team123",
179+
ChannelID: "channel123",
180+
MaxResults: 10,
181+
},
182+
expectedStatus: http.StatusBadRequest,
183+
expectError: true,
184+
},
185+
}
186+
187+
for _, test := range tests {
188+
t.Run(test.name, func(t *testing.T) {
189+
e := SetupTestEnvironment(t)
190+
defer e.Cleanup(t)
191+
192+
// Override the search service for this test
193+
if test.setupMock != nil {
194+
e.api.searchService = test.setupMock(t)
195+
} else {
196+
e.api.searchService = test.searchService
197+
}
198+
199+
// Setup a test bot
200+
e.setupTestBot(llm.BotConfig{
201+
Name: "test-bot",
202+
DisplayName: "Test Bot",
203+
})
204+
205+
// Setup mock expectations
206+
e.mockAPI.On("LogError", mock.Anything).Maybe()
207+
208+
// Create request body
209+
bodyBytes, err := json.Marshal(test.requestBody)
210+
require.NoError(t, err)
211+
212+
// Create request
213+
request := httptest.NewRequest(http.MethodPost, "/search?botUsername=test-bot", bytes.NewReader(bodyBytes))
214+
request.Header.Add("Mattermost-User-ID", "userid")
215+
request.Header.Set("Content-Type", "application/json")
216+
217+
// Execute request
218+
recorder := httptest.NewRecorder()
219+
e.api.ServeHTTP(&plugin.Context{}, recorder, request)
220+
221+
// Verify status code
222+
resp := recorder.Result()
223+
require.Equal(t, test.expectedStatus, resp.StatusCode)
224+
})
225+
}
226+
}

‎api/api_test.go‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package api
55

66
import (
7+
"encoding/json"
78
"io"
89
"net/http"
910
"net/http/httptest"
@@ -13,9 +14,11 @@ import (
1314
"github.com/gin-gonic/gin"
1415
"github.com/mattermost/mattermost-plugin-ai/bots"
1516
"github.com/mattermost/mattermost-plugin-ai/conversations"
17+
"github.com/mattermost/mattermost-plugin-ai/embeddings/mocks"
1618
"github.com/mattermost/mattermost-plugin-ai/enterprise"
1719
"github.com/mattermost/mattermost-plugin-ai/llm"
1820
"github.com/mattermost/mattermost-plugin-ai/metrics"
21+
"github.com/mattermost/mattermost-plugin-ai/search"
1922
"github.com/mattermost/mattermost/server/public/model"
2023
"github.com/mattermost/mattermost/server/public/plugin"
2124
"github.com/mattermost/mattermost/server/public/plugin/plugintest"
@@ -368,3 +371,85 @@ func TestChannelRouter(t *testing.T) {
368371
}
369372
}
370373
}
374+
375+
func TestHandleGetAIBots(t *testing.T) {
376+
gin.SetMode(gin.ReleaseMode)
377+
gin.DefaultWriter = io.Discard
378+
379+
tests := []struct {
380+
name string
381+
searchService *search.Search
382+
expectedSearchEnabled bool
383+
expectedStatus int
384+
envSetup func(e *TestEnvironment)
385+
}{
386+
{
387+
name: "search enabled - non-nil service with non-nil embedding search",
388+
searchService: search.New(mocks.NewMockEmbeddingSearch(t), nil, nil, nil, nil),
389+
expectedSearchEnabled: true,
390+
expectedStatus: http.StatusOK,
391+
envSetup: func(e *TestEnvironment) {
392+
e.mockAPI.On("GetChannelByName", "", mock.AnythingOfType("string"), false).Return(nil, &model.AppError{})
393+
},
394+
},
395+
{
396+
name: "search disabled - non-nil service with nil embedding search",
397+
searchService: search.New(nil, nil, nil, nil, nil),
398+
expectedSearchEnabled: false,
399+
expectedStatus: http.StatusOK,
400+
envSetup: func(e *TestEnvironment) {
401+
e.mockAPI.On("GetChannelByName", "", mock.AnythingOfType("string"), false).Return(nil, &model.AppError{})
402+
},
403+
},
404+
{
405+
name: "no search service - nil service",
406+
searchService: nil,
407+
expectedSearchEnabled: false,
408+
expectedStatus: http.StatusOK,
409+
envSetup: func(e *TestEnvironment) {
410+
e.mockAPI.On("GetChannelByName", "", mock.AnythingOfType("string"), false).Return(nil, &model.AppError{})
411+
},
412+
},
413+
}
414+
415+
for _, test := range tests {
416+
t.Run(test.name, func(t *testing.T) {
417+
e := SetupTestEnvironment(t)
418+
defer e.Cleanup(t)
419+
420+
// Override the search service for this test
421+
e.api.searchService = test.searchService
422+
423+
// Setup a test bot
424+
e.setupTestBot(llm.BotConfig{
425+
Name: "test-bot",
426+
DisplayName: "Test Bot",
427+
})
428+
429+
// Setup mock expectations
430+
test.envSetup(e)
431+
e.mockAPI.On("LogError", mock.Anything).Maybe()
432+
433+
// Create request
434+
request := httptest.NewRequest(http.MethodGet, "/ai_bots", nil)
435+
request.Header.Add("Mattermost-User-ID", "userid")
436+
437+
// Execute request
438+
recorder := httptest.NewRecorder()
439+
e.api.ServeHTTP(&plugin.Context{}, recorder, request)
440+
441+
// Verify status code
442+
resp := recorder.Result()
443+
require.Equal(t, test.expectedStatus, resp.StatusCode)
444+
445+
// Verify response body
446+
if test.expectedStatus == http.StatusOK {
447+
var response AIBotsResponse
448+
err := json.NewDecoder(resp.Body).Decode(&response)
449+
require.NoError(t, err)
450+
require.Equal(t, test.expectedSearchEnabled, response.SearchEnabled, "SearchEnabled field should match expected value")
451+
require.NotEmpty(t, response.Bots, "Should return at least one bot")
452+
}
453+
})
454+
}
455+
}

0 commit comments

Comments
 (0)