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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deleting denied local_users older than a week. Fixes #4434 #4448

Merged
merged 5 commits into from Feb 15, 2024

Conversation

dessalines
Copy link
Member

No description provided.

src/scheduled_tasks.rs Outdated Show resolved Hide resolved
.filter(local_user::id.eq_any(old_denied_registrations))
.filter(local_user::accepted_application.eq(false));

diesel::delete(users).execute(conn).await
Copy link
Member

@Nutomic Nutomic Feb 14, 2024

Choose a reason for hiding this comment

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

I think you also need to explicitly delete from person table. Maybe add a test case to create user, delete with this function and then create new user with same name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Deleting the person should automaticaly delete the local user, so this could be done to have only 1 statement:

let users = person::table
    .filter(exists(local_user::table
        .filter(local_user::id.eq_any(old_denied_registrations))
        .filter(not(local_user::accepted_application))
    ));

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops good call. I'll do that now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed up changes. I don't think you can use exists(... on another table when it doesn't know what columns to check, so I used person::id.eq_any

@Nutomic
Copy link
Member

Nutomic commented Feb 15, 2024

Looks good, will give @dullbananas a chance to review before merging.

@dullbananas
Copy link
Collaborator

Looks good to me

@dessalines dessalines merged commit 890565c into main Feb 15, 2024
2 checks passed
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.

None yet

3 participants