chore: remove feedback records which are beyond retention policy #2766

Mike-Heneghan · 2024-02-08T16:36:53Z

What

Delete feedback records which were created before the retention policy date
Rename development -> local for Slack warnings

Why

feedback can contain personally identifiable information so needs to be sanitised or deleted
Records older than 28 days are likely to have little value and wou;d already have been downloaded and processed
The development name is easy to conflate with staging i.e. our .dev environment
Renaming to Local explicits shows there's been a failure running sanitisation based on local environment

Screenshot

Rename

Testing

Added feedback records and manually changed the dates in the hasura console
When cron job triggered the expected records were deleted successfully

github-actions · 2024-02-08T16:37:18Z

🤖 Hasura Change Summary compared a subset of table metadata including permissions:

Updated Tables (1)

public.feedback permissions:

	insert	select	update	delete
api		➕		➕

2 added column permissions

	insert	select	update
api		➕ created_at ➕ id

Mike-Heneghan · 2024-02-08T16:47:26Z

api.planx.uk/helpers.ts

@@ -247,6 +247,10 @@ const getFormattedEnvironment = (): string => {
    const pizzaNumber = new URL(process.env.API_URL_EXT!).href.split(".")[1];
    environment += ` ${pizzaNumber}`;
  }
+  // For readability redefine development as local for slack warnings
+  if (environment === "development") {
+    environment = "local";


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.

Local is more explicitly not staging, this is fine by me! All for lowering stress / blood pressure 😂

github-actions · 2024-02-08T17:01:40Z

Removed vultr server and associated DNS entries

- Add deleteFeedback operation - Grant delete and select permissions of the feedback table to the API - Update Hasura test with new permissions

Mike-Heneghan · 2024-02-09T10:24:31Z

api.planx.uk/modules/webhooks/service/sanitiseApplicationData/operations.ts

+  const {
+    feedback: { returning: result },
+  } = await $api.client.request<{ feedback: Result }>(mutation, {
+    retentionPeriod: getRetentionPeriod(),


Comment

I erroneously looked at the getRetentionPeriod from pay: editor.planx.uk/src/lib/pay.ts which is 28 days 🤦

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?

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

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.

DafyddLlyr

One small comment / question, but happy for this to go ahead as-is.

I've added a checkbox to the Trello ticket to ensure we remember to update our Data Retention Policy on Notion. This could actually be done now, or after we strip out Feeback Fish.

DafyddLlyr · 2024-02-09T12:08:58Z

api.planx.uk/helpers.ts

@@ -247,6 +247,10 @@ const getFormattedEnvironment = (): string => {
    const pizzaNumber = new URL(process.env.API_URL_EXT!).href.split(".")[1];
    environment += ` ${pizzaNumber}`;
  }
+  // For readability redefine development as local for slack warnings
+  if (environment === "development") {
+    environment = "local";


Local is more explicitly not staging, this is fine by me! All for lowering stress / blood pressure 😂

DafyddLlyr · 2024-02-09T12:10:35Z

api.planx.uk/modules/webhooks/service/sanitiseApplicationData/operations.ts

+  const {
+    feedback: { returning: result },
+  } = await $api.client.request<{ feedback: Result }>(mutation, {
+    retentionPeriod: getRetentionPeriod(),


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

DafyddLlyr · 2024-02-09T12:11:02Z

hasura.planx.uk/tests/feedback.test.js

+    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");


Thanks for picking this up 👍

DafyddLlyr · 2024-02-09T12:12:42Z

hasura.planx.uk/metadata/tables.yaml

@@ -248,6 +248,29 @@
          - user_context
          - user_data
      comment: Allow users who want to leave feedback on their experience to write to the table
+  select_permissions:


nit/question: Are these select permissions needed for the mutation DeleteFeedback to work? It looks like we might need id for the returning clause but no more? Genuinely unusure on this one - this could be correct and I just have a hazy memory of how this works 😅

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.

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. created_at for the where and id for the return value.

I've updated the permissions to reflect this: b259adc

Mike-Heneghan requested a review from a team February 8, 2024 16:37

Mike-Heneghan force-pushed the mh/feedback-post-retention-deletion branch from 2e9b95b to 1e9d355 Compare February 8, 2024 16:41

Mike-Heneghan commented Feb 8, 2024

View reviewed changes

Mike-Heneghan added 2 commits February 8, 2024 17:02

chore: setup deletion of feedback created before retention policy date

6f9755c

- Add deleteFeedback operation - Grant delete and select permissions of the feedback table to the API - Update Hasura test with new permissions

chore: rename "development" to "local" for slack notifications

9705b7f

Mike-Heneghan force-pushed the mh/feedback-post-retention-deletion branch from 1e9d355 to 9705b7f Compare February 8, 2024 17:04

Mike-Heneghan self-assigned this Feb 8, 2024

Mike-Heneghan commented Feb 9, 2024

View reviewed changes

DafyddLlyr approved these changes Feb 9, 2024

View reviewed changes

chore: remove unnecessary columns from api select permission

b259adc

DafyddLlyr approved these changes Feb 9, 2024

View reviewed changes

Mike-Heneghan merged commit 15af1b0 into main Feb 9, 2024
12 checks passed

Mike-Heneghan deleted the mh/feedback-post-retention-deletion branch February 9, 2024 14:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: remove feedback records which are beyond retention policy #2766

chore: remove feedback records which are beyond retention policy #2766

Mike-Heneghan commented Feb 8, 2024 •

edited

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

Mike-Heneghan Feb 8, 2024

DafyddLlyr Feb 9, 2024

github-actions bot commented Feb 8, 2024 •

edited

Loading

Mike-Heneghan Feb 9, 2024

DafyddLlyr Feb 9, 2024

Mike-Heneghan Feb 9, 2024

DafyddLlyr left a comment

DafyddLlyr Feb 9, 2024

DafyddLlyr Feb 9, 2024

DafyddLlyr Feb 9, 2024

DafyddLlyr Feb 9, 2024 •

edited

Loading

Mike-Heneghan Feb 9, 2024

Mike-Heneghan Feb 9, 2024

chore: remove feedback records which are beyond retention policy #2766

chore: remove feedback records which are beyond retention policy #2766

Conversation

Mike-Heneghan commented Feb 8, 2024 • edited Loading

What

Why

Screenshot

Testing

github-actions bot commented Feb 8, 2024 • edited Loading

Updated Tables (1)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DafyddLlyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DafyddLlyr Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mike-Heneghan commented Feb 8, 2024 •

edited

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

DafyddLlyr Feb 9, 2024 •

edited

Loading