Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix optimistic effect deletedAt #7606

Merged
merged 7 commits into from
Oct 11, 2024
Merged

Conversation

charlesBochet
Copy link
Member

@charlesBochet charlesBochet commented Oct 11, 2024

In this PR, I'm fixing part of the impact of soft deletion on optimistic rendering.

Backend Vision

  1. Backend endpoints will not return soft deleted records (having deletedAt set) by default. To get the softDeleted records, we will pass a { withSoftDelete: true } additional param in the query.
  2. Record relations will NEVER contain softDeleted relations

Backend current state

Right now, we have the following behavior:

  • if the query filters do not mention deletedAt, we don't return softDeletedRecords
  • if the query filters mention deletedAt, we take it into consideration. Meaning that if we want to have the softDeleted records in any way we need to do { or: [ deletedAt: NULL, deletedAt: NOT_NULL] }

Optimistic rendering strategy

  1. useDestroyOne/Many is triggering destroyOptimisticEffects (previously deleteOptimisticEffects)
  2. UseDeleteOne/Many and useRestoreOne/Many are actually triggering updateOptimisticEffects (as they only update deletedAt field) AND we need updateOptimisticEffects to take into account deletedAt (future withSoftDelete: true) filter.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

Minor formatting improvement in the OpenAPI parameters utility file, specifically in the computeFilterParameters function's filter description.

  • Removed a newline character in packages/twenty-server/src/engine/core-modules/open-api/utils/parameters.utils.ts, making the filter parameter description more compact
  • This change does not affect the functionality of the computeFilterParameters function or its interaction with optimistic effect handling

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

deletedAt: () => recordToDelete.deletedAt,
},
});
cache.evict({ id: cache.identify(recordToDestroy) });
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverting a previous tentative introduced here: https://github.com/twentyhq/twenty/pull/7200/files

filter: rootQueryFilter,
objectMetadataItem,
});
const updatedRecordMatchesThisRootQueryFilter = isRecordMatchingFilter({
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should always try to matchFilter (as isDeletedAt is implicit)

}
}

if (updatedRecordShouldBeRemovedFromRootQueryEdges) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no changes here, just indent changes

@@ -67,44 +68,69 @@ export const useDeleteManyRecords = ({
(batchIndex + 1) * mutationPageSize,
);

const currentTimestamp = new Date().toISOString();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making the useDeleteMany having the EXACT same logic as useUpdateOne but batched

@@ -41,66 +42,85 @@ export const useDeleteOneRecord = ({
async (idToDelete: string) => {
const currentTimestamp = new Date().toISOString();

const cachedRecord = getRecordFromCache(idToDelete, apolloClient.cache);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

const findManyQueryName = `FindMany${capitalize(
objectMetadataItem.namePlural,
)}`;
const cachedRecords = idsToRestore.map((idToRestore) =>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@@ -120,6 +127,12 @@ export const isRecordMatchingFilter = ({
);
}

if (isLeafFilter(filter)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

taking deletedAt into account in the isRecordMatchingFilter

@@ -82,62 +82,62 @@ export class GraphqlQueryFilterFieldParser {
switch (operator) {
case 'eq':
return {
sql: `${objectNameSingular}.${key} = :${key}${uuid}`,
sql: `"${objectNameSingular}"."${key}" = :${key}${uuid}`,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another bug we spot with @Weiko

@@ -84,8 +84,7 @@ export const computeFilterParameters = (): OpenAPIV3_1.ParameterObject => {
).join('**, **')}**.
Default root conjunction is **${DEFAULT_CONJUNCTION}**.
To filter **null** values use **field[is]:NULL** or **field[is]:NOT_NULL**
To filter using **boolean** values use **field[eq]:true** or **field[eq]:false**
`,
To filter using **boolean** values use **field[eq]:true** or **field[eq]:false**`,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test fix

@charlesBochet charlesBochet merged commit 7b96be6 into main Oct 11, 2024
2 of 3 checks passed
@charlesBochet charlesBochet deleted the fix-deleted-at-optimistic-effect branch October 11, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant