Skip to content

Commit

Permalink
Merge pull request #813 from DFE-Digital/chore/231142/improve-dbo-schema
Browse files Browse the repository at this point in the history
Chore: Improvements to the dbo schema
  • Loading branch information
katie-gardner authored Oct 9, 2024
2 parents 6cb77d9 + 2aa0691 commit edf4d88
Show file tree
Hide file tree
Showing 7 changed files with 260 additions and 25 deletions.
8 changes: 5 additions & 3 deletions docs/adr/0040-sql-schema-improvements.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# 0040 - SQL Schema Improvements

* **Status**: proposed
* **Status**: accepted

## Context and Problem Statement

Expand Down Expand Up @@ -47,8 +47,6 @@ blue is tables we could change, red tables to delete (displayed with two differe

### Contentful schema

- Change `GetCurrentSubmissionId` to a UDF
- It calculates one value and outputs it, which is what UDFs are perfect for and will simplify existing usage of it
- Add a dateCreated and dateLastUpdated column to the `ContentComponents` table.
- This is useful for developers to see when content has changed, and makes it easier to debug content issues.
- Add missing foreign keys
Expand All @@ -69,6 +67,8 @@ blue is tables we could change, red tables to delete (displayed with two differe

### dbo schema

- Change `GetCurrentSubmissionId` to a UDF
- It calculates one value and outputs it, which is what UDFs are perfect for and will simplify existing usage of it
- Alter the `SubmitAnswer` procedure to reduce data duplication
- Currently every time an answer is submitted it puts a new record into the `questions` and `answers` tables with
the question and answer text
Expand All @@ -80,3 +80,5 @@ blue is tables we could change, red tables to delete (displayed with two differe
- Add an index to the `response` table on `submissionId` as this is frequently used to connect the two tables

## Decision Outcome

Make the recommended changes outlined above to the dbo and Contentful schemas.
2 changes: 1 addition & 1 deletion docs/adr/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,4 @@ General information about architectural decision records is available at <https:
| [0037 - Content Validation Process](./0037-content-validation-process.md) | Proposed |
| [0038 - SQL Caching Mechanism](0038-sql-caching-mechanism.md) | Proposed |
| [0039 - Consolidate Azure Function App into Web App](0039-consolidate-function-app.md) | Accepted |
| [0040 - SQL Schema Improvements](0040-sql-schema-improvements.md) | Proposed |
| [0040 - SQL Schema Improvements](0040-sql-schema-improvements.md) | Accepted |
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@ public async Task DeleteCurrentSubmission(ISectionComponent section, Cancellatio

await _db.ExecuteSqlAsync($@"EXEC DeleteCurrentSubmission
@establishmentId={establishmentId},
@sectionId={section.Sys.Id},
@sectionName={section.Name}",
@sectionId={section.Sys.Id}",
cancellationToken);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,213 @@
-- Remove existing GetCurrentSubmissionId procedure

DROP PROCEDURE GetCurrentSubmissionId

GO

-- Make a new GetCurrentSubmissionId UDF

CREATE FUNCTION GetCurrentSubmissionId(
@sectionId NVARCHAR(50),
@establishmentId INT
)
RETURNS INT
AS
BEGIN
RETURN (
SELECT TOP 1
Id
FROM [dbo].[submission]
WHERE
completed = 0
AND deleted = 0
AND sectionId = @sectionId
AND establishmentId = @establishmentId
ORDER BY dateCreated DESC
)
END

GO

-- Simplify usage of old GetCurrentSubmissionId proc with the new UDF

ALTER PROCEDURE DeleteCurrentSubmission
@sectionId NVARCHAR(50),
@establishmentId INT
AS
BEGIN TRY
DECLARE @submissionId INT = dbo.GetCurrentSubmissionId (@sectionId, @establishmentId)

BEGIN TRAN
UPDATE S
SET deleted = 1
FROM [dbo].[submission] S
WHERE S.id = @submissionId
COMMIT TRAN

END TRY
BEGIN CATCH
ROLLBACK TRAN
END CATCH

GO

ALTER PROCEDURE SelectOrInsertSubmissionId
@sectionId NVARCHAR(50),
@sectionName NVARCHAR(50),
@establishmentId INT,
@submissionId INT OUTPUT
AS

BEGIN TRY
BEGIN TRAN
SELECT @submissionId = dbo.GetCurrentSubmissionId(@sectionId, @establishmentId)

IF @submissionId IS NULL
BEGIN
INSERT INTO [dbo].[submission]
(establishmentId, completed, sectionId, sectionName)
VALUES
(@establishmentId, 0, @sectionId, @sectionName)

SELECT @submissionId = SCOPE_IDENTITY()
END
COMMIT TRAN
END TRY
BEGIN CATCH
ROLLBACK TRAN
END CATCH

GO

-- Reduce duplication in SubmitAnswer sproc by reusing question & answer ids where applicable

ALTER PROCEDURE SubmitAnswer
@sectionId NVARCHAR(50),
@sectionName NVARCHAR(50),
@questionContentfulId NVARCHAR(50),
@questionText NVARCHAR(MAX),
@answerContentfulId NVARCHAR(50),
@answerText NVARCHAR(MAX),
@userId INT,
@establishmentId INT,
@maturity NVARCHAR(20),
@responseId INT OUTPUT,
@submissionId INT OUTPUT
AS

BEGIN TRY
BEGIN TRAN
EXEC SelectOrInsertSubmissionId
@sectionId=@sectionId,
@sectionName=@sectionName,
@establishmentId=@establishmentId,
@submissionId = @submissionId OUTPUT;

DECLARE @answerId INT
DECLARE @questionId INT

SELECT @answerId = Id
FROM dbo.answer
WHERE
answerText = @answerText
AND contentfulRef = @answerContentfulId

SELECT @questionId = Id
FROM dbo.question
WHERE
questionText = @questionText
AND contentfulRef = @questionContentfulId

IF @answerId IS NULL
BEGIN
INSERT INTO [dbo].[answer]
(answerText, contentfulRef)
VALUES
(@answerText, @answerContentfulId)
SELECT @answerId = SCOPE_IDENTITY()
END

IF @questionId IS NULL
BEGIN
INSERT INTO [dbo].[question]
(questionText, contentfulRef)
VALUES
(@questionText, @questionContentfulId)
SELECT @questionId = SCOPE_IDENTITY()
END

INSERT INTO [dbo].[response]
(userId, submissionId, questionId, answerId, maturity)
VALUES
(@userId, @submissionId, @questionId, @answerId, @maturity)

SELECT @responseId = SCOPE_IDENTITY()
COMMIT TRAN

END TRY
BEGIN CATCH
ROLLBACK TRAN
END CATCH

GO

-- Remove existing duplication of questions and answers

SELECT
MIN(id) questionId,
questionText,
contentfulRef
INTO #QuestionKeep
FROM dbo.question
GROUP BY questionText, contentfulRef

SELECT
MIN(id) answerId,
answerText,
contentfulRef
INTO #AnswerKeep
FROM dbo.answer
GROUP BY answerText, contentfulRef
GO

-- Disable trigger so that users don't see "Last completed" get updated to the time that this fix was run
DISABLE TRIGGER dbo.tr_response on dbo.response
GO

UPDATE R
SET questionId = QK.questionId,
answerId = AK.answerId
FROM dbo.response R
JOIN dbo.answer A ON R.answerId = A.id
JOIN dbo.question Q ON R.questionId = Q.id
JOIN #QuestionKeep QK ON Q.contentfulRef = QK.contentfulRef AND Q.questionText = QK.questionText
JOIN #AnswerKeep AK ON A.contentfulRef = AK.contentfulRef AND A.answerText = AK.answerText
GO

ENABLE TRIGGER dbo.tr_response on dbo.response
GO

-- deleteDataForEstablishment doesn't remove the questions and answers tied to the responses that get deleted
-- it now doesn't need to because the ids are shared between responses, but we can clean out all the old ones

DELETE Q
FROM dbo.question Q
LEFT JOIN dbo.response R ON Q.id = R.questionId
WHERE
R.id IS NULL

DELETE A
FROM dbo.answer A
LEFT JOIN dbo.response R ON A.id = R.answerId
WHERE
R.id IS NULL

DROP TABLE #QuestionKeep
DROP TABLE #AnswerKeep

GO
-- Add index on for dbo.response submissionId

CREATE INDEX IX_response_submissionId on dbo.response (submissionId)

GO
3 changes: 3 additions & 0 deletions tests/Dfe.PlanTech.Web.SeedTestData/Program.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
using System.Reflection;
using Dfe.PlanTech.Application.Caching.Interfaces;
using Dfe.PlanTech.Application.Caching.Models;
using Dfe.PlanTech.Domain.Persistence.Models;
using Dfe.PlanTech.Infrastructure.Data;
using Microsoft.EntityFrameworkCore;
Expand All @@ -19,6 +21,7 @@ private static ServiceProvider CreateServiceProvider()
var configuration = configurationBuilder.Build();
var services = new ServiceCollection();

services.AddSingleton<IQueryCacher, QueryCacher>();
services.AddDbContext<CmsDbContext>(opts =>
{
opts.UseSqlServer(configuration.GetConnectionString("Database"));
Expand Down
56 changes: 37 additions & 19 deletions tests/Dfe.PlanTech.Web.SeedTestData/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@ The standard E2E tests that run against each environment are kept quite generic

The purpose of this project, seeding with realistic mock content, is to support writing more complex and detailed end to end tests that rely on content being consistent.

This uses a very similar setup to the Azure Function E2E tests. See the [Readme](/tests/Dfe.PlanTech.CmsDbMigrations.E2ETests/README.md)
for a detailed guide on setting a mock database manually that this project can use.
## Requirements

- Either an MSSQL server instance
- Or an installation of [docker](https://www.docker.com/) (Recommended)

## How to use

Expand All @@ -20,30 +22,40 @@ To use this project to get a seeded database for testing you need to:
2. run the database upgrader against it to initialise it with the PlanTech schema
3. run this project to populate the database with test data.

There is a bash script to do this, or you can run each step manually
If you have docker installed, there is a bash script to do this, or you can run each step manually

### Bash script
### Docker instructions

#### Macbooks
#### Macbooks / Machines without sqlcmd

If you're running this locally on a macbook with an M1 chip, the setup script won't work as `sqlcmd` is not available on Arm64 chips.
If you're running this locally on a macbook with an Arm64 chip, or any other machine that doesn't support `sqlcmd`, the `setup.sh` script won't work.

Instead you can install `sql-cli` to your machine with
```bash
npm install -g sql-cli
```

Then run `./scripts/setup-mac.sh` from the project root and pass a server name and password as arguments. For example:
```bash
./scripts/setup.sh azuresqledge Pa5ssw0rd@G0esH3r3
```
Then run `./scripts/setup-arm.sh` from the project root and pass a server name and password as arguments. For example:
- Mac/Linux
```bash
./scripts/setup.sh azuresqledge Pa5ssw0rd@G0esH3r3
```
- Windows
```bash
sh scripts/setup.sh azuresqledge Pa5ssw0rd@G0esH3r3
```

#### Standard Setup

Other machines can use `sqlcmd` so you just need to run `scripts/setup.sh` with a server name and admin password as arguments. For example:
```bash
./scripts/setup.sh azuresqledge Pa5ssw0rd@G0esH3r3
```
If your machine does support `sqlcmd` you can skip installing `sql-cli` and just run `scripts/setup.sh` with a server name and admin password as arguments. For example:
- Mac/Linux
```bash
./scripts/setup.sh azuresqledge Pa5ssw0rd@G0esH3r3
```
- Windows
```bash
sh scripts/setup.sh azuresqledge Pa5ssw0rd@G0esH3r3
```

#### Note
- if you have permission errors when running either script, you can add execute permissions with chmod, for example:
Expand All @@ -52,13 +64,17 @@ chmod +x ./scripts/setup.sh
```


### Alternative setup
### Manual instructions

If having issues with the bash scripts or wanting more control over each step you can instead
If you don't have docker and have an MSSQL instance to use instead, you need to
1. Follow the steps in the [Azure E2E Tests setup](tests/Dfe.PlanTech.CmsDbMigrations.E2ETests/README.md) to create a database and run the database upgrader against it
2. Setup dotnet secrets for this project with the same connection string the Azure E2E test setup would require
3. Run this project to populate the database
1. Create a new database
2. Follow the instructions in the [Database Upgrader Readme](../../src/Dfe.PlanTech.DatabaseUpgrader/README.md) to run the database upgrader against your database
3. Set the connection string to the database in the dotnet secrets for this project using the command
```bash
dotnet user-secrets set "ConnectionStrings:Database" <connection_string>
```
4. Run this project to populate the database with the test data
### Using Plan Tech With Seeded Test Database
Expand All @@ -71,5 +87,7 @@ So using the above name and password for example:
Server=tcp:localhost,1433;Persist Security Info=False;User ID=sa;Password=Pa5ssw0rd@G0esH3r3;MultipleActiveResultSets=False;Encrypt=True;TrustServerCertificate=True;Connection Timeout=30;Max Pool Size=1000;Database=plantech-mock-db
```
and then run Plan Tech as normal.
Any required content that has not been mocked, (e.g. at the moment, the index page) will fallback to Contentful to find it,
so Plan tech continues to operate off dev content unless it has been replaced with a fixed test alternative.

0 comments on commit edf4d88

Please sign in to comment.