From f14ec8bd2d3545c6a6284cfa943f02b3a7a6806d Mon Sep 17 00:00:00 2001 From: Montassar Ghanmy Date: Thu, 5 Sep 2024 14:30:27 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fix=20infinite=20scroll=20for=20?= =?UTF-8?q?shared=20with=20me=20(#641)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../search/adapters/mongosearch/index.ts | 7 +- .../documents/entities/drive-file.search.ts | 1 + .../src/services/documents/services/index.ts | 69 ++++--- .../node/src/services/documents/types.ts | 6 +- .../documents/web/controllers/documents.ts | 43 ++--- .../test/e2e/common/entities/mock_entities.ts | 13 +- .../documents-pagination-sorting.spec.ts | 173 +++++++++++++++++- .../docker-compose.dev.tests.opensearch.yml | 1 + .../features/drive/api-client/api-client.ts | 5 +- .../frontend/src/app/features/drive/types.ts | 5 +- 10 files changed, 253 insertions(+), 70 deletions(-) diff --git a/tdrive/backend/node/src/core/platform/services/search/adapters/mongosearch/index.ts b/tdrive/backend/node/src/core/platform/services/search/adapters/mongosearch/index.ts index ac3884c5..f48b2a63 100644 --- a/tdrive/backend/node/src/core/platform/services/search/adapters/mongosearch/index.ts +++ b/tdrive/backend/node/src/core/platform/services/search/adapters/mongosearch/index.ts @@ -211,7 +211,12 @@ export default class MongoSearch extends SearchAdapter implements SearchAdapterI logger.info(`Search query: ${JSON.stringify(query)}`); console.log(query); - let cursor = collection.find(query).sort(sort); + const sortMapped: any = sort + ? Object.fromEntries( + Object.entries(sort).map(([field, direction]) => [field, direction === "asc" ? 1 : -1]), + ) + : {}; + let cursor = collection.find(query).sort(sortMapped); if (project) { cursor = cursor.project(project); } diff --git a/tdrive/backend/node/src/services/documents/entities/drive-file.search.ts b/tdrive/backend/node/src/services/documents/entities/drive-file.search.ts index d1e6a449..02bda386 100644 --- a/tdrive/backend/node/src/services/documents/entities/drive-file.search.ts +++ b/tdrive/backend/node/src/services/documents/entities/drive-file.search.ts @@ -14,6 +14,7 @@ export default { access_entities: entity.access_info?.entities?.filter(e => e.level != "none").map(e => e.id), last_modified: entity.last_modified, mime_type: entity.last_version_cache?.file_metadata?.mime, + size: entity.last_version_cache?.file_metadata?.size, }), mongoMapping: { text: { diff --git a/tdrive/backend/node/src/services/documents/services/index.ts b/tdrive/backend/node/src/services/documents/services/index.ts index 25d1a8c4..28f32018 100644 --- a/tdrive/backend/node/src/services/documents/services/index.ts +++ b/tdrive/backend/node/src/services/documents/services/index.ts @@ -29,10 +29,8 @@ import { DriveFileAccessLevel, DriveItemDetails, DriveTdriveTab, - PaginateDocumentBody, RootType, SearchDocumentsOptions, - SortDocumentsBody, TrashType, } from "../types"; import { @@ -61,6 +59,7 @@ import archiver from "archiver"; import internal from "stream"; import config from "config"; import { randomUUID } from "crypto"; +import { SortType } from "src/core/platform/services/search/api"; export class DocumentsService { version: "1"; @@ -107,8 +106,6 @@ export class DocumentsService { browse = async ( id: string, options: SearchDocumentsOptions, - sort: SortDocumentsBody, - paginate: PaginateDocumentBody, context: DriveExecutionContext & { public_token?: string }, ): Promise => { if (isSharedWithMeFolder(id)) { @@ -116,7 +113,7 @@ export class DocumentsService { } else { return { nextPage: null, - ...(await this.get(id, context, false, sort, paginate)), + ...(await this.get(id, options, context, false)), }; } }; @@ -125,17 +122,23 @@ export class DocumentsService { options: SearchDocumentsOptions, context: DriveExecutionContext & { public_token?: string }, ): Promise => { - const result = []; - let fileList: ListResult; - do { - fileList = await this.search(options, context); - result.push(...fileList.getEntities()); - options.pagination = fileList.nextPage; - } while (fileList.nextPage?.page_token); + if (options.pagination) { + if (options.pagination.page_token == "1") { + delete options.pagination.page_token; + } + } + + if (options.sort) { + options.sort = this.getSortFieldMapping(options.sort); + } + + const fileList: ListResult = await this.search(options, context); + const result = fileList.getEntities(); + return { access: "read", children: result, - nextPage: null, + nextPage: fileList.nextPage, path: [] as Array, }; }; @@ -157,10 +160,9 @@ export class DocumentsService { */ get = async ( id: string, + options: SearchDocumentsOptions, context: DriveExecutionContext & { public_token?: string }, all?: boolean, - sort?: SortDocumentsBody, - paginate?: PaginateDocumentBody, ): Promise => { if (!context) { this.logger.error("invalid context"); @@ -213,24 +215,21 @@ export class DocumentsService { ) ).getEntities(); - const sortFieldMapping = { - name: "name", - date: "last_modified", - size: "size", - }; - const sortField = {}; - sortField[sortFieldMapping[sort?.by] || "last_modified"] = sort?.order || "desc"; - + let sortField = {}; + if (options?.sort) { + sortField = this.getSortFieldMapping(options.sort); + } const dbType = await globalResolver.database.getConnector().getType(); // Initialize pagination let pagination; - if (paginate) { - const { page, limit } = paginate; - const pageNumber = dbType === "mongodb" ? page : page / limit + 1; + if (options?.pagination) { + const { page_token, limitStr } = options.pagination; + const pageNumber = + dbType === "mongodb" ? parseInt(page_token) : parseInt(page_token) / parseInt(limitStr) + 1; - pagination = new Pagination(`${pageNumber}`, `${limit}`, false); + pagination = new Pagination(`${pageNumber}`, `${limitStr}`, false); } let children = isDirectory @@ -1031,7 +1030,7 @@ export class DocumentsService { context: DriveExecutionContext, ): Promise => { for (const id of ids) { - const item = await this.get(id, context); + const item = await this.get(id, null, context); if (!item) { throw new CrudException("Drive item not found", 404); } @@ -1085,7 +1084,7 @@ export class DocumentsService { size: number; }; }> => { - const item = await this.get(id, context); + const item = await this.get(id, null, context); if (item.item.is_directory) { return { archive: await this.createZip([id], context) }; @@ -1317,4 +1316,16 @@ export class DocumentsService { throw new CrudException(`Not enough space: ${size}, ${leftQuota}.`, 403); } }; + + getSortFieldMapping = (sort: SortType) => { + const sortFieldMapping = { + name: "name", + date: "last_modified", + size: "size", + }; + + const sortField = {}; + sortField[sortFieldMapping[sort?.by] || "last_modified"] = sort?.order || "desc"; + return sortField; + }; } diff --git a/tdrive/backend/node/src/services/documents/types.ts b/tdrive/backend/node/src/services/documents/types.ts index 99c6abec..fc38bbfa 100644 --- a/tdrive/backend/node/src/services/documents/types.ts +++ b/tdrive/backend/node/src/services/documents/types.ts @@ -61,8 +61,8 @@ export type SearchDocumentsOptions = { export type BrowseDocumentsOptions = { filter?: SearchDocumentsBody; - sort?: SortDocumentsBody; - paginate?: PaginateDocumentBody; + sort?: SortType; + paginate?: Paginable; }; export type SearchDocumentsBody = { @@ -85,7 +85,7 @@ export type SortDocumentsBody = { }; export type PaginateDocumentBody = { - page: number; + page?: string; limit: number; }; diff --git a/tdrive/backend/node/src/services/documents/web/controllers/documents.ts b/tdrive/backend/node/src/services/documents/web/controllers/documents.ts index 973c0765..1697d80c 100644 --- a/tdrive/backend/node/src/services/documents/web/controllers/documents.ts +++ b/tdrive/backend/node/src/services/documents/web/controllers/documents.ts @@ -16,12 +16,10 @@ import { DriveItemDetails, DriveTdriveTab, ItemRequestParams, - PaginateDocumentBody, ItemRequestByEditingSessionKeyParams, RequestParams, SearchDocumentsBody, SearchDocumentsOptions, - SortDocumentsBody, } from "../../types"; import { DriveFileDTO } from "../dto/drive-file-dto"; import { DriveFileDTOBuilder } from "../../services/drive-file-dto-builder"; @@ -146,7 +144,7 @@ export class DocumentsController { ): Promise => { const context = getDriveExecutionContext(request); - return await globalResolver.services.documents.documents.get(null, context); + return await globalResolver.services.documents.documents.get(null, null, context); }; /** @@ -165,7 +163,7 @@ export class DocumentsController { const { id } = request.params; return { - ...(await globalResolver.services.documents.documents.get(id, context)), + ...(await globalResolver.services.documents.documents.get(id, null, context)), }; }; @@ -212,19 +210,12 @@ export class DocumentsController { view: DriveFileDTOBuilder.VIEW_SHARED_WITH_ME, onlyDirectlyShared: true, onlyUploadedNotByMe: true, + sort: request.body.sort, + pagination: request.body.paginate, }; - const sortOptions: SortDocumentsBody = request.body.sort; - const paginateOptions: PaginateDocumentBody = request.body.paginate; - return { - ...(await globalResolver.services.documents.documents.browse( - id, - options, - sortOptions, - paginateOptions, - context, - )), + ...(await globalResolver.services.documents.documents.browse(id, options, context)), }; }; @@ -475,12 +466,17 @@ export class DocumentsController { ); if (ids[0] === "root") { - const items = await globalResolver.services.documents.documents.get(ids[0], context); + const items = await globalResolver.services.documents.documents.get(ids[0], null, context); ids = items.children.map(item => item.id); } if (isDirectory === true) { - const items = await globalResolver.services.documents.documents.get(ids[0], context, true); + const items = await globalResolver.services.documents.documents.get( + ids[0], + null, + context, + true, + ); ids = items.children.map(item => item.id); } @@ -594,11 +590,16 @@ export class DocumentsController { type: string; }; }> { - const document = await globalResolver.services.documents.documents.get(req.body.document_id, { - public_token: req.body.token + (req.body.token_password ? "+" + req.body.token_password : ""), - user: null, - company: { id: req.body.company_id }, - }); + const document = await globalResolver.services.documents.documents.get( + req.body.document_id, + null, + { + public_token: + req.body.token + (req.body.token_password ? "+" + req.body.token_password : ""), + user: null, + company: { id: req.body.company_id }, + }, + ); if (!document || !document.access || document.access === "none") throw new CrudException("You don't have access to this document", 401); diff --git a/tdrive/backend/node/test/e2e/common/entities/mock_entities.ts b/tdrive/backend/node/test/e2e/common/entities/mock_entities.ts index 6106028c..9bcf5365 100644 --- a/tdrive/backend/node/test/e2e/common/entities/mock_entities.ts +++ b/tdrive/backend/node/test/e2e/common/entities/mock_entities.ts @@ -37,10 +37,15 @@ export class DriveFileMockClass { } export class DriveItemDetailsMockClass { - path: string[]; - item: DriveFileMockClass; - children: DriveFileMockClass[]; - versions: Record[]; + path: string[]; + item: DriveFileMockClass; + children: DriveFileMockClass[]; + versions: Record[]; + nextPage?: { + page_token: string; + limitStr: string; + reversed: boolean; + }; } export class SearchResultMockClass { diff --git a/tdrive/backend/node/test/e2e/documents/documents-pagination-sorting.spec.ts b/tdrive/backend/node/test/e2e/documents/documents-pagination-sorting.spec.ts index 42c05a48..0ce3eddb 100644 --- a/tdrive/backend/node/test/e2e/documents/documents-pagination-sorting.spec.ts +++ b/tdrive/backend/node/test/e2e/documents/documents-pagination-sorting.spec.ts @@ -36,27 +36,27 @@ describe("The Documents Browser Window and API", () => { const myDriveId = "user_" + currentUser.user.id; await currentUser.uploadAllFilesOneByOne(myDriveId); - let page = 1; - const limit = 2; + let page_token = "1"; + const limitStr = "2"; let docs = await currentUser.browseDocuments(myDriveId, { - paginate: { page, limit }, + paginate: { page_token, limitStr }, }); expect(docs).toBeDefined(); - expect(docs.children).toHaveLength(limit); + expect(docs.children).toHaveLength(parseInt(limitStr)); - page = 2; + page_token = "2"; docs = await currentUser.browseDocuments(myDriveId, { - paginate: { page, limit }, + paginate: { page_token, limitStr }, }); expect(docs).toBeDefined(); - expect(docs.children).toHaveLength(limit); + expect(docs.children).toHaveLength(parseInt(limitStr)); - page = 3; + page_token = "3"; docs = await currentUser.browseDocuments(myDriveId, { - paginate: { page, limit }, + paginate: { page_token, limitStr }, }); expect(docs).toBeDefined(); - expect(docs.children.length).toBeLessThanOrEqual(limit); + expect(docs.children.length).toBeLessThanOrEqual(parseInt(limitStr)); }); it("Should sort documents by name in ascending order", async () => { @@ -152,5 +152,158 @@ describe("The Documents Browser Window and API", () => { const isSorted = docs.children.every((item, i, arr) => !i || arr[i - 1].size >= item.size); expect(isSorted).toBe(true); }); + + it("Should paginate shared with me ", async () => { + const sharedWIthMeFolder = "shared_with_me"; + const oneUser = await UserApi.getInstance(platform, true, { companyRole: "admin" }); + const anotherUser = await UserApi.getInstance(platform, true, { companyRole: "admin" }); + let files = await oneUser.uploadAllFilesOneByOne(); + for (const file of files) { + await oneUser.shareWithPermissions(file, anotherUser.user.id, "read"); + } + + // wait for files to be indexed + await new Promise(r => setTimeout(r, 5000)); + + let page_token: any = "1"; + const limitStr = "2"; + let docs = await anotherUser.browseDocuments(sharedWIthMeFolder, { + paginate: { page_token, limitStr }, + }); + expect(docs).toBeDefined(); + expect(docs.children).toHaveLength(parseInt(limitStr)); + + page_token = docs.nextPage?.page_token || "2"; + docs = await anotherUser.browseDocuments(sharedWIthMeFolder, { + paginate: { page_token, limitStr }, + }); + expect(docs).toBeDefined(); + expect(docs.children).toHaveLength(parseInt(limitStr)); + + page_token = docs.nextPage?.page_token || "3"; + docs = await anotherUser.browseDocuments(sharedWIthMeFolder, { + paginate: { page_token, limitStr }, + }); + expect(docs).toBeDefined(); + expect(docs.children.length).toBeLessThanOrEqual(parseInt(limitStr)); + }); + + it("Should sort shared with me by name in ascending order", async () => { + const sharedWIthMeFolder = "shared_with_me"; + const oneUser = await UserApi.getInstance(platform, true, { companyRole: "admin" }); + const anotherUser = await UserApi.getInstance(platform, true, { companyRole: "admin" }); + let files = await oneUser.uploadAllFilesOneByOne(); + for (const file of files) { + await oneUser.shareWithPermissions(file, anotherUser.user.id, "read"); + } + const sortBy = "name"; + const sortOrder = "asc"; + const docs = await anotherUser.browseDocuments(sharedWIthMeFolder, { + sort: { by: sortBy, order: sortOrder }, + }); + expect(docs).toBeDefined(); + + const isSorted = docs.children.every((item, i, arr) => !i || arr[i - 1].name <= item.name); + expect(isSorted).toBe(true); + }); + + it("Should sort shared with me by name in descending order", async () => { + const sharedWIthMeFolder = "shared_with_me"; + const oneUser = await UserApi.getInstance(platform, true, { companyRole: "admin" }); + const anotherUser = await UserApi.getInstance(platform, true, { companyRole: "admin" }); + let files = await oneUser.uploadAllFilesOneByOne(); + for (const file of files) { + await oneUser.shareWithPermissions(file, anotherUser.user.id, "read"); + } + const sortBy = "name"; + const sortOrder = "desc"; + const docs = await anotherUser.browseDocuments(sharedWIthMeFolder, { + sort: { by: sortBy, order: sortOrder }, + }); + expect(docs).toBeDefined(); + + const isSorted = docs.children.every((item, i, arr) => !i || arr[i - 1].name >= item.name); + expect(isSorted).toBe(true); + }); + + it("Should sort shared with me by size in ascending order", async () => { + const sharedWIthMeFolder = "shared_with_me"; + const oneUser = await UserApi.getInstance(platform, true, { companyRole: "admin" }); + const anotherUser = await UserApi.getInstance(platform, true, { companyRole: "admin" }); + let files = await oneUser.uploadAllFilesOneByOne(); + for (const file of files) { + await oneUser.shareWithPermissions(file, anotherUser.user.id, "read"); + } + const sortBy = "size"; + const sortOrder = "asc"; + const docs = await anotherUser.browseDocuments(sharedWIthMeFolder, { + sort: { by: sortBy, order: sortOrder }, + }); + expect(docs).toBeDefined(); + + const isSorted = docs.children.every((item, i, arr) => !i || arr[i - 1].size <= item.size); + expect(isSorted).toBe(true); + }); + + it("Should sort shared with me by size in descending order", async () => { + const sharedWIthMeFolder = "shared_with_me"; + const oneUser = await UserApi.getInstance(platform, true, { companyRole: "admin" }); + const anotherUser = await UserApi.getInstance(platform, true, { companyRole: "admin" }); + let files = await oneUser.uploadAllFilesOneByOne(); + for (const file of files) { + await oneUser.shareWithPermissions(file, anotherUser.user.id, "read"); + } + const sortBy = "size"; + const sortOrder = "desc"; + const docs = await anotherUser.browseDocuments(sharedWIthMeFolder, { + sort: { by: sortBy, order: sortOrder }, + }); + expect(docs).toBeDefined(); + + const isSorted = docs.children.every((item, i, arr) => !i || arr[i - 1].size >= item.size); + expect(isSorted).toBe(true); + }); + + it("Should sort shared with me by date in ascending order", async () => { + const sharedWIthMeFolder = "shared_with_me"; + const oneUser = await UserApi.getInstance(platform, true, { companyRole: "admin" }); + const anotherUser = await UserApi.getInstance(platform, true, { companyRole: "admin" }); + let files = await oneUser.uploadAllFilesOneByOne(); + for (const file of files) { + await oneUser.shareWithPermissions(file, anotherUser.user.id, "read"); + } + const sortBy = "date"; + const sortOrder = "asc"; + const docs = await anotherUser.browseDocuments(sharedWIthMeFolder, { + sort: { by: sortBy, order: sortOrder }, + }); + expect(docs).toBeDefined(); + + const isSorted = docs.children.every( + (item, i, arr) => !i || new Date(arr[i - 1].added) <= new Date(item.added), + ); + expect(isSorted).toBe(true); + }); + + it("Should sort shared with me by date in descending order", async () => { + const sharedWIthMeFolder = "shared_with_me"; + const oneUser = await UserApi.getInstance(platform, true, { companyRole: "admin" }); + const anotherUser = await UserApi.getInstance(platform, true, { companyRole: "admin" }); + let files = await oneUser.uploadAllFilesOneByOne(); + for (const file of files) { + await oneUser.shareWithPermissions(file, anotherUser.user.id, "read"); + } + const sortBy = "date"; + const sortOrder = "desc"; + const docs = await anotherUser.browseDocuments(sharedWIthMeFolder, { + sort: { by: sortBy, order: sortOrder }, + }); + expect(docs).toBeDefined(); + + const isSorted = docs.children.every( + (item, i, arr) => !i || new Date(arr[i - 1].added) >= new Date(item.added), + ); + expect(isSorted).toBe; + }); }); }); diff --git a/tdrive/docker-compose.dev.tests.opensearch.yml b/tdrive/docker-compose.dev.tests.opensearch.yml index 442d64a2..3cac70f1 100644 --- a/tdrive/docker-compose.dev.tests.opensearch.yml +++ b/tdrive/docker-compose.dev.tests.opensearch.yml @@ -41,6 +41,7 @@ services: - "5432:5432" node: + # Use the build context in the current directory build: context: . dockerfile: docker/tdrive-node/Dockerfile diff --git a/tdrive/frontend/src/app/features/drive/api-client/api-client.ts b/tdrive/frontend/src/app/features/drive/api-client/api-client.ts index 3d2981bf..b82244c2 100644 --- a/tdrive/frontend/src/app/features/drive/api-client/api-client.ts +++ b/tdrive/frontend/src/app/features/drive/api-client/api-client.ts @@ -76,7 +76,10 @@ export class DriveApiClient { { filter, sort, - paginate + paginate: { + page_token: paginate.page.toString(), + limitStr: paginate.limit.toString(), + } }, ); } diff --git a/tdrive/frontend/src/app/features/drive/types.ts b/tdrive/frontend/src/app/features/drive/types.ts index c50743d0..fd605799 100644 --- a/tdrive/frontend/src/app/features/drive/types.ts +++ b/tdrive/frontend/src/app/features/drive/types.ts @@ -1,7 +1,10 @@ export type BrowseQuery = { filter: BrowseFilter; sort: BrowseSort; - paginate: BrowsePaginate; + paginate: { + page_token: string; + limitStr: string; + }; } export type BrowseFilter = {