-
Notifications
You must be signed in to change notification settings - Fork 2
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
chore: remove feedback records which are beyond retention policy #2766
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,9 @@ export const getOperations = (): Operation[] => [ | |
|
||
// Queued up scheduled events (backup method to PG function/trigger) | ||
deleteHasuraScheduledEventsForSubmittedSessions, | ||
|
||
// Feedback records | ||
deleteFeedback, | ||
]; | ||
|
||
export const operationHandler = async ( | ||
|
@@ -291,3 +294,23 @@ export const deleteHasuraScheduledEventsForSubmittedSessions: Operation = | |
const [_column_name, ...ids] = response?.result?.flat() || []; | ||
return ids; | ||
}; | ||
|
||
export const deleteFeedback: Operation = async () => { | ||
const mutation = gql` | ||
mutation DeleteFeedback($retentionPeriod: timestamptz) { | ||
feedback: delete_feedback( | ||
where: { created_at: { _lt: $retentionPeriod } } | ||
) { | ||
returning { | ||
id | ||
} | ||
} | ||
} | ||
`; | ||
const { | ||
feedback: { returning: result }, | ||
} = await $api.client.request<{ feedback: Result }>(mutation, { | ||
retentionPeriod: getRetentionPeriod(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment I erroneously looked at the At the top of the file it has the period as 6 months. I'm wondering whether we stick to 6 months to be consistent with the rest of the sanitisation or swap it to 28 days as originally planned? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep no problems here - the simpler the better 👍 Let's make sure we communicate this to George / August / Silvia and update Notion to now reflect the changes made in this PR - https://www.notion.so/opensystemslab/Data-Retention-Policy-fcd47af2a8244888b544a1e9c92c88f8 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great shout! I didn't think to look for a data policy on Notion. The other place is https://www.planx.uk/privacy and Alastair has advised I should give him a shout when the broader feature is ready and he'll update this. Another place would be the flow setting level privacy policy potentially. |
||
}); | ||
return result; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -248,6 +248,19 @@ | |
- user_context | ||
- user_data | ||
comment: Allow users who want to leave feedback on their experience to write to the table | ||
select_permissions: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit/question: Are these select permissions needed for the mutation There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahahahaha I also wasn't sure about this 😅 I added the full column permissions when I was having issues. I couldn't seem to get a straight answer from the docs and I saw that other sanitised tables had the full select permission. I can work this out by restricting access and testing it not that the feature is working. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks again for picking up on this @DafyddLlyr I kind of wanted to do this testing but was worried I'd been taking too long on this piece of work. So it was a great excuse to try it out! As discussed it seems the delete requires select permissions for any columns it references i.e. I've updated the permissions to reflect this: b259adc |
||
- role: api | ||
permission: | ||
columns: | ||
- created_at | ||
- id | ||
filter: {} | ||
comment: "" | ||
delete_permissions: | ||
- role: api | ||
permission: | ||
filter: {} | ||
comment: "" | ||
- table: | ||
name: feedback_status_enum | ||
schema: public | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,12 +80,21 @@ describe("feedback", () => { | |
i = await introspectAs("api"); | ||
}); | ||
|
||
test("cannot query feedback", () => { | ||
expect(i.queries).not.toContain("feedback"); | ||
test("can query feedback", () => { | ||
expect(i.queries).toContain("feedback"); | ||
}); | ||
|
||
test("cannot create, update, or delete teams", () => { | ||
expect(i).toHaveNoMutationsFor("feedback"); | ||
test("cannot update feedback", () => { | ||
expect(i.mutations).not.toContain("update_feedback"); | ||
expect(i.mutations).not.toContain("update_feedback_by_pk"); | ||
}); | ||
|
||
test("can delete feedback", async () => { | ||
expect(i.mutations).toContain("delete_feedback"); | ||
}); | ||
|
||
test("cannot insert feedback", async () => { | ||
expect(i.mutations).not.toContain("insert_feedback"); | ||
Comment on lines
+83
to
+97
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for picking this up 👍 |
||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context:
I didn't expect to see a Slack message when my local test of running the sanitisation failed. I think the message was very useful but my first (incorrect) instinct was that I'd triggered something in a live environment.
I was relieved when I saw "Development" although with it being the end of the day and being tired and I did wonder if it was referring to staging and if I'd misunderstood the
.env
setup for the webhook.Realistically I don't think I'll make this mistake again and I'm happy to leave this as "Development" if there are any strong opinions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Local is more explicitly not staging, this is fine by me! All for lowering stress / blood pressure 😂