fix: raise notice to delete dependent fields after workspace reset #583
Conversation
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThe changes involve modifications to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
scripts/sql/functions/delete-workspace.sql (2)
336-338: Improve SQL statement formatting and readabilityThe SQL statements in the notice can be improved for better readability:
- Add proper indentation
- Split the long line into multiple lines
- Use consistent quote style with the rest of the file
-delete from platform_schema.dependent_expense_field_mappings where expense_field_id in (select id from platform_schema.expense_fields where org_id =''%'' and type=''DEPENDENT_SELECT''); - -delete from platform_schema.expense_fields where org_id = ''%'' and type = ''DEPENDENT_SELECT'';\n\n\n\n\n\n\n\n\n\n\n' +DELETE FROM platform_schema.dependent_expense_field_mappings +WHERE expense_field_id IN ( + SELECT id + FROM platform_schema.expense_fields + WHERE org_id = ''%'' + AND type = ''DEPENDENT_SELECT'' +); + +DELETE FROM platform_schema.expense_fields +WHERE org_id = ''%'' +AND type = ''DEPENDENT_SELECT'';\n\n\n\n\n\n\n\n\n\n\n'
330-338: Consider adding diagnostic counts for dependent field deletionsFor consistency with the rest of the function, consider adding RAISE NOTICE statements with deletion counts after executing these queries in the production database.
Add these lines to the notice:
GET DIAGNOSTICS rcount = ROW_COUNT; RAISE NOTICE 'Deleted % dependent_expense_field_mappings', rcount; GET DIAGNOSTICS rcount = ROW_COUNT; RAISE NOTICE 'Deleted % dependent expense_fields', rcount;
| rollback; | ||
| begin; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary transaction block
The rollback and begin statements in the notice are unnecessary since these are just example queries to be run later. They should be removed to avoid confusion.
-rollback;
-begin;| RAISE NOTICE E'\n\n\n\n\n\n\n\n\nSwitch to prod db and run the below queries to delete dependent fields | ||
|
|
||
|
|
||
| rollback; | ||
| begin; | ||
|
|
||
| delete from platform_schema.dependent_expense_field_mappings where expense_field_id in (select id from platform_schema.expense_fields where org_id =''%'' and type=''DEPENDENT_SELECT''); | ||
|
|
||
| delete from platform_schema.expense_fields where org_id = ''%'' and type = ''DEPENDENT_SELECT'';\n\n\n\n\n\n\n\n\n\n\n', _org_id, _org_id; |
There was a problem hiding this comment.
💡 Codebase verification
The deletion order is correct, but additional cleanup may be needed
The verification reveals:
- The
mapping_settingstable has a foreign key constraint toexpense_fields, but it's DEFERRABLE INITIALLY DEFERRED, which means the deletion order won't be affected - The current deletion order (dependent mappings before fields) is correct
- However, you should also clean up any related
mapping_settingsrecords before deleting the expense fields
Suggested cleanup order:
- Delete from dependent_expense_field_mappings
- Delete from mapping_settings where expense_field_id matches
- Delete from expense_fields
🔗 Analysis chain
Verify the impact of deleting dependent fields
The new cleanup steps for dependent fields should be verified to ensure:
- No other tables have foreign key dependencies on these records
- The deletion order is correct (dependent mappings before fields)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential foreign key dependencies on dependent fields
# Search for foreign key references to dependent_expense_field_mappings
rg -i "foreign\s+key.*dependent_expense_field_mappings"
# Search for foreign key references to expense_fields
rg -i "foreign\s+key.*expense_fields"
# Search for any other references to these tables that might indicate dependencies
rg -i "dependent_expense_field_mappings|expense_fields.*type\s*=\s*'DEPENDENT_SELECT'"
Length of output: 653
Script:
#!/bin/bash
# Let's check for any references to mapping_settings table and its relationship with expense_fields
rg -i "mapping_settings.*expense_field" -A 3 -B 3
# Also check if there are any other tables referencing expense_fields
ast-grep --pattern 'CREATE TABLE $_ (
$$$
FOREIGN KEY ($$$) REFERENCES expense_fields($$$)
$$$
)'
# Check if mapping_settings table has any relation to DEPENDENT_SELECT type fields
rg -i "mapping_settings.*type.*DEPENDENT_SELECT" -A 3 -B 3
Length of output: 2782
|
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
Clickup
app.clickup.com
Summary by CodeRabbit