Feat: Add Edit/Delete dropdown #3592
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
enhancement
QA Summary
Test CoverageCoverage report for `packages/client`
Coverage report for `packages/server`
|
Terraform Summary
Hint: If "Terraform Format & Style" failed, run OutputValidation OutputPlan Summary
Pusher: @greg-adams, Action: |
TylerHendrickson
requested changes
Oct 10, 2024
There was a problem hiding this comment.
@greg-adams Everything looks good in testing! A couple server-side things to potentially address and/or get your take on.
Comment on lines
151
to
160
| const grantNote = knex | ||
| .select('id') | ||
| .from('grant_notes') | ||
| .where({ grant_id: grantId, user_id: userId }); | ||
|
|
||
| await knex('grant_notes_revisions') | ||
| .whereIn('grant_note_id', grantNote) | ||
| .del(); | ||
|
|
||
| await grantNote.del(); |
There was a problem hiding this comment.
I think it would be preferable to handle this on the Postgres side using ON DELETE CASCADE. Unfortunately, that doesn't currently seem to be implemented on the FK constraint, so it's probably best that we do that now since there's no data in Production just yet.
A few details that feel worth mentioning:
- Postgres doesn't support this kind of alteration on foreign keys, so the migration will need to drop the existing FK constraint and then re-add it with the
ON DELETE CASCADEspecification. - Knex seems to have its own naming convention for foreign key constraints (it doesn't follow the Postgres standard convention of
{tablename}_{columnname(s)}_{suffix}) so rather than usingknex.raw(), it might be best to use the Knex schema builder functions (seetable.dropForeign()andtable.foreign()).
There was a problem hiding this comment.
yes certainly makes sense here - updated!
packages/server/src/routes/grants.js
Outdated
| // sending the notes as JSON response | ||
| res.json(notes); | ||
| } catch (error) { | ||
| res.status(500).json({ error: 'Failed to retrieve notes' }); |
There was a problem hiding this comment.
Is it possible to re-throw error on the next line so that we can preserve the error/trace details, or will that mess with the ability to provide the Failed to retrieve notes message in the response?
There was a problem hiding this comment.
yes, I removed the try..catch here as we aren't using this message on the FE and will still result in a 500
github-actions
bot
added
the
database-changes
TylerHendrickson
requested changes
Oct 17, 2024
There was a problem hiding this comment.
@greg-adams A few quick suggestions for the new Postgres function, but I think this is otherwise ready.
Comment on lines
8
to
16
| const ON_UPDATE_TIMESTAMP_FUNCTION = ` | ||
| CREATE OR REPLACE FUNCTION on_update_timestamp() | ||
| RETURNS trigger AS $$ | ||
| BEGIN | ||
| NEW.updated_at = now(); | ||
| RETURN NEW; | ||
| END; | ||
| $$ language 'plpgsql'; | ||
| `; |
There was a problem hiding this comment.
FWIW when you asked me about this is Slack I thought you confirming that updated_at columns should have a DEFAULT now() specification rather than ensuring that these column updates should happen automatically 😄
That said, this seems like a nice behavior to have on-hand, and is more comprehensive than a simple default. 👍 !
Comment on lines
11
to
13
| BEGIN | ||
| NEW.updated_at = now(); | ||
| RETURN NEW; |
There was a problem hiding this comment.
Slight tweak that avoids setting updated_at if the update operation isn't actually changing the row (e.g. UPDATE my_table SET my_col = 123 when my_col is already 123):
Suggested change
| BEGIN | |
| NEW.updated_at = now(); | |
| RETURN NEW; | |
| BEGIN | |
| IF NEW IS DISTINCT FROM OLD THEN | |
| NEW.updated_at = now(); | |
| END IF; | |
| RETURN NEW; | |
| END; |
| */ | ||
| exports.up = async function (knex) { | ||
| const ON_UPDATE_TIMESTAMP_FUNCTION = ` | ||
| CREATE OR REPLACE FUNCTION on_update_timestamp() |
There was a problem hiding this comment.
Just making it a bit easier to locate quickly during a psql session :)
Suggested change
| CREATE OR REPLACE FUNCTION on_update_timestamp() | |
| CREATE OR REPLACE FUNCTION before_update_set_updated_at() |
packages/server/knexfile.js
Outdated
| // Set trigger for any updated timestamps | ||
| onUpdateTrigger: (table) => ` | ||
| CREATE TRIGGER ${table}_updated_at BEFORE UPDATE ON ${table} | ||
| FOR EACH ROW EXECUTE PROCEDURE on_update_timestamp(); |
There was a problem hiding this comment.
Per my previous comment
Suggested change
| FOR EACH ROW EXECUTE PROCEDURE on_update_timestamp(); | |
| FOR EACH ROW EXECUTE PROCEDURE before_update_set_updated_at(); |
|
@TylerHendrickson thanks! I incorporated your changes and some small changes to another migration |
greg-adams
force-pushed
the
feat/notes-delete
branch
from
e287c45 to
56ddc97
Compare
TylerHendrickson
approved these changes
Oct 17, 2024
| @@ -17,7 +17,8 @@ exports.up = function (knex) { | |||
| * @param { import("knex").Knex } knex | |||
| * @returns { Promise<void> } | |||
| */ | |||
| exports.down = function (knex) { | |||
| exports.down = async function (knex) { | |||
| await knex('email_subscriptions').where({ notification_type: 'GRANT_ACTIVITY' }).del(); | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket #3591
Description
grant_notestable to support soft-delete of notes:is_publishedandupdated_atScreenshots / Demo Video
Testing
Automated and Unit Tests
Manual tests for Reviewer
Checklist