Skip to content

Conversation

@floscodes
Copy link

This PR adds a user deletion functionality to popular tasks.

@floscodes
Copy link
Author

floscodes commented Dec 22, 2025

Hi @kaplanelad,

I refactored the code and fixed the remaining issues.

@floscodes
Copy link
Author

Hi @kaplanelad,

is it ok like this?

@kaplanelad
Copy link
Contributor

Hi @kaplanelad,

is it ok like this?

looks better. thanks.
can you check the ci?

@floscodes floscodes requested a review from kaplanelad January 2, 2026 09:33
@floscodes
Copy link
Author

Hi @kaplanelad,

just a quick heads-up: the CI is now passing and all tests are running successfully.

Thank you!

@kaplanelad
Copy link
Contributor

How group related to delete user task?

@floscodes
Copy link
Author

floscodes commented Jan 8, 2026

The delete-method from the ActiveModelTrait requires the groups table to exist:

let _deleted_user = user_to_delete
            .into_active_model()
            .delete(&app_context.db)
            .await
            .map_err(|err| {
                tracing::error!(message = err.to_string(), "could not delete user");
                Error::string(&format!("Failed to delete user. err: {err}",))
            })?;

otherwise the test fails with the following error:

Execution Error: error returned from database:
(code: 1) no such table: main.groups

That's why I had to add the groups migration file.

this seems to be caused by the following test:

tester.run_generate_migration(&vec![
    "CreateJoinTableUsersAndGroups",
    "count:int",
]);

@kaplanelad
Copy link
Contributor

From where the group came from?

@floscodes
Copy link
Author

From where the group came from?

That also caught my attention 🙂

I didn’t intentionally introduce groups on my side.

From what I could tell, the groups table is implicitly required by the ActiveModel::delete implementation. When the table is missing, the delete call fails at runtime, which is what caused the test to break.

I traced this back to the CreateJoinTableUsersAndGroups migration generated in the test. That seems to be where the dependency comes from, even though groups itself isn’t created explicitly beforehand.

The migration was added purely to make the test pass and avoid the runtime error — happy to change this if there’s a more intended approach 👍

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