Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added facet sorting alphabetically to search adaptor #57

Closed
wants to merge 1 commit into from
Closed

added facet sorting alphabetically to search adaptor #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions Dfe.Data.SearchPrototype/Infrastructure/CognitiveSearchServiceAdapter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ public CognitiveSearchServiceAdapter(

/// <summary>
/// Makes call to underlying azure cognitive search service and uses the prescribed mapper
/// to adapt the raw Azure search results to the <see cref="SearchResults"/> type.
/// to adapt the raw Azure search results to the <see cref="SearchResults"/> type. Facets are
/// returned in alphabetical order.
/// </summary>
/// <param name="searchServiceAdapterRequest">
/// Prescribes the context of the search including the keyword and collection target.
Expand Down Expand Up @@ -102,7 +103,7 @@ await _searchByKeywordService.SearchAsync<TSearchResult>(
{
Establishments = _searchResultMapper.MapFrom(searchResults.Value.GetResults()),
Facets = searchResults.Value.Facets != null
? _facetsMapper.MapFrom(searchResults.Value.Facets.ToDictionary())
? _facetsMapper.MapFrom(searchResults.Value.Facets.OrderBy(facet => facet.Key).ToDictionary())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is sort order/sequencing definitely a search prototype concern?

My worry is that this is being implemented too "low level" (as opposed to, for example, treating it as a display/web UI concern and doing the sort at the last-reasonable-moment within the view code).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roger, I've put the background to this PR in the description, which we should have done in the first place. Maybe it will answer your concern

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it will answer your concern

I've just had a read and I'm not sure it does.

To illustrate the alternative, what if this sorting was performed in the web mvc project (i.e., the project which displays these values in HTML) as opposed to this shared library project?

: null
};

Expand Down
11 changes: 8 additions & 3 deletions Dfe.Data.SearchPrototype/Infrastructure/Tests/CognitiveSearchServiceAdapterAndMapperTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public async Task Search_WithValidSearchContext_ReturnsResults()
.WithSearchResults()
.Create();
var facetResults = new FacetsResultsFakeBuilder()
.WithAutoGeneratedFacets()
.WithAutoGeneratedFacets(new List<string> { "FACET", "AFACET", "ZFacet1"})
.Create();
var options = AzureSearchOptionsTestDouble.Stub();
var mockService = new SearchServiceMockBuilder()
Expand All @@ -56,15 +56,20 @@ public async Task Search_WithValidSearchContext_ReturnsResults()
await cognitiveSearchServiceAdapter.SearchAsync(
new SearchServiceAdapterRequest(
searchKeyword: "SearchKeyword",
searchFields: ["FIELD1", "FIELD2", "FIELD2"],
facets: ["FACET1", "FACET2", "FACET3"]));
searchFields: ["these aren't used but there needs to be one"],
facets: ["these aren't used but there needs to be one"]
));

// assert
response.Should().NotBeNull();
response.Establishments.Should().NotBeNull();
response.Establishments!.Establishments.Count().Should().Be(establishmentSearchResults.Count);
response.Facets.Should().NotBeNull();
response.Facets!.Facets.Count().Should().Be(facetResults.Count());
// facets are returned alphabetically
response.Facets!.Facets.ToArray()[0].Name.Should().Be("AFACET");
response.Facets!.Facets.ToArray()[1].Name.Should().Be("FACET");
response.Facets!.Facets.ToArray()[2].Name.Should().Be("ZFacet1");
}

[Fact]
Expand Down
4 changes: 2 additions & 2 deletions ...rototype/Infrastructure/Tests/Mappers/AzureFacetResultToEstablishmentFacetsMapperTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public void MapFrom_WithStringFacetResults_ReturnsFacets()
// arrange
var azureFacetsResults = new FacetsResultsFakeBuilder()
.WithEducationPhaseFacet()
.WithAutoGeneratedFacet()
.WithAutoGeneratedFacet("Facet1")
.Create();

// act
Expand Down Expand Up @@ -50,7 +50,7 @@ public void MapFrom_WithNonStringFacetResults_ThrowsInvalidCastException()
{
// arrange
var azureFacetsResults = new FacetsResultsFakeBuilder()
.WithFacet(new List<object>() { true, "string2"})
.WithFacetValue(new List<object>() { true, "string2"})
.Create();

// act, assert
Expand Down
15 changes: 12 additions & 3 deletions Dfe.Data.SearchPrototype/Infrastructure/Tests/TestDoubles/FacetsResultsFakeBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,16 @@ public FacetsResultsFakeBuilder WithAutoGeneratedFacets()
return this;
}

public FacetsResultsFakeBuilder WithAutoGeneratedFacet(string? appendFacetName = null)
public FacetsResultsFakeBuilder WithAutoGeneratedFacets(List<string> facetNames)
{
foreach (var facetName in facetNames)
{
_ = WithAutoGeneratedFacet(facetName);
}
return this;
}

public FacetsResultsFakeBuilder WithAutoGeneratedFacet(string facetName, string? appendFacetName = null)
{
var facetResults = new List<FacetResult>();
int resultsCountAnyNumber = new Bogus.Faker().Random.Int(1, 50);
Expand All @@ -58,7 +67,7 @@ public FacetsResultsFakeBuilder WithAutoGeneratedFacet(string? appendFacetName =
facetResults.Add(SearchModelFactory.FacetResult(resultsCountAnyNumber, facetResult));
}

Dictionary<string, IList<FacetResult>> facet = new() { [new Bogus.Faker().Name.JobType() + appendFacetName] = facetResults };
Dictionary<string, IList<FacetResult>> facet = new() { [facetName + appendFacetName] = facetResults };

foreach (var kvp in facet)
{
Expand All @@ -67,7 +76,7 @@ public FacetsResultsFakeBuilder WithAutoGeneratedFacet(string? appendFacetName =
return this;
}

public FacetsResultsFakeBuilder WithFacet(List<object> facetParams)
public FacetsResultsFakeBuilder WithFacetValue(List<object> facetParams)
{
var facetResults = new List<FacetResult>();
int resultsCountAnyNumber = new Bogus.Faker().Random.Int(1, 50);
Expand Down
2 changes: 1 addition & 1 deletion Dfe.Data.SearchPrototype/Infrastructure/Tests/TestDoubles/SearchServiceMockBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public static Expression<Func<ISearchByKeywordService, Task<Response<SearchResul
private string _collection = string.Empty;
private IEnumerable<SearchResult<Establishment>>? _searchResults;
private Dictionary<string, IList<FacetResult>>? _facets;

public ISearchByKeywordService MockFor(Response<SearchResults<Establishment>> searchResult, string keyword, string collection)
{
var searchServiceMock = new Mock<ISearchByKeywordService>();
Expand Down