Skip to content
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

Merged
merged 3 commits into from
Feb 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api.planx.uk/helpers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe("getEnvironment function", () => {

test("Development env", () => {
process.env.NODE_ENV = "development";
expect(getFormattedEnvironment()).toBe("Development");
expect(getFormattedEnvironment()).toBe("Local");
});
});

Expand Down
4 changes: 4 additions & 0 deletions api.planx.uk/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Contributor Author

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.

Copy link
Contributor

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 😂

}
return capitalize(environment);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,3 +67,13 @@ export const mockGetExpiredSessionIdsQuery = {
lowcal_sessions: [{ id: "id1" }, { id: "id2" }, { id: "id3" }],
},
};

export const mockDeleteFeedbackMutation = {
name: "DeleteFeedback",
matchOnVariables: false,
data: {
feedback: {
returning: mockIds,
},
},
};
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
mockSanitiseUniformApplicationsMutation,
mockGetExpiredSessionIdsQuery,
mockDeletePaymentRequests,
mockDeleteFeedbackMutation,
} from "./mocks/queries";
import {
deleteHasuraEventLogs,
Expand All @@ -23,6 +24,7 @@ import {
deleteApplicationFiles,
deletePaymentRequests,
deleteHasuraScheduledEventsForSubmittedSessions,
deleteFeedback,
} from "./operations";

jest.mock("../../../../lib/hasura/schema");
Expand Down Expand Up @@ -142,6 +144,10 @@ describe("Data sanitation operations", () => {
operation: deletePaymentRequests,
query: mockDeletePaymentRequests,
},
{
operation: deleteFeedback,
query: mockDeleteFeedbackMutation,
},
];

for (const { operation, query } of testCases) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
};
13 changes: 13 additions & 0 deletions hasura.planx.uk/metadata/tables.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Contributor

@DafyddLlyr DafyddLlyr Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Expand Down
17 changes: 13 additions & 4 deletions hasura.planx.uk/tests/feedback.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up 👍

});
});
});
Loading