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 relation picker for activity target #8303

Merged
merged 7 commits into from
Nov 4, 2024

Conversation

ijreilly
Copy link
Collaborator

@ijreilly ijreilly commented Nov 4, 2024

This PR introduced a regression, causing noteId or taskId (respectively for noteTarget or taskTarget creation) to be overwritten with an undefined value in the input for noteTarget or taskTarget creation.
This is because in ActivityTargetInlineCellEditMode, in addition to the noteId and taskId we are declaring, we are looking into the object (noteTarget or taskTarget)'s fields and prefilling the record-to-create with a value, potentially undefined, for all of the object fields.
So when looping over noteTarget's fields, we would find the note relation field, and eventually add note: undefined to the record-to-create input, in addition to the non-empty and valid existing noteId.
Then in sanitizeRecordInput, from the note added right above, we add an empty noteId to the input from node, overwriting the "good" noteId.

There are several ways to fix this, I chose to update prefillRecord not to add an empty "note" object that makes no sense in addition to the "noteId" we already have at this stage.
It is also possible to update sanitizeRecordInput not to overwrite a value from a relation (noteId from note relation) if there is already a value in the input.

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

This PR fixes a regression where noteId/taskId were being overwritten with undefined values during activity target creation in the relation picker component.

  • Modified ActivityTargetInlineCellEditMode.tsx to pass the full activity object instead of just ID to prefillRecord() to prevent ID overwriting
  • Fixed issue where sanitizeRecordInput was incorrectly adding empty note/task objects that overwrote valid IDs
  • Ensures proper handling of activity target creation by preventing duplicate/conflicting field declarations

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

Comment on lines 185 to 192
task:
activityObjectNameSingular === CoreObjectNameSingular.Task
? activity.id
? activity
: null,
noteId:
note:
activityObjectNameSingular === CoreObjectNameSingular.Note
? activity.id
? activity
: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Passing the full activity object here could lead to unnecessary data duplication in the database. Consider passing just the ID if that's all that's needed.

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

Discussed with Marie offline:

  • prefillRecord should not take care of relations
  • the caller should

@charlesBochet charlesBochet merged commit 76e8bf3 into main Nov 4, 2024
18 of 19 checks passed
@charlesBochet charlesBochet deleted the fix-relation-picker-note-task branch November 4, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants