From cd10919fe318b6e6dec5026fd03ada899436112e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominique=20J=C3=A4ggi?= Date: Tue, 26 Dec 2023 17:31:23 +0100 Subject: [PATCH 1/2] feat: store previous audit result --- .../src/dto/audit.js | 12 +++++-- .../src/index.d.ts | 13 +++++++ .../src/models/audit.js | 17 +++++++++ .../src/service/audits/accessPatterns.js | 29 +++++++++++---- .../test/it/db.test.js | 21 +++++++++++ .../test/unit/models/audit.test.js | 22 ++++++++++++ .../test/unit/service/audits/index.test.js | 36 ++++++++++++++++++- 7 files changed, 139 insertions(+), 11 deletions(-) diff --git a/packages/spacecat-shared-data-access/src/dto/audit.js b/packages/spacecat-shared-data-access/src/dto/audit.js index e04712058..348195393 100644 --- a/packages/spacecat-shared-data-access/src/dto/audit.js +++ b/packages/spacecat-shared-data-access/src/dto/audit.js @@ -10,6 +10,8 @@ * governing permissions and limitations under the License. */ +import { isObject } from '@adobe/spacecat-shared-utils'; + import { createAudit } from '../models/audit.js'; function parseEpochToDate(epochInSeconds) { @@ -28,10 +30,10 @@ export const AuditDto = { /** * Converts an Audit object into a DynamoDB item. * @param {Readonly} audit - Audit object. - * @param {boolean} latestAudit - If true, returns the latest audit flavor. + * @param {boolean} isLatestAudit - If true, returns the latest audit flavor. * @returns {{siteId, auditedAt, auditResult, auditType, expiresAt, fullAuditRef, SK: string}} */ - toDynamoItem: (audit, latestAudit = false) => { + toDynamoItem: (audit, isLatestAudit = false) => { const GSI1PK = 'ALL_LATEST_AUDITS'; let GSI1SK; @@ -41,7 +43,7 @@ export const AuditDto = { GSI1SK = `${audit.getAuditType()}#${Object.values(audit.getScores()).join('#')}`; } - const latestAuditProps = latestAudit ? { GSI1PK, GSI1SK } : {}; + const latestAuditProps = isLatestAudit ? { GSI1PK, GSI1SK } : {}; return { siteId: audit.getSiteId(), @@ -53,6 +55,8 @@ export const AuditDto = { isLive: audit.isLive(), SK: `${audit.getAuditType()}#${audit.getAuditedAt()}`, ...latestAuditProps, + ...(isLatestAudit && isObject(audit.getPreviousAuditResult()) + && { previousAuditResult: audit.getPreviousAuditResult() }), }; }, @@ -70,6 +74,8 @@ export const AuditDto = { expiresAt: parseEpochToDate(dynamoItem.expiresAt), fullAuditRef: dynamoItem.fullAuditRef, isLive: dynamoItem.isLive, + ...(isObject(dynamoItem.previousAuditResult) + && { previousAuditResult: dynamoItem.previousAuditResult }), }; return createAudit(auditData); diff --git a/packages/spacecat-shared-data-access/src/index.d.ts b/packages/spacecat-shared-data-access/src/index.d.ts index 9d67c7f3d..00cd843e2 100644 --- a/packages/spacecat-shared-data-access/src/index.d.ts +++ b/packages/spacecat-shared-data-access/src/index.d.ts @@ -34,6 +34,19 @@ export interface Audit { */ getAuditResult: () => object; + /** + * Retrieves the result of the previous audit. + * This serves for comparison purposes. + * @returns {object|null} The parsed audit result. + */ + getPreviousAuditResult: () => object | null; + +/** + * Sets the result of the previous audit. + * @param {object} result The parsed audit result. + */ + setPreviousAuditResult: (result: object) => void; + /** * Retrieves the type of the audit. * @returns {object} The audit type. diff --git a/packages/spacecat-shared-data-access/src/models/audit.js b/packages/spacecat-shared-data-access/src/models/audit.js index 827c97a18..1d37499fc 100644 --- a/packages/spacecat-shared-data-access/src/models/audit.js +++ b/packages/spacecat-shared-data-access/src/models/audit.js @@ -36,6 +36,10 @@ const validateScores = (auditResult, auditType) => { return true; } + if (!isObject(auditResult.scores)) { + throw new Error(`Missing scores property for audit type '${auditType}'`); + } + const expectedProperties = AUDIT_TYPE_PROPERTIES[auditType]; if (!expectedProperties) { throw new Error(`Unknown audit type: ${auditType}`); @@ -66,6 +70,11 @@ const Audit = (data = {}) => { self.getFullAuditRef = () => self.state.fullAuditRef; self.isLive = () => self.state.isLive; self.isError = () => hasText(self.getAuditResult().runtimeError?.code); + self.getPreviousAuditResult = () => self.state.previousAuditResult; + self.setPreviousAuditResult = (previousAuditResult) => { + validateScores(previousAuditResult, self.getAuditType()); + self.state.previousAuditResult = previousAuditResult; + }; self.getScores = () => self.getAuditResult().scores; return Object.freeze(self); @@ -98,6 +107,14 @@ export const createAudit = (data) => { validateScores(data.auditResult, data.auditType); + if (data.previousAuditResult && !isObject(data.previousAuditResult)) { + throw new Error('Previous audit result must be an object'); + } + + if (data.previousAuditResult) { + validateScores(data.previousAuditResult, data.auditType); + } + if (!hasText(newState.fullAuditRef)) { throw new Error('Full audit ref must be provided'); } diff --git a/packages/spacecat-shared-data-access/src/service/audits/accessPatterns.js b/packages/spacecat-shared-data-access/src/service/audits/accessPatterns.js index f8f781344..aeeb599c5 100644 --- a/packages/spacecat-shared-data-access/src/service/audits/accessPatterns.js +++ b/packages/spacecat-shared-data-access/src/service/audits/accessPatterns.js @@ -184,25 +184,40 @@ export const addAudit = async ( log, auditData, ) => { - const audit = createAudit(auditData); + const newAudit = createAudit(auditData); const existingAudit = await getAuditForSite( dynamoClient, config, log, - audit.getSiteId(), - audit.getAuditType(), - audit.getAuditedAt(), + newAudit.getSiteId(), + newAudit.getAuditType(), + newAudit.getAuditedAt(), ); if (isObject(existingAudit)) { throw new Error('Audit already exists'); } + const latestAudit = await getLatestAuditForSite( + dynamoClient, + config, + log, + newAudit.getSiteId(), + newAudit.getAuditType(), + ); + + if (isObject(latestAudit)) { + newAudit.setPreviousAuditResult(latestAudit.getAuditResult()); + } + // TODO: Add transaction support - await dynamoClient.putItem(config.tableNameAudits, AuditDto.toDynamoItem(audit)); - await dynamoClient.putItem(config.tableNameLatestAudits, AuditDto.toDynamoItem(audit, true)); + await dynamoClient.putItem(config.tableNameAudits, AuditDto.toDynamoItem(newAudit)); + await dynamoClient.putItem( + config.tableNameLatestAudits, + AuditDto.toDynamoItem(newAudit, true), + ); - return audit; + return newAudit; }; /** diff --git a/packages/spacecat-shared-data-access/test/it/db.test.js b/packages/spacecat-shared-data-access/test/it/db.test.js index df1594de5..d151900bd 100644 --- a/packages/spacecat-shared-data-access/test/it/db.test.js +++ b/packages/spacecat-shared-data-access/test/it/db.test.js @@ -358,6 +358,27 @@ describe('DynamoDB Integration Test', async () => { expect(latestAudit.getSiteId()).to.equal(auditData.siteId); expect(latestAudit.getAuditType()).to.equal(auditData.auditType); expect(latestAudit.getAuditedAt()).to.equal(auditData.auditedAt); + + const additionalAuditData = { + siteId: 'https://example1.com', + auditType: AUDIT_TYPE_LHS_MOBILE, + auditedAt: new Date().toISOString(), + isLive: true, + fullAuditRef: 's3://ref', + auditResult: { + scores: { + performance: 1, + seo: 1, + accessibility: 1, + 'best-practices': 1, + }, + }, + }; + + const anotherAudit = await dataAccess.addAudit(additionalAuditData); + + checkAudit(anotherAudit); + expect(anotherAudit.getPreviousAuditResult()).to.deep.equal(newAudit.getAuditResult()); }); it('throws an error when adding a duplicate audit', async () => { diff --git a/packages/spacecat-shared-data-access/test/unit/models/audit.test.js b/packages/spacecat-shared-data-access/test/unit/models/audit.test.js index f82b01d4c..4f65a999a 100644 --- a/packages/spacecat-shared-data-access/test/unit/models/audit.test.js +++ b/packages/spacecat-shared-data-access/test/unit/models/audit.test.js @@ -48,6 +48,21 @@ describe('Audit Model Tests', () => { expect(() => createAudit({ ...validData, auditResult: 'not-an-object' })).to.throw('Audit result must be an object'); }); + it('throws an error if previous audit result is not an object', () => { + expect(() => createAudit({ ...validData, previousAuditResult: 'not-an-object' })) + .to.throw('Previous audit result must be an object'); + }); + + it('throws an error if previous audit result is missing scores property', () => { + expect(() => createAudit({ ...validData, previousAuditResult: {} })) + .to.throw('Missing scores property for audit type \'lhs-mobile\''); + }); + + it('throws an error if previous audit result is invalid', () => { + expect(() => createAudit({ ...validData, previousAuditResult: { scores: {} } })) + .to.throw('Missing expected property \'performance\' for audit type \'lhs-mobile\''); + }); + it('throws an error if fullAuditRef is not provided', () => { expect(() => createAudit({ ...validData, fullAuditRef: '' })).to.throw('Full audit ref must be provided'); }); @@ -62,6 +77,13 @@ describe('Audit Model Tests', () => { expect(audit.getAuditType()).to.equal(validData.auditType.toLowerCase()); expect(audit.getAuditResult()).to.deep.equal(validData.auditResult); expect(audit.getFullAuditRef()).to.equal(validData.fullAuditRef); + expect(audit.getPreviousAuditResult()).to.be.undefined; + }); + + it('throws an error when updating with invalid previous audit', () => { + const audit = createAudit(validData); + + expect(() => audit.setPreviousAuditResult({})).to.throw('Missing scores property for audit type \'lhs-mobile\''); }); it('automatically sets expiresAt if not provided', () => { diff --git a/packages/spacecat-shared-data-access/test/unit/service/audits/index.test.js b/packages/spacecat-shared-data-access/test/unit/service/audits/index.test.js index 07317a948..d2362525e 100644 --- a/packages/spacecat-shared-data-access/test/unit/service/audits/index.test.js +++ b/packages/spacecat-shared-data-access/test/unit/service/audits/index.test.js @@ -166,7 +166,7 @@ describe('Audit Access Pattern Tests', () => { let exportedFunctions; const auditData = { - siteId: 'siteId', + siteId: 'site1', auditType: 'lhs-mobile', auditedAt: new Date().toISOString(), auditResult: { @@ -204,6 +204,40 @@ describe('Audit Access Pattern Tests', () => { expect(result.getAuditResult()).to.deep.equal(auditData.auditResult); expect(result.getFullAuditRef()).to.equal(auditData.fullAuditRef); expect(result.getScores()).to.be.an('object'); + expect(result.getPreviousAuditResult()).to.be.undefined; + }); + + it('successfully adds a new audit with a previous audit result', async () => { + const auditResult = { + scores: { + performance: 0.2, + seo: 0.3, + accessibility: 0.4, + 'best-practices': 0.5, + }, + }; + mockDynamoClient.getItem.withArgs(TEST_DA_CONFIG.tableNameLatestAudits, { + siteId: 'site1', + auditType: 'lhs-mobile', + }).resolves({ ...auditData, auditResult }); + + const result = await exportedFunctions.addAudit(auditData); + + // Once for 'audits' and once for 'latest_audits' + expect(mockDynamoClient.putItem.calledTwice).to.be.true; + // Once for 'audits' and once for 'latest_audits' + expect(mockDynamoClient.getItem.calledTwice).to.be.true; + expect(result.getSiteId()).to.equal(auditData.siteId); + expect(result.getAuditType()).to.equal(auditData.auditType); + expect(result.getAuditedAt()).to.equal(auditData.auditedAt); + expect(result.getAuditResult()).to.deep.equal(auditData.auditResult); + expect(result.getFullAuditRef()).to.equal(auditData.fullAuditRef); + expect(result.getScores()).to.be.an('object'); + expect(result.getPreviousAuditResult()).to.be.an('object'); + expect(result.getPreviousAuditResult().scores.performance).to.equal(0.2); + expect(result.getPreviousAuditResult().scores.seo).to.equal(0.3); + expect(result.getPreviousAuditResult().scores.accessibility).to.equal(0.4); + expect(result.getPreviousAuditResult().scores['best-practices']).to.equal(0.5); }); it('successfully adds an error audit', async () => { From 4e3211ca8d933ea32a2961eebe62e7923384e966 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominique=20J=C3=A4ggi?= Date: Tue, 26 Dec 2023 17:34:54 +0100 Subject: [PATCH 2/2] fix: docs / optimization --- docs/API.md | 80 ++++++++++++++----- .../docs/schema.json | 4 + .../src/dto/audit.js | 9 ++- 3 files changed, 70 insertions(+), 23 deletions(-) diff --git a/docs/API.md b/docs/API.md index 32227de18..20baea457 100644 --- a/docs/API.md +++ b/docs/API.md @@ -1,17 +1,12 @@ -## Constants - -
-
createDataAccess ⇒ object
-

Creates a data access object.

-
-
- ## Functions
createClient(log, dbClient, docClient) ⇒ Object

Creates a client object for interacting with DynamoDB.

+
createResponse(body, status, headers) ⇒ Response
+

Creates a response with a JSON body. Defaults to 200 status.

+
isArray(value) ⇒ boolean

Determines if the given parameter is an array.

@@ -55,20 +50,14 @@ following UTC time offsets format.

arrayEquals(a, b) ⇒ boolean

Compares two arrays for equality. Supports primitive array item types only.

+
dateAfterDays(days) ⇒ Date
+

Calculates the date after a specified number of days from the current date.

+
+
resolveSecretsName(opts, ctx, defaultPath) ⇒ string
+

Resolves the name of the secret based on the function version.

+
- - -## createDataAccess ⇒ object -Creates a data access object. - -**Kind**: global constant -**Returns**: object - data access object - -| Param | Type | Description | -| --- | --- | --- | -| log | Logger | logger | - ## createClient(log, dbClient, docClient) ⇒ Object @@ -83,6 +72,20 @@ Creates a client object for interacting with DynamoDB. | dbClient | DynamoDB | The AWS SDK DynamoDB client instance. | | docClient | DynamoDBDocument | The AWS SDK DynamoDB Document client instance. | + + +## createResponse(body, status, headers) ⇒ Response +Creates a response with a JSON body. Defaults to 200 status. + +**Kind**: global function +**Returns**: Response - Response. + +| Param | Type | Description | +| --- | --- | --- | +| body | object | JSON body. | +| status | number | Optional status code. | +| headers | object | Optional headers. | + ## isArray(value) ⇒ boolean @@ -248,3 +251,40 @@ Compares two arrays for equality. Supports primitive array item types only. | a | Array | The first array to compare. | | b | Array | The second array to compare. | + + +## dateAfterDays(days) ⇒ Date +Calculates the date after a specified number of days from the current date. + +**Kind**: global function +**Returns**: Date - A new Date object representing the calculated date after the specified days. +**Throws**: + +- TypeError If the provided 'days' parameter is not a number. +- RangeError If the calculated date is outside the valid JavaScript date range. + + +| Param | Type | Description | +| --- | --- | --- | +| days | number | The number of days to add to the current date. | + +**Example** +```js +// Get the date 7 days from now +const sevenDaysLater = dateAfterDays(7); +console.log(sevenDaysLater); // Outputs a Date object representing the date 7 days from now +``` + + +## resolveSecretsName(opts, ctx, defaultPath) ⇒ string +Resolves the name of the secret based on the function version. + +**Kind**: global function +**Returns**: string - - The resolved secret name. + +| Param | Type | Description | +| --- | --- | --- | +| opts | Object | The options object, not used in this implementation. | +| ctx | Object | The context object containing the function version. | +| defaultPath | string | The default path for the secret. | + diff --git a/packages/spacecat-shared-data-access/docs/schema.json b/packages/spacecat-shared-data-access/docs/schema.json index fc99d3886..ca54d4953 100644 --- a/packages/spacecat-shared-data-access/docs/schema.json +++ b/packages/spacecat-shared-data-access/docs/schema.json @@ -218,6 +218,10 @@ "AttributeName": "auditResult", "AttributeType": "M" }, + { + "AttributeName": "previousAuditResult", + "AttributeType": "M" + }, { "AttributeName": "expiresAt", "AttributeType": "N" diff --git a/packages/spacecat-shared-data-access/src/dto/audit.js b/packages/spacecat-shared-data-access/src/dto/audit.js index 348195393..b1e5152ad 100644 --- a/packages/spacecat-shared-data-access/src/dto/audit.js +++ b/packages/spacecat-shared-data-access/src/dto/audit.js @@ -43,7 +43,12 @@ export const AuditDto = { GSI1SK = `${audit.getAuditType()}#${Object.values(audit.getScores()).join('#')}`; } - const latestAuditProps = isLatestAudit ? { GSI1PK, GSI1SK } : {}; + const latestAuditProps = isLatestAudit ? { + GSI1PK, + GSI1SK, + ...(isObject(audit.getPreviousAuditResult()) + && { previousAuditResult: audit.getPreviousAuditResult() }), + } : {}; return { siteId: audit.getSiteId(), @@ -55,8 +60,6 @@ export const AuditDto = { isLive: audit.isLive(), SK: `${audit.getAuditType()}#${audit.getAuditedAt()}`, ...latestAuditProps, - ...(isLatestAudit && isObject(audit.getPreviousAuditResult()) - && { previousAuditResult: audit.getPreviousAuditResult() }), }; },