Academies database connection - add search provider #168
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This project will house interfaces implemented by data providers and types used by data consumers. As this project is intended to only be for interfaces and POCOs there is no need for an associated unit test project Add project reference to AcademiesDb project.
Moq and Fluent Assertions. Add to global usings
We will use this to help mock our dbcontext.
Create new class in `Dfe.FindInformationAcademiesTrusts.Db` which will handle trust search. This should pass our exisitng tests for `TrustSearch`, so have moved these to `Dfe.FindInformationAcademiesTrusts.Db.UnitTests` with a MockDbContext to mock our connection to the database. We have changed the tests/implementation for an empty search term to return an empty list, before calling the database. We will need to add additional tests and implementation for the following: - Trust search entry should include an address - Trust search entry should include identifiers
Move address formatter from trust provider to new Trust Helper utility class. Add address to search response.
Swap 'GetTrustsByName' to use new Academies Db `TrustSearch`. Get Trusts by Ukprn still uses the trust provider for now
The unique identifier for trusts will now be 'GroupId' or 'Trn' - naming this 'Uid' as this will be the unique identifier we use for trusts. This has currently only been renamed in the 'TrustSearchEntry' model, but would need renaming in the route query params for trust pages, and on the search results ui.
This was removed from our designs, as it was deemed not necessary for a search result.
Now that we have the connection to the academies Database we will use the Uid as the unique identifier rather than ukprn, so changing this on the search results page, and in page routing properties. This will cause any 'GetTrustByUkprn' requests to fail on search and trust pages, and will need to be replaced by a new method to fetch trust by UID, calling the database.
The DI framework registers the `academiesDbContext` through `builder.Services.AddDbContext` which doesn't associate it with the `IAcademiesDbContext` interface that we use for testing. We've added a second constructor to allow the DI container to instantiate `TrustSearch` and set it to call the constructor that we use in testing
LINQ cannot translate `string.contains(..., StringComparison.InvariantCultureIgnoreCase)` into SQL. As the standard `string.contains(..)` is translated to case insensitive sql by default we are using that instead. Also: - Added `Take(20)` to protect the database from returning thousands of items
- Add tests to check that only the first 20 results are returned - Refactor tests: - Move address test setup into test, as it is not needed in the constructor - create method to generate and setup a dbcontext response for a list of trusts with any length.
Trust search results should be ordered alphabetically by name. The ordering should take place before taking the first 20, to ensure the first 20 alphabetical results are returned. We may want to add further tests for this.
lookupdaily
temporarily deployed
to
development
GitHub Actions
Inactive
lookupdaily
temporarily deployed
to
development
GitHub Actions
Inactive
lookupdaily
temporarily deployed
to
development
GitHub Actions
Inactive
lookupdaily
temporarily deployed
to
development
GitHub Actions
Inactive
lookupdaily
changed the title
lookupdaily
temporarily deployed
to
development
GitHub Actions
Inactive
lookupdaily
temporarily deployed
to
development
GitHub Actions
Inactive
dynamictulip
reviewed
Oct 9, 2023
DfE.FindInformationAcademiesTrusts.Data.AcademiesDb/TrustSearch.cs
Outdated
Show resolved
Hide resolved
dynamictulip
reviewed
Oct 9, 2023
… groups We only want to return single and multi academy trusts so we need to filter out all other group types including Trust. GroupUid and GroupId are never null for single and multi academy trusts but we should still check as they are nullable fields and we want to use non nullable C# properties.
dynamictulip
reviewed
Oct 9, 2023
…test data. This is because of a previous change where these field could no longer be null. Because of this no results were being returned when we did not populate these properties.
dynamictulip
temporarily deployed
to
development
GitHub Actions
Inactive
dynamictulip
temporarily deployed
to
development
GitHub Actions
Inactive
dynamictulip
temporarily deployed
to
development
GitHub Actions
Inactive
dynamictulip
approved these changes
Oct 11, 2023
dynamictulip
temporarily deployed
to
development
GitHub Actions
Inactive
|
Kudos, SonarCloud Quality Gate passed! |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change swaps the search functionality in the
TrustProviderandTrustSearchclasses of the project for a newTrustSearchclass in the AcademiesDb project - as search will now be handled with a query to the database.Also added the missing properties
UidandGroupIdinto the search results listing, replacingUkprn, as these are the identifiers we have decided to show in our design.This is related to User Story 134572: Build: Connect to Academies DB
Changes
Dfe.FindInformationAcademiesTrusts.Dataproject. This will hold all interfaces which will be used by theDfe.FindInformationAcademiesTrusts.DbprojectTrustSearchprovider in the Academies Db project, responsible for fetching search result using query.Screenshots of UI changes
Before
After
Add screenshot of updated search results page
Checklist