diff --git a/src/app/mork/celery/deletion_tasks.py b/src/app/mork/celery/deletion_tasks.py index ac35803..24d6b42 100644 --- a/src/app/mork/celery/deletion_tasks.py +++ b/src/app/mork/celery/deletion_tasks.py @@ -31,9 +31,7 @@ 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() @@ -41,10 +39,10 @@ def delete_inactive_users(): 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() diff --git a/src/app/mork/edx/crud.py b/src/app/mork/edx/crud.py index 88fd6d5..494a644 100644 --- a/src/app/mork/edx/crud.py +++ b/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=}") diff --git a/src/app/mork/tests/celery/test_deletion_task.py b/src/app/mork/tests/celery/test_deletion_task.py index 3bfd07e..b37618f 100644 --- a/src/app/mork/tests/celery/test_deletion_task.py +++ b/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="johndoe1@example.com", ) EdxAuthUserFactory.create( last_login=Faker().date_time_between(end_date="-3y"), - username="JohnDoe2", email="johndoe2@example.com", ) # 2 users that logged in recently EdxAuthUserFactory.create( last_login=Faker().date_time_between(start_date="-3y"), - username="JaneDah1", email="janedah1@example.com", ) EdxAuthUserFactory.create( last_login=Faker().date_time_between(start_date="-3y"), - username="JaneDah2", email="janedah2@example.com", ) @@ -57,8 +53,8 @@ def test_delete_inactive_users(edx_db, monkeypatch): mock_group.assert_called_once_with( [ - mock_deletion_task.s(email="johndoe1@example.com", username="JohnDoe1"), - mock_deletion_task.s(email="johndoe2@example.com", username="JohnDoe2"), + mock_deletion_task.s(email="johndoe1@example.com"), + mock_deletion_task.s(email="johndoe2@example.com"), ] ) @@ -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="johndoe1@example.com", ) EdxAuthUserFactory.create( last_login=Faker().date_time_between(end_date="-3y"), - username="JohnDoe2", email="johndoe2@example.com", ) @@ -93,17 +87,13 @@ def test_delete_inactive_users_with_batch_size(edx_db, monkeypatch): [ call( [ - mock_deletion_task.s( - email="johndoe1@example.com", username="JohnDoe1" - ), + mock_deletion_task.s(email="johndoe1@example.com"), ] ), call().delay(), call( [ - mock_deletion_task.s( - email="johndoe2@example.com", username="JohnDoe2" - ), + mock_deletion_task.s(email="johndoe2@example.com"), ] ), call().delay(), @@ -120,10 +110,9 @@ def test_deletion_task(monkeypatch): "mork.celery.deletion_tasks.delete_email_status", mock_delete_email_status ) email = "johndoe@example.com" - 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,38 +125,34 @@ def mock_delete(*args): monkeypatch.setattr("mork.celery.deletion_tasks.delete_user", mock_delete) with pytest.raises(UserDeleteError, match="An error occurred"): - deletion_task("johndoe@example.com", "JohnDoe") + deletion_task("johndoe@example.com") def test_delete_user(edx_db, monkeypatch): """Test the `delete_user` function.""" EdxAuthUserFactory._meta.sqlalchemy_session = edx_db.session - EdxAuthUserFactory.create(username="JohnDoe1", email="johndoe1@example.com") - EdxAuthUserFactory.create(username="JohnDoe2", email="johndoe2@example.com") + EdxAuthUserFactory.create(email="johndoe1@example.com") + EdxAuthUserFactory.create(email="johndoe2@example.com") monkeypatch.setattr("mork.celery.deletion_tasks.OpenEdxDB", lambda *args: edx_db) assert crud.get_user( edx_db.session, - username="JohnDoe1", email="johndoe1@example.com", ) assert crud.get_user( edx_db.session, - username="JohnDoe2", email="johndoe2@example.com", ) - delete_user(email="johndoe1@example.com", username="JohnDoe1") + delete_user(email="johndoe1@example.com") assert not crud.get_user( edx_db.session, - username="JohnDoe1", email="johndoe1@example.com", ) assert crud.get_user( edx_db.session, - username="JohnDoe2", email="johndoe2@example.com", ) @@ -175,7 +160,7 @@ def test_delete_user(edx_db, monkeypatch): 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="johndoe1@example.com") + EdxAuthUserFactory.create(email="johndoe1@example.com") 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="johndoe1@example.com", username="JohnDoe1") + delete_user(email="johndoe1@example.com") def test_delete_email_status(db_session, monkeypatch): diff --git a/src/app/mork/tests/edx/test_crud.py b/src/app/mork/tests/edx/test_crud.py index 7d176ec..ec8b9e9 100644 --- a/src/app/mork/tests/edx/test_crud.py +++ b/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="john_doe@example.com", username="john_doe" - ) + user = crud.get_user(session=edx_db.session, email="john_doe@example.com") assert user is None def test_edx_crud_get_user(edx_db): """Test the `get_user` method.""" email = "john_doe@example.com" - 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="john_doe@example.com", username="john_doe" - ) + crud.delete_user(edx_db.session, email="john_doe@example.com") def test_edx_crud_delete_user(edx_db): """Test the `delete_user` method.""" email = "john_doe@example.com" - 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="john_doe@example.com", username="john_doe") + crud.delete_user(edx_db.session, email="john_doe@example.com") # 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="john_doe@example.com", - 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="john_doe@example.com", username="john_doe" - ) + crud.delete_user(edx_db.session, email="john_doe@example.com") # Ensure the parent table is empty assert edx_db.session.query(AuthUser).count() > 0