Skip to content

Commit

Permalink
BC-5832- Add deleting creator ID reference from the files entities (t…
Browse files Browse the repository at this point in the history 
…he legacy files, rewritten from FeathersJS) in the main user deletion use case (#4662)
  • Loading branch information
@sszafGCA
sszafGCA authored Jan 9, 2024
1 parent f335c98 commit e6193d3
Show file tree
Hide file tree
Showing 8 changed files with 200 additions and 65 deletions.
8 changes: 5 additions & 3 deletions apps/server/src/modules/deletion/uc/deletion-request.uc.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ describe(DeletionRequestUc.name, () => {
courseGroupService.deleteUserDataFromCourseGroup.mockResolvedValueOnce(2);
courseService.deleteUserDataFromCourse.mockResolvedValueOnce(2);
filesService.markFilesOwnedByUserForDeletion.mockResolvedValueOnce(2);
filesService.removeUserPermissionsToAnyFiles.mockResolvedValueOnce(2);
filesService.removeUserPermissionsOrCreatorReferenceToAnyFiles.mockResolvedValueOnce(2);
lessonService.deleteUserDataFromLessons.mockResolvedValueOnce(2);
pseudonymService.deleteByUserId.mockResolvedValueOnce(2);
teamService.deleteUserDataFromTeams.mockResolvedValueOnce(2);
Expand Down Expand Up @@ -336,7 +336,9 @@ describe(DeletionRequestUc.name, () => {

await uc.executeDeletionRequests();

expect(filesService.removeUserPermissionsToAnyFiles).toHaveBeenCalledWith(deletionRequestToExecute.targetRefId);
expect(filesService.removeUserPermissionsOrCreatorReferenceToAnyFiles).toHaveBeenCalledWith(
deletionRequestToExecute.targetRefId
);
});

it('should call filesStorageClientAdapterService.removeCreatorIdFromFileRecords to remove cratorId to any files in fileRecords module', async () => {
Expand Down Expand Up @@ -482,7 +484,7 @@ describe(DeletionRequestUc.name, () => {
courseGroupService.deleteUserDataFromCourseGroup.mockResolvedValueOnce(2);
courseService.deleteUserDataFromCourse.mockResolvedValueOnce(2);
filesService.markFilesOwnedByUserForDeletion.mockResolvedValueOnce(2);
filesService.removeUserPermissionsToAnyFiles.mockResolvedValueOnce(2);
filesService.removeUserPermissionsOrCreatorReferenceToAnyFiles.mockResolvedValueOnce(2);
lessonService.deleteUserDataFromLessons.mockResolvedValueOnce(2);
pseudonymService.deleteByUserId.mockResolvedValueOnce(2);
teamService.deleteUserDataFromTeams.mockResolvedValueOnce(2);
Expand Down
4 changes: 2 additions & 2 deletions apps/server/src/modules/deletion/uc/deletion-request.uc.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,14 +204,14 @@ export class DeletionRequestUc {
this.logger.debug({ action: 'removeUsersFilesAndPermissions', deletionRequest });

const filesDeleted: number = await this.filesService.markFilesOwnedByUserForDeletion(deletionRequest.targetRefId);
const filePermissionsUpdated: number = await this.filesService.removeUserPermissionsToAnyFiles(
const filesUpdated: number = await this.filesService.removeUserPermissionsOrCreatorReferenceToAnyFiles(
deletionRequest.targetRefId
);
await this.logDeletion(
deletionRequest,
DomainModel.FILE,
DeletionOperationModel.UPDATE,
filesDeleted + filePermissionsUpdated,
filesDeleted + filesUpdated,
0
);
}
Expand Down
44 changes: 44 additions & 0 deletions apps/server/src/modules/files/entity/file.entity.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,50 @@ describe(FileEntity.name, () => {
});
});

describe('removCreatorId', () => {
describe('when called on a file that contains matching creatorId', () => {
const setup = () => {
const file = fileEntityFactory.build({
ownerId: mainUserId,
creatorId: mainUserId,
});

const expectedFile = copyFile(file);
expectedFile._creatorId = undefined;

return { file, expectedFile };
};
it('should properly remove this creatorId', () => {
const { file, expectedFile } = setup();

file.removeCreatorId(mainUserId);

expect(file).toEqual(expectedFile);
});
});

describe("when called on a file that doesn't have any permission with given refId", () => {
const setup = () => {
const file = fileEntityFactory.build({
ownerId: mainUserId,
creatorId: mainUserId,
});

const originalFile = copyFile(file);

const randomUserId = new ObjectId().toHexString();
return { file, originalFile, randomUserId };
};
it('should not modify the file in any way (including the other present permissions)', () => {
const { file, originalFile, randomUserId } = setup();

file.removeCreatorId(randomUserId);

expect(file).toEqual(originalFile);
});
});
});

describe('markForDeletion', () => {
describe('when called on some typical file', () => {
it('should properly mark the file for deletion', () => {
Expand Down
16 changes: 11 additions & 5 deletions apps/server/src/modules/files/entity/file.entity.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export interface FileEntityProps {
parentId?: EntityId;
ownerId: EntityId;
refOwnerModel: FileOwnerModel;
creatorId: EntityId;
creatorId?: EntityId;
permissions: FilePermissionEntity[];
lockId?: EntityId;
versionKey?: number;
Expand Down Expand Up @@ -95,12 +95,12 @@ export class FileEntity extends BaseEntityWithTimestamps {
@Enum({ nullable: false })
refOwnerModel: FileOwnerModel;

@Property({ fieldName: 'creator' })
@Property({ fieldName: 'creator', nullable: true })
@Index()
_creatorId: ObjectId;
_creatorId?: ObjectId;

get creatorId(): EntityId {
return this._creatorId.toHexString();
get creatorId(): EntityId | undefined {
return this._creatorId?.toHexString();
}

@Embedded(() => FilePermissionEntity, { array: true, nullable: false })
Expand Down Expand Up @@ -141,6 +141,12 @@ export class FileEntity extends BaseEntityWithTimestamps {
return this.deleted && this.deletedAt !== undefined && !Number.isNaN(this.deletedAt.getTime());
}

public removeCreatorId(creatorId: EntityId): void {
if (creatorId === this._creatorId?.toHexString()) {
this._creatorId = undefined;
}
}

constructor(props: FileEntityProps) {
super();

Expand Down
53 changes: 46 additions & 7 deletions apps/server/src/modules/files/repo/files.repo.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,13 +228,19 @@ describe(FilesRepo.name, () => {
});
});

describe('findByPermissionRefId', () => {
describe('when searching for a files to which the user with given userId has access', () => {
describe('findByPermissionRefIdOrCreatorId', () => {
describe('when searching for a files to which the user with given userId has access or is creator', () => {
const setup = async () => {
const mainUserId = new ObjectId().toHexString();
const otherUserId = new ObjectId().toHexString();

// Test files created, owned and accessible only by the other user.
const otherUserFileWithMainUserCreator = fileEntityFactory.build({
ownerId: otherUserId,
creatorId: mainUserId,
permissions: [filePermissionEntityFactory.build({ refId: otherUserId })],
});

const otherUserFile = fileEntityFactory.build({
ownerId: otherUserId,
creatorId: otherUserId,
Expand Down Expand Up @@ -268,9 +274,35 @@ describe(FilesRepo.name, () => {
permissions: [filePermissionEntityFactory.build({ refId: mainUserId })],
});

await em.persistAndFlush([otherUserFile, mainUserSharedFile, otherUserSharedFile, mainUserFile]);
await em.persistAndFlush([
otherUserFileWithMainUserCreator,
mainUserSharedFile,
otherUserSharedFile,
mainUserFile,
otherUserFile,
]);
em.clear();

const expectedOtherUserFileWithMainUserCreatorProps = {
id: otherUserFileWithMainUserCreator.id,
createdAt: otherUserFileWithMainUserCreator.createdAt,
updatedAt: otherUserFileWithMainUserCreator.updatedAt,
deleted: false,
isDirectory: false,
name: otherUserFileWithMainUserCreator.name,
size: otherUserFileWithMainUserCreator.size,
type: otherUserFileWithMainUserCreator.type,
storageFileName: otherUserFileWithMainUserCreator.storageFileName,
bucket: otherUserFileWithMainUserCreator.bucket,
thumbnail: otherUserFileWithMainUserCreator.thumbnail,
thumbnailRequestToken: otherUserFileWithMainUserCreator.thumbnailRequestToken,
securityCheck: otherUserFileWithMainUserCreator.securityCheck,
shareTokens: [],
refOwnerModel: otherUserFileWithMainUserCreator.refOwnerModel,
permissions: otherUserFileWithMainUserCreator.permissions,
versionKey: 0,
};

const expectedMainUserSharedFileProps = {
id: mainUserSharedFile.id,
createdAt: mainUserSharedFile.createdAt,
Expand Down Expand Up @@ -336,6 +368,8 @@ describe(FilesRepo.name, () => {
mainUserSharedFile,
otherUserSharedFile,
mainUserFile,
otherUserFileWithMainUserCreator,
expectedOtherUserFileWithMainUserCreatorProps,
expectedMainUserSharedFileProps,
expectedOtherUserSharedFileProps,
expectedMainUserFileProps,
Expand All @@ -349,18 +383,21 @@ describe(FilesRepo.name, () => {
mainUserSharedFile,
otherUserSharedFile,
mainUserFile,
otherUserFileWithMainUserCreator,
expectedMainUserSharedFileProps,
expectedOtherUserSharedFileProps,
expectedMainUserFileProps,
expectedOtherUserFileWithMainUserCreatorProps,
} = await setup();

const results = await repo.findByPermissionRefId(mainUserId);
const results = await repo.findByPermissionRefIdOrCreatorId(mainUserId);

expect(results).toHaveLength(3);
expect(results).toHaveLength(4);

// Verify explicit fields.
expect(results).toEqual(
expect.arrayContaining([
expect.objectContaining(expectedOtherUserFileWithMainUserCreatorProps),
expect.objectContaining(expectedMainUserSharedFileProps),
expect.objectContaining(expectedOtherUserSharedFileProps),
expect.objectContaining(expectedMainUserFileProps),
Expand All @@ -370,6 +407,7 @@ describe(FilesRepo.name, () => {
// Verify storage provider id.
expect(results.map((result) => result.storageProvider?.id)).toEqual(
expect.arrayContaining([
otherUserFileWithMainUserCreator.storageProvider?.id,
mainUserSharedFile.storageProvider?.id,
otherUserSharedFile.storageProvider?.id,
mainUserFile.storageProvider?.id,
Expand All @@ -384,6 +422,7 @@ describe(FilesRepo.name, () => {
// Verify implicit creatorId field.
expect(results.map((result) => result.creatorId)).toEqual(
expect.arrayContaining([
otherUserFileWithMainUserCreator.creatorId,
mainUserSharedFile.creatorId,
otherUserSharedFile.creatorId,
mainUserFile.creatorId,
Expand All @@ -397,7 +436,7 @@ describe(FilesRepo.name, () => {
await em.persistAndFlush([fileEntityFactory.build(), fileEntityFactory.build(), fileEntityFactory.build()]);
em.clear();

const results = await repo.findByPermissionRefId(new ObjectId().toHexString());
const results = await repo.findByPermissionRefIdOrCreatorId(new ObjectId().toHexString());

expect(results).toHaveLength(0);
});
Expand All @@ -407,7 +446,7 @@ describe(FilesRepo.name, () => {
it('should return an empty array', async () => {
const testPermissionRefId = new ObjectId().toHexString();

const results = await repo.findByPermissionRefId(testPermissionRefId);
const results = await repo.findByPermissionRefIdOrCreatorId(testPermissionRefId);

expect(results).toHaveLength(0);
});
Expand Down
23 changes: 18 additions & 5 deletions apps/server/src/modules/files/repo/files.repo.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,28 @@ export class FilesRepo extends BaseRepo<FileEntity> {
return files as FileEntity[];
}

public async findByPermissionRefId(permissionRefId: EntityId): Promise<FileEntity[]> {
public async findByPermissionRefIdOrCreatorId(userId: EntityId): Promise<FileEntity[]> {
const refId = new ObjectId(userId);

const pipeline = [
{
$match: {
permissions: {
$elemMatch: {
refId: new ObjectId(permissionRefId),
$and: [
{
$or: [
{
permissions: {
$elemMatch: {
refId,
},
},
},
{ creator: refId },
],
},
},
{ deleted: false },
{ deletedAt: undefined },
],
},
},
];
Expand Down
Loading

0 comments on commit e6193d3

Please sign in to comment.