MLPAB-1473: Add holding modal while reduced fee evidence documents are scanned #805

acsauk · 2023-10-27T11:51:30Z

Purpose

Adds a holding modal that initiates a SSE connection to monitor when files are scanned.

I also had to add some optimistic locking to dynamo PUTs as the virus scan results were sometimes arriving within milliseconds creating a race condition that was overwriting data. I've added 1 automatic retry to the lambda to try to mitigate this where we can but we'll need to see how this works to see if we need to rethink how we save the data (maybe to separate rows like the evidence-received event?) I've moved what was called Evidence on the Lpa into Documents on individual rows to account for this.

I also found the spinner component in the Home Office design system but it felt like overkill to install the whole design system for what is a few lines of CSS.

Fixes MLPAB-1473

…pending

codecov · 2023-10-27T12:26:53Z

Codecov Report

Attention: 62 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
internal/app/app.go	`86.66% <100.00%> (+0.36%)`	⬆️
internal/app/donor_store.go	`92.89% <100.00%> (+0.59%)`	⬆️
internal/page/data.go	`92.41% <ø> (-0.80%)`	⬇️
internal/page/document.go	`100.00% <100.00%> (ø)`
internal/page/donor/payment_confirmation.go	`100.00% <100.00%> (+8.16%)`	⬆️
internal/page/donor/register.go	`100.00% <100.00%> (ø)`
internal/page/donor/upload_evidence.go	`88.50% <100.00%> (+0.12%)`	⬆️
internal/page/donor/upload_evidence_sse.go	`100.00% <100.00%> (ø)`
internal/page/paths.go	`100.00% <ø> (ø)`
internal/validation/validation.go	`97.56% <ø> (ø)`
... and 6 more

📢 Thoughts on this report? Let us know!.

github-actions · 2023-10-27T12:43:23Z

PR Environment Terraform Plan Summary

Plan: 1 to add, 4 to change, 1 to destroy

…s added

internal/page/donor/upload_evidence.go

internal/page/donor/witnessing_your_signature.go

hawx · 2023-10-30T09:41:06Z

lang/en.json

@@ -840,5 +844,9 @@
    "lpaWithdrawn": "LPA withdrawn",
    "youHaveWithdrawnLpaNumber": "You have withdrawn LPA number <span class=\"govuk-!-font-weight-bold\">{{.UID}}</span>.",
    "opgWillNowContactAnyoneWhoHasAlreadyBeenContacted": "OPG will now contact anyone who has already been contacted in relation to your LPA and notify them of your decision to withdraw.",
-    "withdrawn": "Withdrawn"
+    "withdrawn": "Withdrawn",
+    "yourFilesAreUploading": "Your files are uploading",


the wording on this page is now weird, "Your files are uploading" even though they are in a section called "Uploaded files"

Missed this - it will make more sense to have the modal appear when clicking upload files rather than continue.

hawx · 2023-10-30T09:46:31Z

I also think it would be nice if it were stored in separate rows, I think you'd be able to simplify the polling then instead of getting the Lpa each time

…sing-lpa into MLPAB-1473

hawx · 2023-11-01T12:57:45Z

cmd/event-received/main.go

@@ -55,6 +55,11 @@ type shareCodeSender interface {
 	SendCertificateProvider(context.Context, notify.Template, page.AppData, bool, *page.Lpa) error
 }

