fix: soft deleted records are read only #8198

pau-not-paul · 2024-10-30T12:22:56Z

TODO:

It should not be possible to add tasks, notes, files, etc.

Fix for #7172

Note for reviewer:

With these changes, deletedAt is now always included in recordGqlFields.

--- Edit from Charles --
In this PR:

As mentionned by @pau-not-paul, we are adding deletedAt to our recordGqlFields for board and table
I'm removing cellReadOnly logic, it is now fully computed using useIsFieldValueReadonly like it's done in other places, there is no need to maintain two read only systems
refactoring useIsFieldValueReadonly to take all the business logics into one place together. It's now a function of the 5 factors (isRemote, isDeleted, objectName, fieldName, etc...). Later it's likely to get back to a function of Pick, Pick, record.isDeleted but we are not there yet

Note: as all cells are listening to the record (for isDeleted), updating a field will trigger a re-rendering of the row which is not an issue right now. It will be if we introduce bulk edition. In this case we would need to use a selector on fields on top of record store

FelixMalfait · 2024-10-30T13:10:25Z

Let's go with this approach! but we need to disable additional items:

-> List view

-> Ability to create relation, tasks, etc.

FelixMalfait · 2024-10-30T13:10:54Z

/award 500

oss-gg · 2024-10-30T13:10:58Z

Awarding pau-not-paul: 500 points 🕹️ Well done! Check out your new contribution on oss.gg/pau-not-paul

charlesBochet · 2024-11-18T16:49:00Z

packages/twenty-front/src/modules/object-record/record-show/components/FieldsCard.tsx

