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

Right to be forgotten #603

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Right to be forgotten #603

wants to merge 15 commits into from

Conversation

Mikearaya
Copy link
Contributor

No description provided.

@Mikearaya
Copy link
Contributor Author

@Mikearaya Mikearaya requested a review from pozylon August 20, 2024 20:18
@pozylon
Copy link
Member

pozylon commented Aug 21, 2024

@Mikearaya please check the requested changes on the old PR: #581

We should not alter the orders after checkout, also reviews should stay by default and when the user has been marked deleted, a frontend can show the review as authored by a deleted user. I'd add a second mutation to delete all product reviews of a user.

So we have options:

  1. remove my own or any (when admin) user but keep reviews
  2. remove my own or any (when admin) user and also remove reviews
  3. remove any (when admin) user's reviews later, no matter if the user is deleted or not

changes.diff Outdated Show resolved Hide resolved
packages/core-users/src/module/configureUsersModule.ts Outdated Show resolved Hide resolved
@pozylon
Copy link
Member

pozylon commented Nov 26, 2024

Okay, i have many requests to make this perfect:

Keep the same naming of deletion mutation, thus the mutation should be called removeUser. That mutation already exists and it should be extended then to remove the reviews when the boolean flag has been set:

removeUser(removeUserReviews: Boolean = false)

In general, removeUser should always (in addition to removing/masking personal data):

  • Remove carts of that user (and the cart's order positions, order delivery, order payment etc.)
  • Remove login sessions
  • Remove bookmarks of that user
  • Remove draft/open quotations
  • Remove draft/open enrollmens
  • Remove payment credentials

You don't need to purge events or work queue because that data is transient and get's removed after a while automatically.

Also make sure to emit an event when the user has been deleted, that event should contain the old data of the user before masking.

Removing a user should not be allowed if there is any active subscription (enrollment), pending orders, quotations in status requested/processing, in that case you need to throw errors explaining the reason why they can't delete the user yet.

@pozylon pozylon assigned pozylon and Mikearaya and unassigned pozylon Nov 26, 2024
@Mikearaya
Copy link
Contributor Author

Mikearaya commented Nov 26, 2024

@pozylon ok there is no problem with deleting order payment and delivery but why delete order positions and orders when the user data is masked, that is why we masked the user instead if removing it completely right? we might need it for amalitics ir some part if the system might depend on it

@Mikearaya
Copy link
Contributor Author

@pozylon we had this discussion here #603 (comment)

@pozylon
Copy link
Member

pozylon commented Nov 26, 2024

@Mikearaya only those related entities belonging to an order in status open/null (= carts)

@pozylon
Copy link
Member

pozylon commented Nov 26, 2024

Currently "orders.delete" looks like this and is currently only triggered through removeOrder mutation which checks first if it's a cart or not.:

delete: async (orderId) => {
const deletedCount = await mutations.delete(orderId);
await emit('ORDER_REMOVE', { orderId });
return deletedCount;
},

which is bad, because it leaves order_* documents in the db with orderId's that don't exist anymore. So we should extend that function and call it within user deletion for all carts of that user.

@pozylon pozylon closed this Nov 26, 2024
@pozylon pozylon reopened this Nov 26, 2024
@pozylon
Copy link
Member

pozylon commented Nov 26, 2024

ups, didn't want to close. i just had one more remark coming to my mind about this and our discussion on basecamp:

If a user to be deleted (no matter if guest or not) meets a range of conditions after all the mutations done (like masking, removing payment credentials, carts etc.), we can safely delete the document from the collection to get rid of the lonely user document.

To be a valid user candidate for complete removal, the user needs to match all of the following conditions:

  • No reviews linked to the user left
  • No orders (carts and checked out orders) linked to the user left
  • No enrollments linked to the user left
  • No quotations linked to the user left
  • No tokens linked to the user left

So when I were building this, I'd do all the stuff that we discussed, then do those checks and eventually completely remove the user from the db.

In general this will help us to clean up (mostly guest) users that are hanging around in the db. The ZombieKiller plugin could be extended to do this for guests that didn't login for X days. And because of the other logic, would also completely get rid of stale guests that have stuff in the cart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants