fix: dependent field queries not printing the org id #231
Conversation
WalkthroughThe changes primarily involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Database
User->>Database: Call delete_workspace(_workspace_id)
Database->>Database: Retrieve org_id from workspaces
Database->>Database: Execute DELETE statements
Database->>Database: GET DIAGNOSTICS for affected rows
Database->>User: RAISE NOTICE with affected rows
Database->>User: RETURN
Possibly related PRs
Suggested reviewers
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 (
|
|
PR description must contain a link to a ClickUp (case-insensitive) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
scripts/sql/reset_db.sql (2)
206-207: Improve clarity and readability of the notice messages.While the org_id is now correctly included, there are several improvements we can make to these notice messages:
- Reduce the excessive newlines that make logs harder to read
- Add more context about why these queries need to be run in prod
- Format the SQL queries for better readability
Consider this improvement:
- RAISE NOTICE E'\n\n\n\n\n\n\n\n\nSwitch to prod db and run the below queries to delete dependent fields'; - RAISE NOTICE E'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; + RAISE NOTICE E'\n=== Manual Steps Required ==='; + RAISE NOTICE 'The following queries must be executed in the PRODUCTION database to clean up dependent fields for org_id: %', _org_id; + RAISE NOTICE E'BEGIN; + -- Delete dependent field mappings + 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 dependent fields + DELETE FROM platform_schema.expense_fields + WHERE org_id = ''%'' AND type = ''DEPENDENT_SELECT''; +COMMIT;', _org_id, _org_id;This format:
- Clearly indicates these are manual steps
- Shows these are production database operations
- Formats the SQL for better readability
- Adds comments explaining each operation
- Uses a single newline for separation
Line range hint
1-210: Consider adding transaction management and error handling.While the specific changes look good, there are some architectural improvements that could make this function more robust:
- Add explicit transaction management to ensure atomic operations
- Validate workspace existence before proceeding
- Add error handling for failed deletions
Consider wrapping the operations in a transaction and adding validation:
CREATE OR REPLACE FUNCTION delete_workspace(IN _workspace_id integer) RETURNS void AS $$ DECLARE rcount integer; _org_id varchar(255); BEGIN -- Validate workspace exists IF NOT EXISTS (SELECT 1 FROM workspaces WHERE id = _workspace_id) THEN RAISE EXCEPTION 'Workspace % does not exist', _workspace_id; END IF; -- Start transaction BEGIN -- Existing delete operations... -- Commit if all operations succeed COMMIT; EXCEPTION WHEN OTHERS THEN -- Rollback on any error ROLLBACK; RAISE EXCEPTION 'Failed to delete workspace %: %', _workspace_id, SQLERRM; END; END $$ LANGUAGE plpgsql;This would:
- Ensure all operations succeed or none do
- Prevent attempts to delete non-existent workspaces
- Provide clear error messages on failure
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/sql/reset_db.sql(1 hunks)
🔇 Additional comments (1)
scripts/sql/reset_db.sql (1)
198-199: LGTM! Strategic placement of org_id retrieval.
The _org_id is now correctly retrieved from the workspaces table before the workspace deletion. This fixes the issue where dependent field queries weren't printing the org id.
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
JustARatherRidiculouslyLongUsername
requested review from
ashwin1111 and
ruuushhh
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
JustARatherRidiculouslyLongUsername
merged commit def3e25
into
dependent-field-notice-after-reset
) * fix: raise notice to delete dependent fields after workspace reset * refactor: squish query into one-liner * fix: dependent field queries not printing the org id (#231) * fix: dependent field queries not printing the org id * fix: delete `expense_attributes_deletion_cache` while workspace reset (#232)
app.clickup.com
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Style