-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
🔥(mork) simplify user deletion by removing username check
Since emails uniquely identify users, relying on both the username and email is unnecessary and adds complexity. This commit removes the use of username, simplifying the deletion process to rely solely on the email.
- Loading branch information
Showing
4 changed files
with
32 additions
and
67 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,23 +26,19 @@ def test_delete_inactive_users(edx_db, monkeypatch): | |
# 2 users that did not log in for 3 years | ||
EdxAuthUserFactory.create( | ||
last_login=Faker().date_time_between(end_date="-3y"), | ||
username="JohnDoe1", | ||
email="[email protected]", | ||
) | ||
EdxAuthUserFactory.create( | ||
last_login=Faker().date_time_between(end_date="-3y"), | ||
username="JohnDoe2", | ||
email="[email protected]", | ||
) | ||
# 2 users that logged in recently | ||
EdxAuthUserFactory.create( | ||
last_login=Faker().date_time_between(start_date="-3y"), | ||
username="JaneDah1", | ||
email="[email protected]", | ||
) | ||
EdxAuthUserFactory.create( | ||
last_login=Faker().date_time_between(start_date="-3y"), | ||
username="JaneDah2", | ||
email="[email protected]", | ||
) | ||
|
||
|
@@ -57,8 +53,8 @@ def test_delete_inactive_users(edx_db, monkeypatch): | |
|
||
mock_group.assert_called_once_with( | ||
[ | ||
mock_deletion_task.s(email="[email protected]", username="JohnDoe1"), | ||
mock_deletion_task.s(email="[email protected]", username="JohnDoe2"), | ||
mock_deletion_task.s(email="[email protected]"), | ||
mock_deletion_task.s(email="[email protected]"), | ||
] | ||
) | ||
|
||
|
@@ -68,12 +64,10 @@ def test_delete_inactive_users_with_batch_size(edx_db, monkeypatch): | |
# 2 users that did not log in for 3 years | ||
EdxAuthUserFactory.create( | ||
last_login=Faker().date_time_between(end_date="-3y"), | ||
username="JohnDoe1", | ||
email="[email protected]", | ||
) | ||
EdxAuthUserFactory.create( | ||
last_login=Faker().date_time_between(end_date="-3y"), | ||
username="JohnDoe2", | ||
email="[email protected]", | ||
) | ||
|
||
|
@@ -93,17 +87,13 @@ def test_delete_inactive_users_with_batch_size(edx_db, monkeypatch): | |
[ | ||
call( | ||
[ | ||
mock_deletion_task.s( | ||
email="[email protected]", username="JohnDoe1" | ||
), | ||
mock_deletion_task.s(email="[email protected]"), | ||
] | ||
), | ||
call().delay(), | ||
call( | ||
[ | ||
mock_deletion_task.s( | ||
email="[email protected]", username="JohnDoe2" | ||
), | ||
mock_deletion_task.s(email="[email protected]"), | ||
] | ||
), | ||
call().delay(), | ||
|
@@ -120,10 +110,9 @@ def test_deletion_task(monkeypatch): | |
"mork.celery.deletion_tasks.delete_email_status", mock_delete_email_status | ||
) | ||
email = "[email protected]" | ||
username = "JohnDoe" | ||
deletion_task(email, username) | ||
deletion_task(email) | ||
|
||
mock_delete_user.assert_called_once_with(email, username) | ||
mock_delete_user.assert_called_once_with(email) | ||
mock_delete_email_status.assert_called_once_with(email) | ||
|
||
|
||
|
@@ -136,46 +125,42 @@ def mock_delete(*args): | |
monkeypatch.setattr("mork.celery.deletion_tasks.delete_user", mock_delete) | ||
|
||
with pytest.raises(UserDeleteError, match="An error occurred"): | ||
deletion_task("[email protected]", "JohnDoe") | ||
deletion_task("[email protected]") | ||
|
||
|
||
def test_delete_user(edx_db, monkeypatch): | ||
"""Test the `delete_user` function.""" | ||
EdxAuthUserFactory._meta.sqlalchemy_session = edx_db.session | ||
EdxAuthUserFactory.create(username="JohnDoe1", email="[email protected]") | ||
EdxAuthUserFactory.create(username="JohnDoe2", email="[email protected]") | ||
EdxAuthUserFactory.create(email="[email protected]") | ||
EdxAuthUserFactory.create(email="[email protected]") | ||
|
||
monkeypatch.setattr("mork.celery.deletion_tasks.OpenEdxDB", lambda *args: edx_db) | ||
|
||
assert crud.get_user( | ||
edx_db.session, | ||
username="JohnDoe1", | ||
email="[email protected]", | ||
) | ||
assert crud.get_user( | ||
edx_db.session, | ||
username="JohnDoe2", | ||
email="[email protected]", | ||
) | ||
|
||
delete_user(email="[email protected]", username="JohnDoe1") | ||
delete_user(email="[email protected]") | ||
|
||
assert not crud.get_user( | ||
edx_db.session, | ||
username="JohnDoe1", | ||
email="[email protected]", | ||
) | ||
assert crud.get_user( | ||
edx_db.session, | ||
username="JohnDoe2", | ||
email="[email protected]", | ||
) | ||
|
||
|
||
def test_delete_user_with_failure(edx_db, monkeypatch): | ||
"""Test the `delete_user` function with a commit failure.""" | ||
EdxAuthUserFactory._meta.sqlalchemy_session = edx_db.session | ||
EdxAuthUserFactory.create(username="JohnDoe1", email="[email protected]") | ||
EdxAuthUserFactory.create(email="[email protected]") | ||
|
||
def mock_session_commit(): | ||
raise SQLAlchemyError("An error occurred") | ||
|
@@ -184,7 +169,7 @@ def mock_session_commit(): | |
monkeypatch.setattr("mork.celery.deletion_tasks.OpenEdxDB", lambda *args: edx_db) | ||
|
||
with pytest.raises(UserDeleteError, match="Failed to delete user."): | ||
delete_user(email="[email protected]", username="JohnDoe1") | ||
delete_user(email="[email protected]") | ||
|
||
|
||
def test_delete_email_status(db_session, monkeypatch): | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -129,38 +129,31 @@ def test_edx_crud_get_inactive_users_slice_empty(edx_db): | |
def test_edx_crud_get_user_missing(edx_db): | ||
"""Test the `get_user` method with missing user in the database.""" | ||
|
||
user = crud.get_user( | ||
session=edx_db.session, email="[email protected]", username="john_doe" | ||
) | ||
user = crud.get_user(session=edx_db.session, email="[email protected]") | ||
assert user is None | ||
|
||
|
||
def test_edx_crud_get_user(edx_db): | ||
"""Test the `get_user` method.""" | ||
email = "[email protected]" | ||
username = "john_doe" | ||
|
||
EdxAuthUserFactory.create_batch(1, email=email, username=username) | ||
EdxAuthUserFactory.create_batch(1, email=email) | ||
|
||
user = crud.get_user(session=edx_db.session, email=email, username=username) | ||
user = crud.get_user(session=edx_db.session, email=email) | ||
assert user.email == email | ||
assert user.username == username | ||
|
||
|
||
def test_edx_crud_delete_user_missing(edx_db): | ||
"""Test the `delete_user` method with missing user in the database.""" | ||
|
||
with pytest.raises(UserDeleteError, match="User to delete does not exist"): | ||
crud.delete_user( | ||
edx_db.session, email="[email protected]", username="john_doe" | ||
) | ||
crud.delete_user(edx_db.session, email="[email protected]") | ||
|
||
|
||
def test_edx_crud_delete_user(edx_db): | ||
"""Test the `delete_user` method.""" | ||
email = "[email protected]" | ||
username = "john_doe" | ||
EdxAuthUserFactory.create_batch(1, email=email, username=username) | ||
EdxAuthUserFactory.create_batch(1, email=email) | ||
EdxStudentCourseenrollmentallowedFactory.create_batch(3, email=email) | ||
|
||
# Get all related tables that have foreign key constraints on the parent table | ||
|
@@ -203,7 +196,7 @@ def test_edx_crud_delete_user(edx_db): | |
table = Base.metadata.tables[table_name] | ||
assert edx_db.session.query(table).count() > 0 | ||
|
||
crud.delete_user(edx_db.session, email="[email protected]", username="john_doe") | ||
crud.delete_user(edx_db.session, email="[email protected]") | ||
|
||
# Ensure the parent table is empty | ||
assert edx_db.session.query(AuthUser).count() == 0 | ||
|
@@ -219,7 +212,6 @@ def test_edx_crud_delete_user_protected_table(edx_db): | |
EdxAuthUserFactory.create_batch( | ||
1, | ||
email="[email protected]", | ||
username="john_doe", | ||
with_protected_tables=True, | ||
) | ||
|
||
|
@@ -243,9 +235,7 @@ def test_edx_crud_delete_user_protected_table(edx_db): | |
UserProtectedDeleteError, | ||
match="User is linked to a protected table and cannot be deleted", | ||
): | ||
crud.delete_user( | ||
edx_db.session, email="[email protected]", username="john_doe" | ||
) | ||
crud.delete_user(edx_db.session, email="[email protected]") | ||
|
||
# Ensure the parent table is empty | ||
assert edx_db.session.query(AuthUser).count() > 0 | ||
|