@@ -82,7 +82,8 @@ export const FieldsCard = ({
      objectNameSingular !== CoreObjectNameSingular.Task &&
      fieldMetadataItem.name !== 'taskTargets',
  );
-  const isReadOnly = objectMetadataItem.isRemote;
+  const isReadOnly =
+    objectMetadataItem.isRemote || isDefined(recordFromStore?.deletedAt);


let's introduce a util for that isRecordReadOnly, this will help keep the code maintainable

charlesBochet · 2024-11-18T16:49:04Z

packages/twenty-front/src/modules/object-record/record-show/components/SummaryCard.tsx

@@ -45,7 +45,8 @@ export const SummaryCard = ({

  const { Icon, IconColor } = useGetStandardObjectIcon(objectNameSingular);
  const isMobile = useIsMobile() || isInRightDrawer;
-  const isReadOnly = objectMetadataItem.isRemote;
+  const isReadOnly =
+    objectMetadataItem.isRemote || isDefined(recordFromStore?.deletedAt);


charlesBochet · 2024-11-18T16:49:17Z

...-record/record-show/record-detail-section/components/RecordDetailRelationRecordsListItem.tsx

-  const canEdit = !isFieldMetadataReadOnly(fieldDefinition.metadata);
+  const record = useRecoilValue(recordStoreFamilyState(recordId));
+
+  const canEdit =


charlesBochet · 2024-11-18T16:49:21Z

...s/object-record/record-show/record-detail-section/components/RecordDetailRelationSection.tsx

@@ -158,7 +159,9 @@ export const RecordDetailRelationSection = ({
      recordId,
    });

-  const canEdit = !isFieldMetadataReadOnly(fieldDefinition.metadata);
+  const canEdit =


charlesBochet · 2024-11-18T16:49:34Z

...src/modules/object-record/record-table/record-table-row/components/RecordTableRowWrapper.tsx

-              isReadOnly: objectMetadataItem.isRemote ?? false,
+              isReadOnly:
+                objectMetadataItem.isRemote ||
+                isDefined(recordFromStore?.deletedAt),


charlesBochet

Hi @pau-not-paul, thanks for the PR and sorry for the slow review.
Overall the approach looks good, I'm going to rebase this PR and try to merge it

greptile-apps

PR Summary

This PR adds comprehensive checks for soft-deleted records across the application's UI components, preventing edits to records that have been soft-deleted by checking the deletedAt field.

Added deletedAt field to GraphQL queries in useRecordBoardRecordGqlFields.ts and useRecordTableRecordGqlFields.ts
Implemented isReadOnly logic in FieldsCard.tsx and SummaryCard.tsx to prevent edits on soft-deleted records
Added deletion checks in relation components to prevent modifications of relations for soft-deleted records
Aligned frontend behavior with backend restrictions by making soft-deleted records read-only throughout the UI

_{7 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-11-18T16:58:16Z

...s/object-record/record-show/record-detail-section/components/RecordDetailRelationSection.tsx

+  const canEdit =
+    !isFieldMetadataReadOnly(fieldDefinition.metadata) &&
+    !isDefined(record?.deletedAt);


style: Consider adding a visual indicator or tooltip to explain why editing is disabled when record is soft-deleted

greptile-apps · 2024-11-18T16:58:43Z

...src/modules/object-record/record-table/record-table-row/components/RecordTableRowWrapper.tsx

+  const [recordFromStore] = useRecoilState<any>(
+    recordStoreFamilyState(recordId),
+  );


style: Type any used for recordFromStore could be made more specific with proper record type

charlesBochet · 2024-11-20T18:19:38Z

Todo before merge: fix and complete tests on useIsFieldValueReadOnly hook and isFieldValueEmpty util

github-actions · 2024-11-21T13:03:11Z

	Fails
🚫	`node` failed.

Log

�[31mError: �[39m RequestError [HttpError]: Validation Failed: {"message":"The listed users cannot be searched either because the users do not exist or you do not have permission to view the users.","resource":"Search","field":"q","code":"invalid"}
    at /home/runner/work/twenty/twenty/node_modules/�[4m@octokit�[24m/request/dist-node/index.js:86:21
�[90m    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)�[39m {
  status: �[33m422�[39m,
  response: {
    url: �[32m'https://api.github.com/search/issues?q=is%3Apr%20author%3Apau-not-paul%20is%3Aclosed%20repo%3Atwentyhq%2Ftwenty&per_page=2&page=1'�[39m,
    status: �[33m422�[39m,
    headers: {
      �[32m'access-control-allow-origin'�[39m: �[32m'*'�[39m,
      �[32m'access-control-expose-headers'�[39m: �[32m'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset'�[39m,
      �[32m'cache-control'�[39m: �[32m'no-cache'�[39m,
      connection: �[32m'close'�[39m,
      �[32m'content-length'�[39m: �[32m'300'�[39m,
      �[32m'content-security-policy'�[39m: �[32m"default-src 'none'"�[39m,
      �[32m'content-type'�[39m: �[32m'application/json; charset=utf-8'�[39m,
      date: �[32m'Thu, 21 Nov 2024 13:03:10 GMT'�[39m,
      �[32m'referrer-policy'�[39m: �[32m'origin-when-cross-origin, strict-origin-when-cross-origin'�[39m,
      server: �[32m'github.com'�[39m,
      �[32m'strict-transport-security'�[39m: �[32m'max-age=31536000; includeSubdomains; preload'�[39m,
      vary: �[32m'Accept, Authorization, Cookie, X-GitHub-OTP,Accept-Encoding, Accept, X-Requested-With'�[39m,
      �[32m'x-accepted-github-permissions'�[39m: �[32m'allows_permissionless_access=true'�[39m,
      �[32m'x-content-type-options'�[39m: �[32m'nosniff'�[39m,
      �[32m'x-frame-options'�[39m: �[32m'deny'�[39m,
      �[32m'x-github-api-version-selected'�[39m: �[32m'2022-11-28'�[39m,
      �[32m'x-github-media-type'�[39m: �[32m'github.v3; format=json'�[39m,
      �[32m'x-github-request-id'�[39m: �[32m'0487:31003:443066:8687F2:673F2F8E'�[39m,
      �[32m'x-ratelimit-limit'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-remaining'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-reset'�[39m: �[32m'1732194250'�[39m,
      �[32m'x-ratelimit-resource'�[39m: �[32m'search'�[39m,
      �[32m'x-ratelimit-used'�[39m: �[32m'1'�[39m,
      �[32m'x-xss-protection'�[39m: �[32m'0'�[39m
    },
    data: {
      message: �[32m'Validation Failed'�[39m,
      errors: �[36m[Array]�[39m,
      documentation_url: �[32m'https://docs.github.com/v3/search/'�[39m,
      status: �[32m'422'�[39m
    }
  },
  request: {
    method: �[32m'GET'�[39m,
    url: �[32m'https://api.github.com/search/issues?q=is%3Apr%20author%3Apau-not-paul%20is%3Aclosed%20repo%3Atwentyhq%2Ftwenty&per_page=2&page=1'�[39m,
    headers: {
      accept: �[32m'application/vnd.github.v3+json'�[39m,
      �[32m'user-agent'�[39m: �[32m'octokit-rest.js/18.12.0 octokit-core.js/3.6.0 Node.js/18.20.5 (linux; x64)'�[39m,
      authorization: �[32m'token [REDACTED]'�[39m
    },
    request: { hook: �[36m[Function: bound bound register]�[39m }
  }
}
danger-results://tmp/danger-results-14644e1c.json

Generated by 🚫 dangerJS against 95bc91a

This was referenced Oct 30, 2024

fix: can update soft deleted records #8197

Closed

Impossible to update soft-deleted record #7172

Closed

pau-not-paul force-pushed the fix/soft-deleted-records-are-read-only branch from 0f166da to a2d53b3 Compare October 30, 2024 14:01

pau-not-paul marked this pull request as draft October 30, 2024 14:10

pau-not-paul force-pushed the fix/soft-deleted-records-are-read-only branch 4 times, most recently from 8548ecf to f029e44 Compare October 30, 2024 16:29

fix: soft deleted records are read only

b64611c

pau-not-paul force-pushed the fix/soft-deleted-records-are-read-only branch from f029e44 to b64611c Compare October 30, 2024 17:34

pau-not-paul marked this pull request as ready for review October 30, 2024 18:10

pau-not-paul marked this pull request as draft October 30, 2024 18:10

charlesBochet self-assigned this Nov 18, 2024

charlesBochet added the -PR: awaiting review label Nov 18, 2024

charlesBochet reviewed Nov 18, 2024

View reviewed changes

Merge branch 'main' into fix/soft-deleted-records-are-read-only

c59bcc0

charlesBochet marked this pull request as ready for review November 18, 2024 16:55

greptile-apps bot reviewed Nov 18, 2024

View reviewed changes

charlesBochet added 4 commits November 20, 2024 18:08

Merge branch 'main' into fix/soft-deleted-records-are-read-only

c85ce62

Refactor

94760d2

Fixes

587c26a

Fixes

c5fe912

charlesBochet added 2 commits November 20, 2024 19:10

Fixes

b66a394

Fix

aab99b5

charlesBochet added 2 commits November 21, 2024 13:25

Fix

45d31b7

Fix tests

95bc91a

charlesBochet merged commit ae4fb7d into twentyhq:main Nov 21, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: soft deleted records are read only #8198

fix: soft deleted records are read only #8198

pau-not-paul commented Oct 30, 2024 •

edited by charlesBochet

Loading

FelixMalfait commented Oct 30, 2024

FelixMalfait commented Oct 30, 2024

oss-gg bot commented Oct 30, 2024

charlesBochet Nov 18, 2024

charlesBochet Nov 18, 2024

charlesBochet Nov 18, 2024

charlesBochet Nov 18, 2024

charlesBochet Nov 18, 2024

charlesBochet left a comment

greptile-apps bot left a comment

greptile-apps bot Nov 18, 2024

greptile-apps bot Nov 18, 2024

charlesBochet commented Nov 20, 2024

github-actions bot commented Nov 21, 2024

fix: soft deleted records are read only #8198

fix: soft deleted records are read only #8198

Conversation

pau-not-paul commented Oct 30, 2024 • edited by charlesBochet Loading

FelixMalfait commented Oct 30, 2024

FelixMalfait commented Oct 30, 2024

oss-gg bot commented Oct 30, 2024

charlesBochet Nov 18, 2024

Choose a reason for hiding this comment

charlesBochet Nov 18, 2024

Choose a reason for hiding this comment

charlesBochet Nov 18, 2024

Choose a reason for hiding this comment

charlesBochet Nov 18, 2024

Choose a reason for hiding this comment

charlesBochet Nov 18, 2024

Choose a reason for hiding this comment

charlesBochet left a comment

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Nov 18, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 18, 2024

Choose a reason for hiding this comment

charlesBochet commented Nov 20, 2024

github-actions bot commented Nov 21, 2024

Log

pau-not-paul commented Oct 30, 2024 •

edited by charlesBochet

Loading