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

feat: conditional field deletion warning #1337

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ryankontos
Copy link
Collaborator

feat: conditional field deletion warning

JIRA Ticket

<JIRA_TICKET>

Description

This PR introduces a warning dialogue when attempting to delete a field that is referenced in:

  • A field condition
  • A section condition
  • A templated string field

This ensures that deleting a field does not break existing conditions or fields.

Proposed Changes

  • Added a validation check before deleting a field
  • **Implemented findFieldUsage() in field-editor.tsx to detect references to a field in:
    • Field conditions
    • Section conditions
    • Templated string fields
  • **Implemented isFieldUsedInCondition in condition.tsx to recursively check if a given field is used within a given condition.
  • Modified field deletion logic to prevent deleting fields if they are referenced

How to Test

  1. Go to the Designer UI
  2. Create a new field (e.g., animal_field)
  3. Use it in one or more of the following:
    • A field condition (e.g., Show if animal_field == "Elephant")
    • A section condition
    • A TemplatedStringField (e.g., "The animal is {{animal_field}}")
  4. Try deleting the field
    • Expected result: A warning should appear, listing the places where the field is used.
    • If the field is not used anywhere, it should be deleted normally.

Additional Information

  • This change prevents unintended data loss or errors by ensuring users are warned before deleting a referenced field.
  • The warning lists all references to the field so users can update conditions or templates accordingly.

Checklist

  • I have confirmed all commits have been signed.
  • I have added JSDoc style comments to any new functions or classes.
  • Relevant documentation such as READMEs, guides, and class comments are updated.

Copy link
Contributor

@stevecassidy stevecassidy left a comment

Choose a reason for hiding this comment

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

Generally good, just a few comments and I think a change in behaviour - don't let them delete a field that is used.

const allFields = useAppSelector(
state => state.notebook['ui-specification'].fields
);
const allFviews = useAppSelector(
Copy link
Contributor

Choose a reason for hiding this comment

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

You already have this above in views

*/
const findFieldUsage = (
fieldName: string,
allFields: Record<string, any>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, I've not come across this before, very useful. Shame about the name though since we have many Record* types!

/**
* Finds where a field is used in conditions or templated string fields
*/
const findFieldUsage = (
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good but probably belongs in condition.tsx, might be useful in other places.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Funny you say that, I originally had it in condition.tsx and decided to move it when I added template support

Cancel
</Button>
<Button onClick={confirmDelete} color="error">
Delete Anyway
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would prefer to forbid deleting the field if it is used in conditions - ask them to go delete the conditions first. Then we don't end up with invalid notebooks.

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.

2 participants