From dbdc7d930885145cfade4ffc050a97df0b488d46 Mon Sep 17 00:00:00 2001 From: Dan Ribbens Date: Fri, 20 Sep 2024 09:08:58 -0400 Subject: [PATCH] chore: use updatedAt instead of editedAt for locked documents (#8324) - Removes locked documents `editedAt` as it was redundant with the `updatedAt` timestamp - Adjust stale lock tests to configure the duration down to 1 second and await it to not lose any test coverage - DB performance changes: 1. Switch to payload.db.find instead of payload.find for checkDocumentLockStatus to avoid populating the user and other payload find overhead 2. Add maxDepth: 1 to user relationship 3. Add index to global slug --- .../src/elements/DocumentLocked/index.tsx | 6 +-- .../next/src/views/Edit/Default/index.tsx | 2 +- .../lockedDocumentsCollection.ts | 6 +-- .../src/utilities/checkDocumentLockStatus.ts | 11 +++-- .../ui/src/providers/DocumentInfo/index.tsx | 1 - packages/ui/src/utilities/buildFormState.ts | 5 +-- test/locked-documents/e2e.spec.ts | 5 --- test/locked-documents/int.spec.ts | 43 +++++++++++-------- test/locked-documents/payload-types.ts | 1 - 9 files changed, 38 insertions(+), 42 deletions(-) diff --git a/packages/next/src/elements/DocumentLocked/index.tsx b/packages/next/src/elements/DocumentLocked/index.tsx index d839fda573..a02eede8af 100644 --- a/packages/next/src/elements/DocumentLocked/index.tsx +++ b/packages/next/src/elements/DocumentLocked/index.tsx @@ -25,13 +25,13 @@ const formatDate = (date) => { } export const DocumentLocked: React.FC<{ - editedAt?: null | number handleGoBack: () => void isActive: boolean onReadOnly: () => void onTakeOver: () => void + updatedAt?: null | number user?: ClientUser -}> = ({ editedAt, handleGoBack, isActive, onReadOnly, onTakeOver, user }) => { +}> = ({ handleGoBack, isActive, onReadOnly, onTakeOver, updatedAt, user }) => { const { closeModal, openModal } = useModal() const { t } = useTranslation() @@ -52,7 +52,7 @@ export const DocumentLocked: React.FC<{ {user?.email ?? user?.id} {t('general:currentlyEditing')}

- {t('general:editedSince')} {formatDate(editedAt)} + {t('general:editedSince')} {formatDate(updatedAt)}

diff --git a/packages/next/src/views/Edit/Default/index.tsx b/packages/next/src/views/Edit/Default/index.tsx index 35416bc834..3763e2c11f 100644 --- a/packages/next/src/views/Edit/Default/index.tsx +++ b/packages/next/src/views/Edit/Default/index.tsx @@ -421,7 +421,6 @@ export const DefaultEditView: React.FC = () => { {BeforeDocument} {isLockingEnabled && shouldShowDocumentLockedModal && !isReadOnlyForIncomingUser && ( { @@ -429,6 +428,7 @@ export const DefaultEditView: React.FC = () => { setShowTakeOverModal(false) }} onTakeOver={handleTakeOver} + updatedAt={lastUpdateTime} user={currentEditor} /> )} diff --git a/packages/payload/src/lockedDocuments/lockedDocumentsCollection.ts b/packages/payload/src/lockedDocuments/lockedDocumentsCollection.ts index 9cc0f1cd73..e2508ac3df 100644 --- a/packages/payload/src/lockedDocuments/lockedDocumentsCollection.ts +++ b/packages/payload/src/lockedDocuments/lockedDocumentsCollection.ts @@ -14,17 +14,15 @@ export const getLockedDocumentsCollection = (config: Config): CollectionConfig = maxDepth: 0, relationTo: [...config.collections.map((collectionConfig) => collectionConfig.slug)], }, - { - name: 'editedAt', - type: 'date', - }, { name: 'globalSlug', type: 'text', + index: true, }, { name: 'user', type: 'relationship', + maxDepth: 1, relationTo: config.collections .filter((collectionConfig) => collectionConfig.auth) .map((collectionConfig) => collectionConfig.slug), diff --git a/packages/payload/src/utilities/checkDocumentLockStatus.ts b/packages/payload/src/utilities/checkDocumentLockStatus.ts index 4924f60d86..ff0dece07b 100644 --- a/packages/payload/src/utilities/checkDocumentLockStatus.ts +++ b/packages/payload/src/utilities/checkDocumentLockStatus.ts @@ -55,9 +55,8 @@ export const checkDocumentLockStatus = async ({ const finalLockErrorMessage = lockErrorMessage || defaultLockErrorMessage - const lockedDocumentResult: PaginatedDocs = await payload.find({ + const lockedDocumentResult: PaginatedDocs = await payload.db.find({ collection: 'payload-locked-documents', - depth: 1, limit: 1, pagination: false, req, @@ -68,8 +67,8 @@ export const checkDocumentLockStatus = async ({ // If there's a locked document, check lock conditions const lockedDoc = lockedDocumentResult?.docs[0] if (lockedDoc) { - const lastEditedAt = new Date(lockedDoc?.editedAt) - const now = new Date() + const lastEditedAt = new Date(lockedDoc?.updatedAt).getTime() + const now = new Date().getTime() const lockDuration = typeof lockDocumentsProp === 'object' ? lockDocumentsProp.duration : lockDurationDefault @@ -79,8 +78,8 @@ export const checkDocumentLockStatus = async ({ // document is locked by another user and the lock hasn't expired if ( - lockedDoc.user?.value?.id !== currentUserId && - now.getTime() - lastEditedAt.getTime() <= lockDurationInMilliseconds + lockedDoc.user?.value !== currentUserId && + now - lastEditedAt <= lockDurationInMilliseconds ) { throw new Locked(finalLockErrorMessage) } diff --git a/packages/ui/src/providers/DocumentInfo/index.tsx b/packages/ui/src/providers/DocumentInfo/index.tsx index d4da463400..3c0ad5815b 100644 --- a/packages/ui/src/providers/DocumentInfo/index.tsx +++ b/packages/ui/src/providers/DocumentInfo/index.tsx @@ -193,7 +193,6 @@ const DocumentInfo: React.FC< // Send a patch request to update the _lastEdited info await requests.patch(`${serverURL}${api}/payload-locked-documents/${lockId}`, { body: JSON.stringify({ - editedAt: new Date(), user: { relationTo: user?.collection, value: user?.id }, }), headers: { diff --git a/packages/ui/src/utilities/buildFormState.ts b/packages/ui/src/utilities/buildFormState.ts index 7c0a74d42b..f20249b3e8 100644 --- a/packages/ui/src/utilities/buildFormState.ts +++ b/packages/ui/src/utilities/buildFormState.ts @@ -265,9 +265,7 @@ export const buildFormState = async ({ await req.payload.db.updateOne({ id: lockedDocument.docs[0].id, collection: 'payload-locked-documents', - data: { - editedAt: new Date().toISOString(), - }, + data: {}, req, }) } @@ -284,7 +282,6 @@ export const buildFormState = async ({ value: id, } : undefined, - editedAt: new Date().toISOString(), globalSlug: globalSlug ? globalSlug : undefined, user: { relationTo: [req.user.collection], diff --git a/test/locked-documents/e2e.spec.ts b/test/locked-documents/e2e.spec.ts index 68d7b2319c..d982c81c38 100644 --- a/test/locked-documents/e2e.spec.ts +++ b/test/locked-documents/e2e.spec.ts @@ -98,7 +98,6 @@ describe('locked documents', () => { relationTo: 'posts', value: postDoc.id, }, - editedAt: new Date().toISOString(), globalSlug: undefined, user: { relationTo: 'users', @@ -318,7 +317,6 @@ describe('locked documents', () => { relationTo: 'posts', value: postDoc.id, }, - editedAt: new Date().toISOString(), globalSlug: undefined, user: { relationTo: 'users', @@ -422,7 +420,6 @@ describe('locked documents', () => { relationTo: 'posts', value: postDoc.id, }, - editedAt: new Date().toISOString(), globalSlug: undefined, user: { relationTo: 'users', @@ -512,7 +509,6 @@ describe('locked documents', () => { relationTo: 'posts', value: postDoc.id, }, - editedAt: new Date().toISOString(), globalSlug: undefined, user: { relationTo: 'users', @@ -784,7 +780,6 @@ describe('locked documents', () => { collection: lockedDocumentCollection, data: { document: undefined, - editedAt: new Date().toISOString(), globalSlug: 'menu', user: { relationTo: 'users', diff --git a/test/locked-documents/int.spec.ts b/test/locked-documents/int.spec.ts index 370118d4e1..8d23a60175 100644 --- a/test/locked-documents/int.spec.ts +++ b/test/locked-documents/int.spec.ts @@ -1,5 +1,8 @@ +import type { Payload, SanitizedCollectionConfig } from 'payload' + import path from 'path' -import { Locked, NotFound, type Payload } from 'payload' +import { Locked, NotFound } from 'payload' +import { wait } from 'payload/shared' import { fileURLToPath } from 'url' import type { NextRESTClient } from '../helpers/NextRESTClient.js' @@ -26,10 +29,13 @@ describe('Locked documents', () => { let menu: Menu let user: any let user2: any + let postConfig: SanitizedCollectionConfig beforeAll(async () => { ;({ payload, restClient } = await initPayloadInt(dirname)) + postConfig = payload.config.collections.find(({ slug }) => slug === postsSlug) + const loginResult = await payload.login({ collection: 'users', data: { @@ -77,6 +83,10 @@ describe('Locked documents', () => { } }) + afterEach(() => { + postConfig.lockDocuments = { duration: 300 } + }) + it('should update unlocked document - collection', async () => { const updatedPost = await payload.update({ collection: postsSlug, @@ -129,16 +139,13 @@ describe('Locked documents', () => { }, }) - // Subtract 3.5 minutes (210 seconds) from the current time - const pastEditedAt = new Date() - pastEditedAt.setMinutes(pastEditedAt.getMinutes() - 3) - pastEditedAt.setSeconds(pastEditedAt.getSeconds() - 30) + // Set lock duration to 1 second for testing purposes + postConfig.lockDocuments = { duration: 1 } // Give locking ownership to another user const lockedDocInstance = await payload.create({ collection: lockedDocumentCollection, data: { - editedAt: pastEditedAt.toISOString(), user: { relationTo: 'users', value: user2.id, @@ -151,6 +158,8 @@ describe('Locked documents', () => { }, }) + await wait(1100) + const updateLockedDoc = await payload.update({ collection: postsSlug, data: { @@ -159,6 +168,7 @@ describe('Locked documents', () => { overrideLock: false, id: newPost2.id, }) + postConfig.lockDocuments = { duration: 300 } // Should allow update since editedAt date is past expiration duration. // Therefore the document is considered stale @@ -187,16 +197,13 @@ describe('Locked documents', () => { }) it('should allow update of stale locked document - global', async () => { - // Subtract 5.5 minutes (330 seconds) from the current time - const pastEditedAt = new Date() - pastEditedAt.setMinutes(pastEditedAt.getMinutes() - 5) - pastEditedAt.setSeconds(pastEditedAt.getSeconds() - 30) - + // Set lock duration to 1 second for testing purposes + const globalConfig = payload.config.globals.find(({ slug }) => slug === menuSlug) + globalConfig.lockDocuments = { duration: 1 } // Give locking ownership to another user const lockedGlobalInstance = await payload.create({ collection: lockedDocumentCollection, data: { - editedAt: pastEditedAt.toISOString(), // stale date user: { relationTo: 'users', value: user2.id, @@ -206,6 +213,8 @@ describe('Locked documents', () => { }, }) + await wait(1100) + const updateGlobalLockedDoc = await payload.updateGlobal({ data: { globalText: 'global text 2', @@ -213,6 +222,7 @@ describe('Locked documents', () => { overrideLock: false, slug: menuSlug, }) + globalConfig.lockDocuments = { duration: 300 } // Should allow update since editedAt date is past expiration duration. // Therefore the document is considered stale @@ -379,16 +389,13 @@ describe('Locked documents', () => { }, }) - // Subtract 3.5 minutes (210 seconds) from the current time - const pastEditedAt = new Date() - pastEditedAt.setMinutes(pastEditedAt.getMinutes() - 3) - pastEditedAt.setSeconds(pastEditedAt.getSeconds() - 30) + // Set lock duration to 1 second for testing purposes + postConfig.lockDocuments = { duration: 1 } // Give locking ownership to another user const lockedDocInstance = await payload.create({ collection: lockedDocumentCollection, data: { - editedAt: pastEditedAt.toISOString(), // stale date user: { relationTo: 'users', value: user2.id, @@ -401,6 +408,8 @@ describe('Locked documents', () => { }, }) + await wait(1100) + await payload.delete({ collection: postsSlug, id: newPost4.id, diff --git a/test/locked-documents/payload-types.ts b/test/locked-documents/payload-types.ts index 44597368e8..229f66aa6d 100644 --- a/test/locked-documents/payload-types.ts +++ b/test/locked-documents/payload-types.ts @@ -111,7 +111,6 @@ export interface PayloadLockedDocument { relationTo: 'users'; value: string | User; }; - editedAt?: string | null; updatedAt: string; createdAt: string; }