🔥(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.

src/app/mork/celery/deletion_tasks.py

@@ -31,20 +31,18 @@ def delete_inactive_users():
             offset=batch_offset,
             limit=settings.EDX_QUERY_BATCH_SIZE,
         )
-        delete_group = group(
-            [deletion_task.s(user.email, user.username) for user in inactive_users]
-        )
+        delete_group = group([deletion_task.s(user.email) for user in inactive_users])
         delete_group.delay()
 
 
 @app.task(
     bind=True,
     retry_kwargs={"max_retries": settings.DELETE_MAX_RETRIES},
 )
-def deletion_task(self, email: str, username: str):
+def deletion_task(self, email: str):
     """Celery task that delete a specified user."""
     try:
-        delete_user(email, username)
+        delete_user(email)
     except UserDeleteError as exc:
         logger.exception(exc)
         raise self.retry(exc=exc) from exc
@@ -53,17 +51,17 @@ def deletion_task(self, email: str, username: str):
     delete_email_status(email)
 
 
-def delete_user(email, username):
+def delete_user(email):
     """Delete user from edX database."""
     db = OpenEdxDB()
 
     # Delete user from edX database
-    crud.delete_user(db.session, email=email, username=username)
+    crud.delete_user(db.session, email=email)
     try:
         db.session.commit()
     except SQLAlchemyError as exc:
         db.session.rollback()
-        logger.error(f"Failed to delete user {username} with {email=}: {exc}")
+        logger.error(f"Failed to delete user with {email=}: {exc}")
         raise UserDeleteError("Failed to delete user.") from exc
     finally:
         db.session.close()

src/app/mork/edx/crud.py

@@ -78,17 +78,14 @@ def get_inactive_users(
     return session.scalars(query).unique().all()
 
 
-def get_user(session: Session, email: str, username: str):
-    """Get a user entry based on the provided email and username.
+def get_user(session: Session, email: str):
+    """Get a user entry based on the provided email.
 
     Parameters:
     session (Session): SQLAlchemy session object.
     email (str): The email of the user to get.
-    username (str): The username of the user to get.
     """
-    query = select(AuthUser).where(
-        AuthUser.email == email, AuthUser.username == username
-    )
+    query = select(AuthUser).where(AuthUser.email == email)
     return session.execute(query).scalar()
 
 
@@ -115,21 +112,16 @@ def _has_protected_children(session: Session, user_id) -> bool:
     return bool(result)
 
 
-def delete_user(session: Session, email: str, username: str) -> None:
+def delete_user(session: Session, email: str) -> None:
     """Delete a user entry based on the provided email and username.
 
     Parameters:
     session (Session): SQLAlchemy session object.
     email (str): The email of the user to delete.
-    username (str): The username of the user to delete.
     """
-    user_to_delete = (
-        session.query(AuthUser)
-        .filter(AuthUser.email == email, AuthUser.username == username)
-        .first()
-    )
+    user_to_delete = session.query(AuthUser).filter(AuthUser.email == email).first()
     if not user_to_delete:
-        logger.error(f"No user found with {username=} {email=} for deletion")
+        logger.error(f"No user found with {email=} for deletion")
         raise UserDeleteError("User to delete does not exist")
 
     if _has_protected_children(session, user_to_delete.id):
@@ -147,4 +139,4 @@ def delete_user(session: Session, email: str, username: str) -> None:
     # Delete user from auth_user table and all its children
     session.delete(user_to_delete)
 
-    logger.info(f"Deleting user {username=} {email=}")
+    logger.info(f"Deleting user {email=}")

src/app/mork/tests/celery/test_deletion_task.py

@@ -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):

src/app/mork/tests/edx/test_crud.py

@@ -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