Skip to content

Create delete route for account deletion#3968

Open
5e-Cleric wants to merge 15 commits intomasterfrom
delete-route-for-account-deletion
Open

Create delete route for account deletion#3968
5e-Cleric wants to merge 15 commits intomasterfrom
delete-route-for-account-deletion

Conversation

@5e-Cleric
Copy link
Copy Markdown
Member

Description

Just one change, adds a deletion route in app.js, which makes a request to the delete method for all documents based on username.

The delete method should handle deletion or author removal automatically.

Related Issues or Discussion

I might transform this into a clone of the delete brew function but working in batches

After this PR is merged i plan on moving some of the routes in app.js into other files, as that one is starting to get gargantuan.

@5e-Cleric 5e-Cleric self-assigned this Jan 2, 2025
@5e-Cleric 5e-Cleric added feature Epic Big feature that touches a lot of files or involves many sub-features P1 - high priority Obvious bug or popular features 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed labels Jan 2, 2025
@calculuschild calculuschild temporarily deployed to homebrewery-pr-3968 January 2, 2025 14:44 Inactive
Copy link
Copy Markdown
Member

@calculuschild calculuschild left a comment

Choose a reason for hiding this comment

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

Left some fixes.

@calculuschild calculuschild added 🔍 R3 - Reviewed - Awaiting Fixes 🔧 PR is okayed but needs fixes before merging and removed 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed labels Jan 3, 2025
@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-3968 January 11, 2025 22:46 Inactive
@calculuschild
Copy link
Copy Markdown
Member

calculuschild commented Apr 14, 2025

What else is needed on this? I recall there was some issue with CORS or Cookies something?

@5e-Cleric
Copy link
Copy Markdown
Member Author

