Skip to content

feat: implement builder deletion and associated data cleanup #276

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

Merged
merged 2 commits into from
Jun 24, 2025

Conversation

pablojhl
Copy link
Contributor

closes #141

@pablojhl pablojhl requested a review from tim-hm June 23, 2025 15:20
Copy link

github-actions bot commented Jun 23, 2025

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 86.27% 5269 / 6107
🔵 Statements 86.27% 5269 / 6107
🔵 Functions 77.66% 313 / 403
🔵 Branches 91.11% 697 / 765
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/builders/builders.services.ts 94.44% 100% 100% 94.44% 48-50
src/collections/collections.controllers.ts 90.51% 100% 100% 90.51% 236-247, 276-287
src/collections/collections.repository.ts 74.29% 80% 60% 74.29% 123-128, 165-170, 207-212, 245-249, 259, 274, 304, 324-350, 356-381
src/collections/collections.services.ts 72.38% 100% 71.42% 72.38% 146-171, 177-187
src/collections/collections.types.ts 100% 100% 100% 100%
src/data/data.repository.ts 80.05% 90.9% 64.86% 80.05% 55, 66-76, 85-95, 134, 152, 206-225, 282-301, 436-441
src/data/data.services.ts 100% 100% 100% 100%
src/queries/queries.repository.ts 80.4% 80% 62.5% 80.4% 70-75, 112-117, 149, 153-158, 193-198, 208-211
src/queries/queries.services.ts 88.64% 94.44% 90% 88.64% 38-48, 139-141, 168-171, 409-412, 448-451, 462-469, 489-495, 498-503
src/users/users.mapper.ts 74.28% 95.23% 80% 74.28% 54-65, 71-83, 104-112, 118-126, 249-250
src/users/users.repository.ts 100% 100% 66.66% 100%
src/users/users.services.ts 100% 100% 100% 100%
Generated in workflow #383 for commit ede2984 by the Vitest Coverage Report Action

Copy link
Collaborator

@tim-hm tim-hm left a comment

Choose a reason for hiding this comment

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

I think this needs a few weeks .. wdty?

@pablojhl
Copy link
Contributor Author

I think this needs a few weeks .. wdty?

If you mean waiting for the release to be public before merging this, I agree, this could add noise to the release. The comments seem like are focused on a refactor and they shouldn't take weeks.

@tim-hm
Copy link
Collaborator

tim-hm commented Jun 24, 2025

I think having the functionality merged in is important for release ... and we can tidy up the rest later. Maybe one change for this PR is having the UsersService manage the delete data references, then check if user has data, and if not then delete the user? I think the wider review can wait.

@pablojhl
Copy link
Contributor Author

I think having the functionality merged in is important for release ... and we can tidy up the rest later. Maybe one change for this PR is having the UsersService manage the delete data references, then check if user has data, and if not then delete the user? I think the wider review can wait.

Ok, I'm already working on the refactor. Hopefully it's ready in a while.

@pablojhl pablojhl force-pushed the feat/delete-builder branch from d0b7ed8 to ede2984 Compare June 24, 2025 11:11
@pablojhl pablojhl requested a review from tim-hm June 24, 2025 11:14
@pablojhl
Copy link
Contributor Author

pablojhl commented Jun 24, 2025

@tim-hm now it's cleaner. Any suggestion is welcome.

@pablojhl
Copy link
Contributor Author

I'll propose another pr cleaning other parts of the code today

@tim-hm tim-hm merged commit aa85c88 into main Jun 24, 2025
2 checks passed
@tim-hm tim-hm deleted the feat/delete-builder branch June 24, 2025 17:50
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.

Support account deletion
2 participants