+//go:generate mockery --testonly --inpackage --name DocumentStore --structname mockDocumentStore
+type DocumentStore interface {
+	UpdateScanResults(ctx context.Context, PK, SK string, virusDetected bool) error


keep pk, sk lowercase

hawx · 2023-11-01T12:59:03Z

cmd/event-received/main.go

-
-	if err := client.Put(ctx, lpa); err != nil {
-		return fmt.Errorf("failed to update LPA for '%s': %w", objectTagsAddedEventName, err)
+	err = documentStore.UpdateScanResults(ctx, lpaKey.PK, "#DOCUMENT#"+objectKey, hasVirus)


It would be good to push #DOCUMENT# into the store, so outside only has to deal with IDs and not how it is stored in dynamo. And the same with changing PK to be the Lpa ID (even though it is awkward)

Much simpler - I was going around in circles with circular dependencies but makes more sense to hide the detail.

hawx · 2023-11-01T13:02:28Z

internal/app/app.go

@@ -49,13 +50,16 @@ type DynamoClient interface {
 	Put(ctx context.Context, v interface{}) error
 	Create(ctx context.Context, v interface{}) error
 	DeleteKeys(ctx context.Context, keys []dynamo.Key) error
+	DeleteOne(ctx context.Context, key dynamo.Key) error


maybe I should have made it DeleteKeys(ctx context.Context, keys ...dynamo.Key)

hawx · 2023-11-01T13:04:16Z

internal/app/app.go

@@ -49,13 +50,16 @@ type DynamoClient interface {
 	Put(ctx context.Context, v interface{}) error
 	Create(ctx context.Context, v interface{}) error
 	DeleteKeys(ctx context.Context, keys []dynamo.Key) error
+	DeleteOne(ctx context.Context, key dynamo.Key) error
+	Update(ctx context.Context, input *dynamodb.UpdateItemInput) error


I'm not sure about exposing the dynamodb type here, we avoid it in the other signatures. Would Update(ctx context.Context, pk, sk string, values map[string]types.AttributeValue, expression string) error be any nicer?

That works for me - slightly less boilerplate code to be passing in.

hawx · 2023-11-01T13:06:37Z

internal/app/document_store.go

+}
+
+func NewDocumentStore(dynamoClient DynamoClient, s3Client S3Client) DocumentStore {
+	return documentStore{dynamoClient: dynamoClient, s3Client: s3Client}


probably makes sense to have this return &documentStore and change all the receivers to (s *documentStore)

hawx · 2023-11-01T13:16:52Z

internal/app/donor_store.go

+					if document.Sent.IsZero() && !document.Scanned {
+						document.Sent = s.now()
+						if err := s.documentStore.Put(ctx, document, nil); err != nil {
+							return err


We shouldn't be returning errors from here. It should just fail, log, and retry next time

also would be nice to batch these changes to documents up (but don't worry if it ends up being too much)

hawx · 2023-11-01T13:18:06Z

internal/dynamo/client.go

-		var updatedAtTime time.Time
-		err = attributevalue.Unmarshal(updatedAt, &updatedAtTime)
+	// Tracking Value equality against data on write allows for optimistic locking
+	if currentVersion, exists := item["Version"]; exists {


do we still need this if documents are split out?

I think there's still a (admittedly small) chance that an LPA could be updated at the same time by a donor and an event coming in. Maybe we could: look at seeing if there's a way to restructure all event data persistence to rows to be safe or stick with optimistic locking OR remove and monitor once we go live 🤷

hawx · 2023-11-01T13:19:23Z

internal/page/data.go

@@ -177,56 +177,101 @@ type Lpa struct {
 	HasSentPreviousApplicationLinkedEvent bool
 }

-type Evidence struct {
-	Documents []Document
+type Document struct {


could all the document related stuff live in a new file

hawx · 2023-11-01T13:23:29Z

internal/page/donor/payment_confirmation.go

-			for i, evidence := range lpa.Evidence.Documents {
-				if evidence.Sent.IsZero() {
-					err := evidenceS3Client.PutObjectTagging(r.Context(), evidence.Key, []types.Tag{
+			documents, err := documentStore.GetAll(r.Context())


do we need all of this since it happens in payHelper.Pay?

It does, but not for HalfFee - we wanted to be sure that the payment had been successful before tagging the docs as being ready for replication hence the replication.

hawx · 2023-11-01T13:25:46Z

internal/page/donor/upload_evidence.go

+							Filename: file.Filename,
+							Key:      key,
+						}
+						if err := documentStore.Put(r.Context(), document, file.Data); err != nil {


it would be nice to provide a documentStore.Create(ctx, lpa.ID, key, file.Filename) method, then you won't have LPA# and #DOCUMENT# exposed here

hawx · 2023-11-02T07:39:50Z

cmd/event-received/main.go

-	var lpaKey dynamo.Key
-	if err := dynamodbClient.OneByUID(ctx, parts[0], &lpaKey); err != nil {
-		return fmt.Errorf("failed to resolve uid for '%s': %w", objectTagsAddedEventName, err)
+	lpa, err := getLpaByUID(ctx, dynamodbClient, parts[0], objectTagsAddedEventName)


is it worth introducing another dynamo call vs _, lpaID, _ := strings.Cut(lpaKey.PK, "#")

hawx · 2023-11-02T07:42:06Z

internal/app/document_store.go

-		}
-
-		return []page.Document{}, err
+	if err := s.dynamoClient.AllByPartialSk(ctx, lpaKey(data.LpaID), documentKey(""), &ds); err != nil && !errors.Is(err, dynamo.NotFoundError{}) {


I hadn't thought before, but AllByPartialSk doesn't return dynamo.NotFoundError, we only do that in the One... methods

hawx · 2023-11-02T07:45:38Z

internal/app/document_store.go

-	return s.dynamoClient.Update(ctx, input)
-}
+	toWrite := len(converted)
+	written, err := s.dynamoClient.BatchPut(ctx, converted)


I shouldn't have mentioned this, it was fine before. For batch stuff I think TransactWriteItem is better because it either works or fails

acsauk added 12 commits October 24, 2023 14:45

add dialog with loading spinner

2db2ddd

trigger files uploadig on continue button click

faf047b

add dummy random events endpoint

16a5adc

update upload count and submit form when finished

70b2542

show errors on infected files

99fbf87

cover pay action

ba65a6b

lock modal focus

5d6dd88

close SSE connection on esc

3ab6d0b

ensure only scanned documents are sent, redirect from upload on task …

8f0ae1b

…pending

cover virus uploads in e2e

6586032

tidy and test fixes

f80f6f1

Merge branch 'main' into MLPAB-1473

c562a25

acsauk temporarily deployed to dev_805mlpab147 October 27, 2023 12:54 — with GitHub Actions Inactive

acsauk added 3 commits October 27, 2023 15:40

add ttl to sse

df8b323

add auto-increment version to LPA, add retry when handling object tag…

3642c00

…s added

handle ttl expiration

cc619e3

acsauk had a problem deploying to dev_805mlpab147 October 29, 2023 14:02 — with GitHub Actions Failure

ensure we have a fresh lpa when sending independent witness codes

b87d069

acsauk temporarily deployed to dev_805mlpab147 October 29, 2023 15:01 — with GitHub Actions Inactive

Merge branch 'main' into MLPAB-1473

bf90939

acsauk marked this pull request as ready for review October 30, 2023 08:41

acsauk requested a review from a team as a code owner October 30, 2023 08:41

acsauk temporarily deployed to dev_805mlpab147 October 30, 2023 09:08 — with GitHub Actions Inactive

hawx reviewed Oct 30, 2023

View reviewed changes

acsauk added 3 commits October 30, 2023 16:11

move infected file error building to FilesInfectedError

922c48f

move documents to individual rows

c1f423d

update mocks

f32bea7

acsauk added 8 commits October 31, 2023 16:42

cover upload_evidence_sse

3346ca8

cover upload_evidence

048321c

cover payHelper

adccd28

cover payment_confirmation

01057ef

more coverage

27e2e39

fix payments e2e

362049c

Merge branch 'main' into MLPAB-1473

65abb2a

drop unrequired namespace

cb31c93

acsauk temporarily deployed to dev_805mlpab147 October 31, 2023 22:25 — with GitHub Actions Inactive

acsauk added 2 commits November 1, 2023 10:40

Merge branch 'main' into MLPAB-1473

6403b44

Merge branch 'MLPAB-1473' of github.com:ministryofjustice/opg-moderni…

61c4897

…sing-lpa into MLPAB-1473

acsauk temporarily deployed to dev_805mlpab147 November 1, 2023 11:09 — with GitHub Actions Inactive

acsauk added 3 commits November 1, 2023 12:12

provide UpdateItem permissions to lambda

06e83f0

rename modal JS

8e2d42b

bump SSE ttl

aed4c62

acsauk temporarily deployed to dev_805mlpab147 November 1, 2023 12:50 — with GitHub Actions Inactive

hawx reviewed Nov 1, 2023

View reviewed changes

acsauk added 3 commits November 1, 2023 19:01

fix up

447c1c9

fix lambda sk

878a128

Merge branch 'main' into MLPAB-1473

baebc5d

acsauk deployed to dev_805mlpab147 November 1, 2023 19:59 — with GitHub Actions View deployment

hawx approved these changes Nov 2, 2023

View reviewed changes

acsauk merged commit eb7e9b4 into main Nov 2, 2023
24 checks passed

acsauk deleted the MLPAB-1473 branch November 2, 2023 10:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MLPAB-1473: Add holding modal while reduced fee evidence documents are scanned #805

MLPAB-1473: Add holding modal while reduced fee evidence documents are scanned #805

acsauk commented Oct 27, 2023 •

edited

Loading

codecov bot commented Oct 27, 2023 •

edited

Loading

github-actions bot commented Oct 27, 2023 •

edited

Loading

hawx Oct 30, 2023

acsauk Oct 30, 2023

hawx commented Oct 30, 2023

hawx Nov 1, 2023

hawx Nov 1, 2023

acsauk Nov 1, 2023

hawx Nov 1, 2023

hawx Nov 1, 2023

acsauk Nov 1, 2023

hawx Nov 1, 2023

hawx Nov 1, 2023

hawx Nov 1, 2023 •

edited

Loading

hawx Nov 1, 2023

acsauk Nov 1, 2023

hawx Nov 1, 2023

hawx Nov 1, 2023

acsauk Nov 1, 2023

hawx Nov 1, 2023

hawx Nov 2, 2023

hawx Nov 2, 2023

hawx Nov 2, 2023

MLPAB-1473: Add holding modal while reduced fee evidence documents are scanned #805

MLPAB-1473: Add holding modal while reduced fee evidence documents are scanned #805

Conversation

acsauk commented Oct 27, 2023 • edited Loading

Purpose

codecov bot commented Oct 27, 2023 • edited Loading

Codecov Report

github-actions bot commented Oct 27, 2023 • edited Loading

PR Environment Terraform Plan Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawx commented Oct 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawx Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acsauk commented Oct 27, 2023 •

edited

Loading

codecov bot commented Oct 27, 2023 •

edited

Loading

github-actions bot commented Oct 27, 2023 •

edited

Loading

hawx Nov 1, 2023 •

edited

Loading