From ed8858cc905874a7fc2a42b05bd414809ba61021 Mon Sep 17 00:00:00 2001 From: Ben Helleman Date: Mon, 23 Sep 2024 16:43:40 -0400 Subject: [PATCH 1/3] fix: SITES-25634 [Importer] Expose hasCustomImportJs and hasCustomHeaders as two new fields on a import job. --- .../docs/schema.json | 8 + .../src/dto/import-job.js | 4 + .../src/index.d.ts | 10 + .../src/models/importer/import-job.js | 35 ++- .../test/it/db.test.js | 6 + .../unit/models/importer/import-job.test.js | 27 +++ .../unit/service/import-job/index.test.js | 211 +++++------------- 7 files changed, 144 insertions(+), 157 deletions(-) diff --git a/packages/spacecat-shared-data-access/docs/schema.json b/packages/spacecat-shared-data-access/docs/schema.json index 1771cb03..20caf987 100644 --- a/packages/spacecat-shared-data-access/docs/schema.json +++ b/packages/spacecat-shared-data-access/docs/schema.json @@ -986,6 +986,14 @@ { "AttributeName": "initiatedBy", "AttributeType": "M" + }, + { + "AttributeName": "hasCustomHeaders", + "AttributeType": "B" + }, + { + "AttributeName": "hasCustomImportJs", + "AttributeType": "B" } ], "GlobalSecondaryIndexes": [ diff --git a/packages/spacecat-shared-data-access/src/dto/import-job.js b/packages/spacecat-shared-data-access/src/dto/import-job.js index bb01102c..e6797db4 100644 --- a/packages/spacecat-shared-data-access/src/dto/import-job.js +++ b/packages/spacecat-shared-data-access/src/dto/import-job.js @@ -40,6 +40,8 @@ export const ImportJobDto = { redirectCount: importJob.getRedirectCount(), importQueueId: importJob.getImportQueueId(), initiatedBy: importJob.getInitiatedBy(), + hasCustomHeaders: importJob.hasCustomHeaders(), + hasCustomImportJs: importJob.hasCustomImportJs(), GSI1PK: 'ALL_IMPORT_JOBS', }), @@ -64,6 +66,8 @@ export const ImportJobDto = { redirectCount: dynamoItem.redirectCount, importQueueId: dynamoItem.importQueueId, initiatedBy: dynamoItem.initiatedBy, + hasCustomHeaders: dynamoItem.hasCustomHeaders, + hasCustomImportJs: dynamoItem.hasCustomImportJs, }; return createImportJob(importJobData); diff --git a/packages/spacecat-shared-data-access/src/index.d.ts b/packages/spacecat-shared-data-access/src/index.d.ts index f8792576..5354db2b 100644 --- a/packages/spacecat-shared-data-access/src/index.d.ts +++ b/packages/spacecat-shared-data-access/src/index.d.ts @@ -535,6 +535,16 @@ export interface ImportJob { * Retrieves the initiatedBy metadata (name, imsOrgId, imsUserId, userAgent) of the import job. */ getInitiatedBy: () => object; + + /** + * Indicates if the import job has custom headers. + */ + hasCustomHeaders: () => boolean; + + /** + * Indicates if the import job has custom import js. + */ + hasCustomImportJs: () => boolean; } export interface ImportUrl { diff --git a/packages/spacecat-shared-data-access/src/models/importer/import-job.js b/packages/spacecat-shared-data-access/src/models/importer/import-job.js index df852e74..5731c301 100644 --- a/packages/spacecat-shared-data-access/src/models/importer/import-job.js +++ b/packages/spacecat-shared-data-access/src/models/importer/import-job.js @@ -11,7 +11,7 @@ */ import { - hasText, isIsoDate, isValidUrl, isObject, isString, isNumber, isInteger, + hasText, isIsoDate, isValidUrl, isObject, isString, isNumber, isInteger, isBoolean, } from '@adobe/spacecat-shared-utils'; import { Base } from '../base.js'; import { ImportJobStatus, ImportOptions } from './import-constants.js'; @@ -39,6 +39,8 @@ const ImportJob = (data) => { self.getRedirectCount = () => self.state.redirectCount; self.getImportQueueId = () => self.state.importQueueId; self.getInitiatedBy = () => self.state.initiatedBy; + self.hasCustomHeaders = () => self.state.hasCustomHeaders || false; + self.hasCustomImportJs = () => self.state.hasCustomImportJs || false; /** * Updates the state of the ImportJob. @@ -147,6 +149,29 @@ const ImportJob = (data) => { }, ); + /** + * Update the hasCustomHeaders value to true if the ImportJob has custom headers, false otherwise. + * @param {boolean} hasCustomHeaders - The new value for hasCustomHeaders. + * @return {ImportJob} The updated ImportJob object. + */ + self.updateHasCustomHeaders = (hasCustomHeaders) => updateState('hasCustomHeaders', hasCustomHeaders, (value) => { + if (!isBoolean(value)) { + throw new Error(`Invalid hasCustomHeaders value: ${value}`); + } + }); + + /** + * Update the hasCustomImportJs value to true if the ImportJob has custom import js, false + * otherwise. + * @param {boolean} hasCustomImportJs - The new value for hasCustomImportJs. + * @return {ImportJob} The updated ImportJob object. + */ + self.updateHasCustomImportJs = (hasCustomImportJs) => updateState('hasCustomImportJs', hasCustomImportJs, (value) => { + if (!isBoolean(value)) { + throw new Error(`Invalid hasCustomImportJs value: ${value}`); + } + }); + return Object.freeze(self); }; @@ -215,5 +240,13 @@ export const createImportJob = (data) => { }); } + if (!isBoolean(newState.hasCustomImportJs)) { + throw new Error(`Invalid hasCustomImportJs value: ${newState.hasCustomImportJs}`); + } + + if (!isBoolean(newState.hasCustomHeaders)) { + throw new Error(`Invalid hasCustomHeaders value: ${newState.hasCustomHeaders}`); + } + return ImportJob(newState); }; 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 3b84f634..8d0f7a2a 100644 --- a/packages/spacecat-shared-data-access/test/it/db.test.js +++ b/packages/spacecat-shared-data-access/test/it/db.test.js @@ -1022,6 +1022,8 @@ describe('DynamoDB Integration Test', async () => { options: { [ImportOptions.ENABLE_JAVASCRIPT]: true, }, + hasCustomImportJs: true, + hasCustomHeaders: false, }); // helper @@ -1036,6 +1038,8 @@ describe('DynamoDB Integration Test', async () => { const job = await createNewImportJob(); expect(job.getId()).to.be.a('string'); expect(job.getCreatedAt()).to.be.a('string'); + expect(job.hasCustomHeaders()).to.be.false; + expect(job.hasCustomImportJs()).to.be.true; }); it('Verify updateImportJob', async () => { @@ -1047,6 +1051,7 @@ describe('DynamoDB Integration Test', async () => { newJob.updateDuration(1234); newJob.updateUrlCount(100); newJob.updateImportQueueId('Q-456'); + newJob.updateHasCustomHeaders(true); const updatedJob = await dataAccess.updateImportJob(newJob); @@ -1058,6 +1063,7 @@ describe('DynamoDB Integration Test', async () => { expect(updatedJob.getOptions()).to.deep.equal({ [ImportOptions.ENABLE_JAVASCRIPT]: true, }); + expect(updatedJob.hasCustomHeaders()).to.be.true; }); it('Verify getImportJobsByStatus', async () => { diff --git a/packages/spacecat-shared-data-access/test/unit/models/importer/import-job.test.js b/packages/spacecat-shared-data-access/test/unit/models/importer/import-job.test.js index e8d70174..4748bd78 100644 --- a/packages/spacecat-shared-data-access/test/unit/models/importer/import-job.test.js +++ b/packages/spacecat-shared-data-access/test/unit/models/importer/import-job.test.js @@ -28,6 +28,8 @@ const validImportJob = { initiatedBy: { apiKeyName: 'test', }, + hasCustomHeaders: false, + hasCustomImportJs: false, }; describe('ImportJob Model tests', () => { describe('Validation Tests', () => { @@ -112,6 +114,11 @@ describe('ImportJob Model tests', () => { const importJob = createImportJob({ ...validImportJob, startTime: '' }); expect(importJob.getStartTime()).to.match(/^20/); }); + + it('create an import job with invalid custom fields', () => { + expect(() => createImportJob({ ...validImportJob, hasCustomHeaders: 'x' })).to.throw('Invalid hasCustomHeaders value: x'); + expect(() => createImportJob({ ...validImportJob, hasCustomImportJs: 'x' })).to.throw('Invalid hasCustomImportJs value: x'); + }); }); describe('Import Job Functionality Tests', () => { @@ -171,6 +178,26 @@ describe('ImportJob Model tests', () => { expect(() => importJob.updateRedirectCount(-1)).to.throw('Invalid redirect count during update: -1'); }); + it('update hasCustomHeaders of import job', () => { + importJob.updateHasCustomHeaders(true); + expect(importJob.hasCustomHeaders()).to.equal(true); + + importJob.updateHasCustomHeaders(false); + expect(importJob.hasCustomHeaders()).to.equal(false); + + expect(() => importJob.updateHasCustomHeaders(undefined)).to.throw('Invalid hasCustomHeaders value: undefined'); + }); + + it('update hasCustomImportJs of import job', () => { + importJob.updateHasCustomImportJs(true); + expect(importJob.hasCustomImportJs()).to.equal(true); + + importJob.updateHasCustomImportJs(false); + expect(importJob.hasCustomImportJs()).to.equal(false); + + expect(() => importJob.updateHasCustomImportJs(undefined)).to.throw('Invalid hasCustomImportJs value: undefined'); + }); + it('updates import queue id of import job', () => { importJob.updateImportQueueId('123'); expect(importJob.getImportQueueId()).to.equal('123'); diff --git a/packages/spacecat-shared-data-access/test/unit/service/import-job/index.test.js b/packages/spacecat-shared-data-access/test/unit/service/import-job/index.test.js index 969a8b36..e533eb9e 100644 --- a/packages/spacecat-shared-data-access/test/unit/service/import-job/index.test.js +++ b/packages/spacecat-shared-data-access/test/unit/service/import-job/index.test.js @@ -18,7 +18,6 @@ import sinon from 'sinon'; import sinonChai from 'sinon-chai'; import { importJobFunctions } from '../../../../src/service/import-job/index.js'; import { createImportJob } from '../../../../src/models/importer/import-job.js'; -import { ImportUrlStatus } from '../../../../src/index.js'; use(sinonChai); use(chaiAsPromised); @@ -51,149 +50,79 @@ describe('Import Job Tests', () => { ); }); - describe('getImportJobByID', () => { - it('should return an ImportJobDto when an item is found', async () => { - const mockImportJob = { - id: 'test-id', - status: 'RUNNING', - options: {}, - baseURL: 'https://www.test.com', - hashedApiKey: '4c806362b613f7496abf284146efd31da90e4b16169fe001841ca17290f427c4', - importQueueId: 'test-import-queue-id', - initiatedBy: { - apiKeyName: 'test-user', - imsUserId: 'test-ims-user-id', - imsOrgId: 'test-ims-org-id', - userAgent: 'test-user-agent', - }, - }; - - const urls = []; - Object.values(ImportUrlStatus).forEach((status) => { - const mockImportUrl = { - id: `test-import-url-${status}`, - jobId: 'test-id', - status, - url: `https://www.test.com/${status}`, - }; - urls.push(mockImportUrl); - }); - - mockDynamoClient.getItem.resolves(mockImportJob); - mockDynamoClient.query.resolves(urls); - - const result = await exportedFunctions.getImportJobByID('test-id'); - - expect(result.state.id).to.equal('test-id'); - }); + const mockImportJob = { + id: 'test-id', + status: 'RUNNING', + options: {}, + baseURL: 'https://www.test.com', + hashedApiKey: '4c806362b613f7496abf284146efd31da90e4b16169fe001841ca17290f427c4', + importQueueId: 'test-import-queue-id', + initiatedBy: { + apiKeyName: 'test-user', + imsUserId: 'test-ims-user-id', + imsOrgId: 'test-ims-org-id', + userAgent: 'test-user-agent', + }, + hasCustomImportJs: false, + hasCustomHeaders: false, + }; + describe('getImportJobByID', () => { it('should return null if item is not found', async () => { - mockDynamoClient.getItem.resolves(undefined); - + mockDynamoClient.getItem.resolves(null); const result = await exportedFunctions.getImportJobByID('test-id'); - expect(result).to.equal(null); }); }); describe('getImportJobsByDateRange', () => { - it('should return ImportJobDto[] if items are found', async () => { - const mockImportJobs = [{ - id: 'test-id', - status: 'RUNNING', - options: {}, - baseURL: 'https://www.test.com', - hashedApiKey: '4c806362b613f7496abf284146efd31da90e4b16169fe001841ca17290f427c4', - startTime: '2024-05-28T14:26:00.000Z', - importQueueId: 'test-import-queue-id', - initiatedBy: { - apiKeyName: 'test-user', - imsUserId: 'test-ims-user-id', - imsOrgId: 'test-ims-org-id', - userAgent: 'test-user-agent', - }, + it( + 'Verify that getImportJobsByDateRange perform no additional changes to the result size', + async () => { + const mockImportJobs = [ + { ...mockImportJob, id: 'test-1' }, + { ...mockImportJob, id: 'test-2' }, + ]; + mockDynamoClient.query.resolves(mockImportJobs); + + const result = await exportedFunctions.getImportJobsByDateRange( + mockDynamoClient, + TEST_DA_CONFIG, + mockLog, + '2024-05-27T14:26:00.000Z', + '2024-06-02T14:26:00.000Z', + ); + + // expect that 2 results were in the db, and that the results were not modified + expect(result).to.be.an('array').and.to.have.lengthOf(2); }, - { - id: 'test-id-1', - status: 'RUNNING', - options: {}, - baseURL: 'https://www.test1.com', - hashedApiKey: '5c806362b613f7496abf284146efd31da90e4b16169fe001841ca17290f427c4', - startTime: '2024-06-01T14:26:00.000Z', - importQueueId: 'test-import-queue-id-1', - initiatedBy: { - apiKeyName: 'test-user', - imsUserId: 'test-ims-user-id', - imsOrgId: 'test-ims-org-id', - userAgent: 'test-user-agent', - }, - }]; - mockDynamoClient.query.resolves(mockImportJobs); - - const result = await exportedFunctions.getImportJobsByDateRange(mockDynamoClient, TEST_DA_CONFIG, mockLog, '2024-05-27T14:26:00.000Z', '2024-06-02T14:26:00.000Z'); - - expect(result).to.be.an('array').and.to.have.lengthOf(2); - }); + ); }); describe('getImportJobsByStatus', () => { - it('should return ImportJobDto[] if items are found', async () => { - const mockImportJobs = [{ - id: 'test-id', - status: 'RUNNING', - options: {}, - baseURL: 'https://www.test.com', - hashedApiKey: '4c806362b613f7496abf284146efd31da90e4b16169fe001841ca17290f427c4', - importQueueId: 'test-import-queue-id', - initiatedBy: { - apiKeyName: 'test-user', - imsUserId: 'test-ims-user-id', - imsOrgId: 'test-ims-org-id', - userAgent: 'test-user-agent', - }, - }, - { - id: 'test-id-1', - status: 'RUNNING', - options: {}, - baseURL: 'https://www.test1.com', - hashedApiKey: '5c806362b613f7496abf284146efd31da90e4b16169fe001841ca17290f427c4', - importQueueId: 'test-import-queue-id-1', - initiatedBy: { - apiKeyName: 'test-user', - imsUserId: 'test-ims-user-id', - imsOrgId: 'test-ims-org-id', - userAgent: 'test-user-agent', - }, - }]; + it('Verify that getImportJobsByStatus perform no additional changes to the result size', async () => { + const mockImportJobs = [ + { ...mockImportJob, id: 'test-1' }, + { ...mockImportJob, id: 'test-2' }, + ]; + mockDynamoClient.query.resolves(mockImportJobs); - const result = await exportedFunctions.getImportJobsByStatus(mockDynamoClient, TEST_DA_CONFIG, mockLog, 'test-status'); + const result = await exportedFunctions.getImportJobsByStatus( + mockDynamoClient, + TEST_DA_CONFIG, + mockLog, + 'dummy-status', + ); + // verify that 2 results were in the db, and that the results were not modified expect(result).to.be.an('array').and.to.have.lengthOf(2); }); }); describe('createNewImportJob', () => { it('should create a new ImportJob', async () => { - const mockImportJobData = { - id: 'test-id', - status: 'RUNNING', - options: {}, - baseURL: 'https://www.test.com', - hashedApiKey: '4c806362b613f7496abf284146efd31da90e4b16169fe001841ca17290f427c4', - importQueueId: 'test-import-queue-id', - initiatedBy: { - apiKeyName: 'test-user', - imsUserId: 'test-ims-user-id', - imsOrgId: 'test-ims-org-id', - userAgent: 'test-user-agent', - }, - }; - const result = await exportedFunctions.createNewImportJob( - mockImportJobData, - ); - + const result = await exportedFunctions.createNewImportJob(mockImportJob); expect(result.state.initiatedBy.apiKeyName).to.equal('test-user'); expect(result.state.baseURL).to.equal('https://www.test.com'); }); @@ -201,45 +130,15 @@ describe('Import Job Tests', () => { describe('updateImportJob', () => { it('should update an existing ImportJob', async () => { - const mockImportJobData = { - id: 'test-id', - status: 'RUNNING', - options: {}, - baseURL: 'https://www.test.com', - hashedApiKey: '4c806362b613f7496abf284146efd31da90e4b16169fe001841ca17290f427c4', - importQueueId: 'test-import-queue-id', - initiatedBy: { - apiKeyName: 'test-user', - imsUserId: 'test-ims-user-id', - imsOrgId: 'test-ims-org-id', - userAgent: 'test-user-agent', - }, - }; - mockDynamoClient.getItem.resolves(mockImportJobData); - + mockDynamoClient.getItem.resolves(mockImportJob); const importJob = await exportedFunctions.getImportJobByID('test-id'); importJob.updateStatus('COMPLETE'); - const result = await exportedFunctions.updateImportJob( - importJob, - ); + const result = await exportedFunctions.updateImportJob(importJob); expect(result.getStatus()).to.equal('COMPLETE'); }); it('should throw an error if the ImportJob does not exist', async () => { - const mockImportJobData = { - id: 'test-id', - status: 'RUNNING', - hashedApiKey: '4c806362b613f7496abf284146efd31da90e4b16169fe001841ca17290f427c4', - options: {}, - baseURL: 'https://www.test.com', - initiatedBy: { - apiKeyName: 'test-user', - imsUserId: 'test-ims-user-id', - imsOrgId: 'test-ims-org-id', - userAgent: 'test-user-agent', - }, - }; - const importJob = createImportJob(mockImportJobData); + const importJob = createImportJob(mockImportJob); const result = exportedFunctions.updateImportJob(importJob); expect(result).to.be.rejectedWith('Import Job with id:test-id does not exist'); }); From 0464d128a54b9ec3eb5cf581e748ade12e4be56b Mon Sep 17 00:00:00 2001 From: Ben Helleman Date: Mon, 23 Sep 2024 16:52:44 -0400 Subject: [PATCH 2/3] fix: SITES-25634 [Importer] Expose hasCustomImportJs and hasCustomHeaders as two new fields on a import job. * hasCustomHeaders and hasCustomImportJs are optional, and default to false --- .../src/models/importer/import-job.js | 4 ++-- .../unit/models/importer/import-job.test.js | 17 ++++++++++++++--- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/packages/spacecat-shared-data-access/src/models/importer/import-job.js b/packages/spacecat-shared-data-access/src/models/importer/import-job.js index 5731c301..5d48a6c8 100644 --- a/packages/spacecat-shared-data-access/src/models/importer/import-job.js +++ b/packages/spacecat-shared-data-access/src/models/importer/import-job.js @@ -240,11 +240,11 @@ export const createImportJob = (data) => { }); } - if (!isBoolean(newState.hasCustomImportJs)) { + if (newState.hasCustomImportJs && !isBoolean(newState.hasCustomImportJs)) { throw new Error(`Invalid hasCustomImportJs value: ${newState.hasCustomImportJs}`); } - if (!isBoolean(newState.hasCustomHeaders)) { + if (newState.hasCustomHeaders && !isBoolean(newState.hasCustomHeaders)) { throw new Error(`Invalid hasCustomHeaders value: ${newState.hasCustomHeaders}`); } diff --git a/packages/spacecat-shared-data-access/test/unit/models/importer/import-job.test.js b/packages/spacecat-shared-data-access/test/unit/models/importer/import-job.test.js index 4748bd78..b8a70c72 100644 --- a/packages/spacecat-shared-data-access/test/unit/models/importer/import-job.test.js +++ b/packages/spacecat-shared-data-access/test/unit/models/importer/import-job.test.js @@ -28,8 +28,6 @@ const validImportJob = { initiatedBy: { apiKeyName: 'test', }, - hasCustomHeaders: false, - hasCustomImportJs: false, }; describe('ImportJob Model tests', () => { describe('Validation Tests', () => { @@ -115,8 +113,21 @@ describe('ImportJob Model tests', () => { expect(importJob.getStartTime()).to.match(/^20/); }); - it('create an import job with invalid custom fields', () => { + it('create an import job with custom fields', () => { + let headersJob = createImportJob({ ...validImportJob, hasCustomHeaders: true }); + expect(headersJob.hasCustomHeaders()).to.be.true; + headersJob = createImportJob({ ...validImportJob, hasCustomHeaders: false }); + expect(headersJob.hasCustomHeaders()).to.be.false; + headersJob = createImportJob({ ...validImportJob }); + expect(headersJob.hasCustomHeaders()).to.be.false; expect(() => createImportJob({ ...validImportJob, hasCustomHeaders: 'x' })).to.throw('Invalid hasCustomHeaders value: x'); + + let importJsJob = createImportJob({ ...validImportJob, hasCustomImportJs: true }); + expect(importJsJob.hasCustomImportJs()).to.be.true; + importJsJob = createImportJob({ ...validImportJob, hasCustomImportJs: false }); + expect(importJsJob.hasCustomImportJs()).to.be.false; + importJsJob = createImportJob({ ...validImportJob }); + expect(importJsJob.hasCustomImportJs()).to.be.false; expect(() => createImportJob({ ...validImportJob, hasCustomImportJs: 'x' })).to.throw('Invalid hasCustomImportJs value: x'); }); }); From 70f03596801e6b2a2b16cbe4defa978e22f3651e Mon Sep 17 00:00:00 2001 From: Ben Helleman Date: Tue, 24 Sep 2024 14:20:18 -0400 Subject: [PATCH 3/3] fix: SITES-25634 [Importer] Expose hasCustomImportJs and hasCustomHeaders as two new fields on a import job. --- .../test/unit/service/import-job/index.test.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/spacecat-shared-data-access/test/unit/service/import-job/index.test.js b/packages/spacecat-shared-data-access/test/unit/service/import-job/index.test.js index e533eb9e..6ba32075 100644 --- a/packages/spacecat-shared-data-access/test/unit/service/import-job/index.test.js +++ b/packages/spacecat-shared-data-access/test/unit/service/import-job/index.test.js @@ -95,6 +95,14 @@ describe('Import Job Tests', () => { // expect that 2 results were in the db, and that the results were not modified expect(result).to.be.an('array').and.to.have.lengthOf(2); + + // verify that the mocked properties are present in the result + function hasPropertiesOf(mockJob, importJob) { + return Object.keys(mockJob).every((key) => mockJob[key] === importJob.state[key]); + } + + expect(hasPropertiesOf(mockImportJobs[0], result[0])).to.be.true; + expect(hasPropertiesOf(mockImportJobs[1], result[1])).to.be.true; }, ); });