The main problem is that testing for this feature is a pain to do since it requires both naturalcrit and homebrewery to be running locally, and certain changes to be made (i don't recall specifics right now), it also cannot be tested in deployments for cookie domain reasons.

These are also why i had many problems with the other functions, apart from my inexperience.

I am going to come back to this shortly, lost my drive on this project, but i can do it nonetheless.

@5e-Cleric
Copy link
Copy Markdown
Member Author

@calculuschild this would be way easier if the account systems in local were connected like they are on live, otherwise its a mess, i can do it, but it is a mess.

@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-3968 April 19, 2025 17:25 Inactive
@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-3968 April 19, 2025 18:31 Inactive
@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-3968 April 19, 2025 18:44 Inactive
@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-3968 April 19, 2025 18:48 Inactive
@5e-Cleric
Copy link
Copy Markdown
Member Author

This should be done, but it cannot be tested thoroughly:

to test in local you must make up a req.account object with at least a username
same thing in a deployment since the cookies do not work through heroku deployments

@5e-Cleric
Copy link
Copy Markdown
Member Author

this should be released before the actual deletion api is set in naturalcrit.

@calculuschild
Copy link
Copy Markdown
Member

calculuschild commented Apr 19, 2025

@calculuschild this would be way easier if the account systems in local were connected like they are on live, otherwise its a mess, i can do it, but it is a mess.

They are connected in local in the same way. Or they should be; I used it extensively when I was implementing Google Drive integration. Log in via a naturalcrit server on localhost, and you will be logged in on the homebrewery at localhost.

If you are not seeing that there is a bug somewhere, or I am misunderstanding what you mean.

@5e-Cleric
Copy link
Copy Markdown
Member Author

@calculuschild this would be way easier if the account systems in local were connected like they are on live, otherwise its a mess, i can do it, but it is a mess.

They are connected in local in the same way. Or they should be; I used it extensively when I was implementing Google Drive integration. Log in via a naturalcrit server on localhost, and you will be logged in on the homebrewery at localhost.

If you are not seeing that there is a bug somewhere, or I am misunderstanding what you mean.

The logins are not connected for me at all, logins are kept or at least require hard reloads, and logging in homebrewery just requires you to type a username and it logs you in, while in naturalcrit you need to provide a valid username and password, how is that connected?

@calculuschild
Copy link
Copy Markdown
Member

calculuschild commented Apr 22, 2025

logging in homebrewery just requires you to type a username and it logs you in, while in naturalcrit you need to provide a valid username and password, how is that connected?

We added the username-only login as a convenience for people using an offline deployment of Homebrewery. It creates a simplified cookie and does not go through the user authentication process at all.

But if you have both servers running locally (probably naturalcrit on port :8010, and homebrewery on port :8000), including mongoDB databases for both, you should still be able to log in from the naturalcrit server. You will have to manually visit localhost:8010, because on local installs the login button points to the "simplified" login popup instead. But once you are there, you can follow the username/password prompts as normal, which generates a "full" cookie that will work on the local Homebrewery Server. I did this not too long ago when we were hitting the Google throttling errors in order to test some things.

@5e-Cleric
Copy link
Copy Markdown
Member Author

Huh, well, nevertheless, the PR works

@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-3968 May 9, 2025 10:25 Inactive
@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-3968 May 9, 2025 16:16 Inactive
@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-3968 May 9, 2025 16:23 Inactive
@5e-Cleric 5e-Cleric added 🔍 R4 - Fixed! Awaiting re-review⭐ PR review comments addressed and removed 🔍 R3 - Reviewed - Awaiting Fixes 🔧 PR is okayed but needs fixes before merging labels May 9, 2025
@5e-Cleric 5e-Cleric moved this from Backlog to Under Review in @calculuschild's backlog May 11, 2025
@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-3968 May 25, 2025 13:32 Inactive
@5e-Cleric 5e-Cleric moved this from Under Review to Pushing to Finish in @calculuschild's backlog Jun 25, 2025
@calculuschild
Copy link
Copy Markdown
Member

calculuschild commented Jul 16, 2025

Huh, well, nevertheless, the PR works

Unfortunately, it still doesn't work. If there is more than one brew on an account, it only deletes the first one, and then crashes the Homebrewery side. The Naturalcrit side will continue to delete the account but the rest of the brews remain.

Homebrewery provides this log:

node:_http_outgoing:848
    throw new ERR_HTTP_HEADERS_SENT('remove');
          ^

Error [ERR_HTTP_HEADERS_SENT]: Cannot remove headers after they are sent to the client
    at ServerResponse.removeHeader (node:_http_outgoing:848:11)
    at ServerResponse.send (C:\Users\Trevor\Documents\GitHub\homebrewery\node_modules\express\lib\response.js:201:10)
    at Object.deleteBrew (file:///C:/Users/Trevor/Documents/GitHub/homebrewery/server/homebrew.api.js:529:19)
    at process.processTicksAndRejections (node:internal/process/task_queues:105:5) {
  code: 'ERR_HTTP_HEADERS_SENT'
}

Looks like this line does not wan to be called more than once, i.e., can't reuse the same res object to return multiple responses.

res.status(204).send();

Before, you were directly deleting from homebrewModel, which would work, except that does not delete Google Drive documents, and also skips over some of the other possibilities (deleting yourself as author instead of the whole file if you are a co-author, etc.), and this is where I suggested we just reuse the api.deleteBrew(). However, I did not realize multiple responses would be an issue here.

What we may need to do is pull out the core part of api.deleteBrew() (maybe most of it) into its own function that does not rely on the req/res objects, and call that instead. Similar to how we have api.excludePropsFromUpdate() pulled out of api.updateBrew(). Then api.deleteBrew() becomes just a wrapper that takes in req, res, and pulls out the necessary values for the actual delete step.

@calculuschild calculuschild added 🔍 R3 - Reviewed - Awaiting Fixes 🔧 PR is okayed but needs fixes before merging and removed 🔍 R4 - Fixed! Awaiting re-review⭐ PR review comments addressed labels Jul 16, 2025
@5e-Cleric 5e-Cleric moved this from Pushing to Finish to Under Review in @calculuschild's backlog Mar 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Epic Big feature that touches a lot of files or involves many sub-features feature P1 - high priority Obvious bug or popular features 🔍 R3 - Reviewed - Awaiting Fixes 🔧 PR is okayed but needs fixes before merging

Projects

Status: Under Review

Development

Successfully merging this pull request may close these issues.

2 participants