Skip to content

Commit

Permalink
Preventing empty error message suggestions (#1235)
Browse files Browse the repository at this point in the history 
Preventing empty error message suggestions. The problem was that the errorMessage field was filled with an empty string if there was no error. Therefore the empty string suggestion was correct. The bug was prevented in three places ingestion, request to the API from the UI, and the search bar component.

Signed-off-by: Lance Finfrock <lfinfrock@chef.io>
  • Loading branch information
@lancewf
lancewf committed Aug 14, 2019
1 parent 61aa99a commit d99e44a
Show file tree
Hide file tree
Showing 6 changed files with 118 additions and 24 deletions.
31 changes: 29 additions & 2 deletions components/automate-ui/src/app/entities/client-runs/client-runs.requests.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ describe('ClientRunsRequests', () => {
it('returns all provided values', () => {
const expectedData: Chicklet[] = [];
for (let i = 0; i < 50; i++) {
expectedData.push({'text': '', 'type': ''});
expectedData.push({'text': i.toString(), 'type': 'name'});
}
const expectedUrl = `${CONFIG_MGMT_URL}/suggestions?type=name&text=fred`;
const filters: NodeFilter = <NodeFilter>{};
Expand All @@ -50,8 +50,35 @@ describe('ClientRunsRequests', () => {
it('returns 10 values when 10 are provided', () => {
const expectedData: Chicklet[] = [];
for (let i = 0; i < 10; i++) {
expectedData.push({'text': i.toString(), 'type': ''});
expectedData.push({'text': i.toString(), 'type': 'name'});
}
const expectedUrl = `${CONFIG_MGMT_URL}/suggestions?type=name&text=fred`;

const filters: NodeFilter = <NodeFilter>{};
service.getSuggestions('name', 'fred', filters).subscribe(data => {
expect(data.length).toEqual(10);
for ( const item of data ) {
expect(Number(item.text)).toBeLessThan(10);
}
});

const req = httpTestingController.expectOne(expectedUrl);

expect(req.request.method).toEqual('GET');

req.flush(expectedData);
});

it('remove empty suggestions', () => {
const expectedData: any[] = [];
for (let i = 0; i < 10; i++) {
expectedData.push({'text': i.toString(), 'type': 'name'});
}

// Add empty suggestions
expectedData.push({'type': ''});
expectedData.push({'text': '', 'type': 'name'});

const expectedUrl = `${CONFIG_MGMT_URL}/suggestions?type=name&text=fred`;

const filters: NodeFilter = <NodeFilter>{};
Expand Down
7 changes: 6 additions & 1 deletion components/automate-ui/src/app/entities/client-runs/client-runs.requests.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ const CONFIG_MGMT_URL = environment.config_mgmt_url;
const INGEST_URL = environment.ingest_url;
const DEPLOYMENT_URL = environment.deployment_url;

interface RespSuggestion {
text: string;
}

@Injectable()
export class ClientRunsRequests {
constructor(
Expand Down Expand Up @@ -79,7 +83,8 @@ export class ClientRunsRequests {
const params = this.formatFilters(filters).set('type', type).set('text', text);
const url = `${CONFIG_MGMT_URL}/suggestions`;

return this.httpClient.get<Chicklet[]>(url, {params});
return this.httpClient.get<RespSuggestion[]>(url, {params}).pipe(map(
(suggestions) => suggestions.filter(s => s && s.text && s.text.length !== 0)));
} else {
return observableOf([]);
}
Expand Down
11 changes: 11 additions & 0 deletions components/automate-ui/src/app/page-components/search-bar/search-bar.component.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,17 @@ describe('SearchBarComponent', () => {

expect(component.suggestionsVisible).toEqual(true);
});

it('remove empty string returned suggestions', () => {
component.selectedCategoryType = component.categories[0];
component.dynamicSuggestions = ['1', '2', '3', '', undefined, null];
component.ngOnChanges({
dynamicSuggestions: new SimpleChange(null, component.dynamicSuggestions, false)
});
fixture.detectChanges();

expect(component.suggestions.map(s => s.name).toArray().sort()).toEqual(['1', '2', '3']);
});
});

describe('enter is pressed', () => {
Expand Down
5 changes: 3 additions & 2 deletions components/automate-ui/src/app/page-components/search-bar/search-bar.component.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
} from '@angular/core';
import { Subject, Observable, of as observableOf } from 'rxjs';
import { List } from 'immutable';
import { clamp, compact } from 'lodash';
import { clamp, compact, isEmpty } from 'lodash';
import { SearchBarCategoryItem, Chicklet, SuggestionItem } from '../../types/types';
import {
debounceTime, switchMap, distinctUntilChanged
Expand Down Expand Up @@ -71,7 +71,8 @@ export class SearchBarComponent implements OnChanges {
ngOnChanges(changes: SimpleChanges) {
if (changes.dynamicSuggestions) {
this.suggestions = List<SuggestionItem>(
compact(changes.dynamicSuggestions.currentValue.map((suggestion) => {
compact(changes.dynamicSuggestions.currentValue.filter((suggestion) =>
!isEmpty(suggestion)).map((suggestion) => {
return {name: suggestion, title: suggestion};
})));

Expand Down
86 changes: 68 additions & 18 deletions components/config-mgmt-service/integration_test/suggestions_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ func TestSuggestionsLargeArrayValues(t *testing.T) {
res, err := cfgmgmt.GetSuggestions(ctx, &test.request)
assert.Nil(t, err)

actualSuggestions := extractTextFromSuggestionsResponse(res, t)
actualSuggestions := extractTextFromSuggestionsResponse(res)

assert.ElementsMatch(t, test.expected, actualSuggestions)
})
Expand Down Expand Up @@ -210,7 +210,7 @@ func TestSuggestionsLargeCollectionOfSamePrefixTerm(t *testing.T) {
res, err := cfgmgmt.GetSuggestions(context.Background(), &test.request)
assert.Nil(t, err)

actualSuggestions := extractTextFromSuggestionsResponse(res, t)
actualSuggestions := extractTextFromSuggestionsResponse(res)

assert.Contains(t, actualSuggestions, test.expectedTerm)
})
Expand Down Expand Up @@ -707,14 +707,66 @@ func TestSuggestionsFiltered(t *testing.T) {
res, err := cfgmgmt.GetSuggestions(ctx, &test.request)
assert.Nil(t, err)

actualSuggestions := extractTextFromSuggestionsResponse(res, t)
actualSuggestions := extractTextFromSuggestionsResponse(res)

assert.ElementsMatch(t, test.expected, actualSuggestions)
})
}

}

func TestSuggestionsEmptyErrorMessage(t *testing.T) {
nodes := []iBackend.Node{
{
NodeInfo: iBackend.NodeInfo{
NodeName: "1",
},
ErrorMessage: "", //nonvalid suggestion 1"
},
{
NodeInfo: iBackend.NodeInfo{
NodeName: "2",
},
ErrorMessage: "", //nonvalid suggestion 2"
},
{
NodeInfo: iBackend.NodeInfo{
NodeName: "3",
},
ErrorMessage: "valid suggestion 1",
},
{
NodeInfo: iBackend.NodeInfo{
NodeName: "4",
},
ErrorMessage: "valid suggestion 2",
},
}

request := request.Suggestion{
Type: "error",
Text: "v",
}

expected := []string{"valid suggestion 1", "valid suggestion 2"}

// Adding required node data
for index := range nodes {
nodes[index].Exists = true
nodes[index].NodeInfo.EntityUuid = newUUID()
}

// Add node with project
suite.IngestNodes(nodes)
defer suite.DeleteAllDocuments()
res, err := cfgmgmt.GetSuggestions(context.Background(), &request)
assert.Nil(t, err)

actualSuggestions := extractTextFromSuggestionsResponse(res)

assert.ElementsMatch(t, expected, actualSuggestions)
}

func TestSuggestionsWithTableDriven(t *testing.T) {
dataNodes := []struct {
number int
Expand Down Expand Up @@ -873,13 +925,13 @@ func TestSuggestionsWithTableDriven(t *testing.T) {
// Suggestions for Environments
{"should return all environment suggestions",
request.Suggestion{Type: "environment"},
[]string{"dev", "prod", "games", "new-york"}},
[]string{"dev", "prod", "games", "new-york", ""}},
{"should return zero environment suggestions",
request.Suggestion{Type: "platform", Text: "lol"},
[]string{}},
{"should return results when starting typing 'new-york'",
request.Suggestion{Type: "environment", Text: "n"}, // less than 2 characters we return all results?
[]string{"dev", "prod", "games", "new-york"}},
[]string{"dev", "prod", "games", "new-york", ""}},
{"should return one environment suggestion 'new-york'",
request.Suggestion{Type: "environment", Text: "ne"},
[]string{"new-york"}},
Expand Down Expand Up @@ -907,13 +959,13 @@ func TestSuggestionsWithTableDriven(t *testing.T) {
// Suggestions for Policy Group
{"should return all policy_group suggestions",
request.Suggestion{Type: "policy_group"},
[]string{"sports", "tv_series", "rpgs", "heros", "movies", "nintendo"}},
[]string{"sports", "tv_series", "rpgs", "heros", "movies", "nintendo", ""}},
{"should return zero policy_group suggestions",
request.Suggestion{Type: "policy_group", Text: "lol"},
[]string{}},
{"should return results when starting typing 'nintendo'",
request.Suggestion{Type: "policy_group", Text: "n"}, // less than 2 characters we return all results?
[]string{"sports", "tv_series", "rpgs", "heros", "movies", "nintendo"}},
[]string{"sports", "tv_series", "rpgs", "heros", "movies", "nintendo", ""}},
{"should return one policy_group suggestion 'nintendo'",
request.Suggestion{Type: "policy_group", Text: "ni"},
[]string{"nintendo"}},
Expand All @@ -924,13 +976,13 @@ func TestSuggestionsWithTableDriven(t *testing.T) {
// Suggestions for Policy Name
{"should return all policy_name suggestions",
request.Suggestion{Type: "policy_name"},
[]string{"time_to_be_funny", "games", "boardgames", "comics", "videogames"}},
[]string{"time_to_be_funny", "games", "boardgames", "comics", "videogames", ""}},
{"should return zero policy_name suggestions",
request.Suggestion{Type: "policy_name", Text: "lol"},
[]string{}},
{"should return results when starting typing 'games'",
request.Suggestion{Type: "policy_name", Text: "g"}, // less than 2 characters we return all results?
[]string{"time_to_be_funny", "games", "boardgames", "comics", "videogames"}},
[]string{"time_to_be_funny", "games", "boardgames", "comics", "videogames", ""}},
{"should return one policy_name suggestion 'games'",
request.Suggestion{Type: "policy_name", Text: "ga"},
[]string{"games"}},
Expand All @@ -941,13 +993,13 @@ func TestSuggestionsWithTableDriven(t *testing.T) {
// Suggestions for Policy Revision
{"should return all policy_revision suggestions",
request.Suggestion{Type: "policy_revision"},
[]string{"extream", "friends", "fantasy", "marvel", "old_school", "zelda"}},
[]string{"extream", "friends", "fantasy", "marvel", "old_school", "zelda", ""}},
{"should return zero policy_revision suggestions",
request.Suggestion{Type: "policy_revision", Text: "lol"},
[]string{}},
{"should return results when starting typing 'friends'",
request.Suggestion{Type: "policy_revision", Text: "f"}, // less than 2 characters we return all results?
[]string{"extream", "friends", "fantasy", "marvel", "old_school", "zelda"}},
[]string{"extream", "friends", "fantasy", "marvel", "old_school", "zelda", ""}},
{"should return one policy_revision suggestion 'friends'",
request.Suggestion{Type: "policy_revision", Text: "fr"},
[]string{"friends"}},
Expand Down Expand Up @@ -1065,7 +1117,7 @@ func TestSuggestionsWithTableDriven(t *testing.T) {
// Suggestions for chef version
{"should return all chef_version suggestions",
request.Suggestion{Type: "chef_version"},
[]string{"1.0", "1.1", "1.1.0", "2.0", "3.0"}},
[]string{"1.0", "1.1", "1.1.0", "2.0", "3.0", ""}},
{"should return zero chef_version suggestions",
request.Suggestion{Type: "chef_version", Text: "lol"},
[]string{}},
Expand Down Expand Up @@ -1101,7 +1153,7 @@ func TestSuggestionsWithTableDriven(t *testing.T) {
// We actually don't care about the scores since it is something
// the UI uses to order the results, therefore we will just abstract
// the text into an array and compare it
actualSuggestionsArray := extractTextFromSuggestionsResponse(res, t)
actualSuggestionsArray := extractTextFromSuggestionsResponse(res)

// Verify they both are the same length
assert.Equal(t, len(test.expected), len(actualSuggestionsArray))
Expand Down Expand Up @@ -1277,7 +1329,7 @@ func TestSuggestionsProjectFilter(t *testing.T) {
res, err := cfgmgmt.GetSuggestions(test.ctx, &request.Suggestion{Type: "name"})
assert.Nil(t, err)

actualSuggestionsArray := extractTextFromSuggestionsResponse(res, t)
actualSuggestionsArray := extractTextFromSuggestionsResponse(res)

assert.ElementsMatch(t, test.expected, actualSuggestionsArray)
})
Expand Down Expand Up @@ -1323,16 +1375,14 @@ func TestSuggestionsProjectFilter(t *testing.T) {
// TODO: (@afiune) Is this the normal behavior? If not lets fix it.
// for now the fixt in the tests will be to check if there is a "string"
// value or not.
func extractTextFromSuggestionsResponse(list *gp.ListValue, t *testing.T) []string {
func extractTextFromSuggestionsResponse(list *gp.ListValue) []string {
// We don't initialize the slice size since we might found empty Values
textArray := make([]string, 0)

if list != nil {
for _, sugg := range list.Values {
sugStruct := sugg.GetStructValue()
if txt := sugStruct.Fields["text"].GetStringValue(); txt != "" {
textArray = append(textArray, txt)
}
textArray = append(textArray, sugStruct.Fields["text"].GetStringValue())
}
}
return textArray
Expand Down
2 changes: 1 addition & 1 deletion components/ingest-service/backend/converge.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ type Node struct {
HasDeprecations bool `json:"has_deprecations"`
DeprecationsCount int `json:"deprecations_count"`
Projects []string `json:"projects"`
ErrorMessage string `json:"error_message"`
ErrorMessage string `json:"error_message,omitempty"`
}

// NodeAttribute is the representation of the
Expand Down

0 comments on commit d99e44a

Please sign in to comment.