Skip to content

✨ preserve user history with soft deletion#1987

Open
BenoitSerrano wants to merge 1 commit into
mainfrom
add-soft-delete
Open

✨ preserve user history with soft deletion#1987
BenoitSerrano wants to merge 1 commit into
mainfrom
add-soft-delete

Conversation

@BenoitSerrano

Copy link
Copy Markdown
Contributor

Problem
Deleting a user permanently removes all associated history and records.

Proposal
Perform a soft delete when removing a user so that historical data is preserved.

Comment thread src/managers/totp.ts Fixed
**Problem**
Deleting a user permanently removes all associated history and records.

**Proposal**
Perform a soft delete when removing a user so that historical data is preserved.

@douglasduteil douglasduteil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Very cool !

Should we close #1415 then ?

});
});

test("❎ fail to find the God-Emperor of Mankind if not active", async () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
test("❎ fail to find the God-Emperor of Mankind if not active", async () => {
test("❎ fail to find the Lion El'Jonson if deleted", async () => {

});
});

test("❎ fail to find the God-Emperor of Mankind if not active", async () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
test("❎ fail to find the God-Emperor of Mankind if not active", async () => {
test("❎ fail to find Lion El'Jonson if deleted", async () => {

});
});

test("❎ fail to find the God-Emperor of Mankind", async () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
test("❎ fail to find the God-Emperor of Mankind", async () => {
test("❎ fail to find Lion El'Jonson if deleted", async () => {

@BenoitSerrano

Copy link
Copy Markdown
Contributor Author

@rdubigny rdubigny left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thought: I find this proposal well executed but intrusive: the notion of “active” is spread widely across the code, in places where we do not want to deal with this notion. I also find this error-prone, as the odds are high that the next time we want to list users, we forget to filter against the deleted_at field.

Also, the problem you are trying to solve is unclear to me.

I am not confident merging this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants