diff --git a/src/app/mork/api/models.py b/src/app/mork/api/models.py index 2142fe6..7d67524 100644 --- a/src/app/mork/api/models.py +++ b/src/app/mork/api/models.py @@ -4,8 +4,8 @@ from pydantic import BaseModel -from mork.celery.deletion_tasks import delete_inactive_users -from mork.celery.emailing_tasks import warn_inactive_users +from mork.celery.tasks.deletion import delete_inactive_users +from mork.celery.tasks.emailing import warn_inactive_users @unique @@ -25,8 +25,8 @@ class TaskStatus(str, Enum): class TaskType(str, Enum): """Possible task types.""" - EMAILING = "emailing" - DELETION = "deletion" + EMAIL_INACTIVE_USERS = "email_inactive_users" + DELETE_INACTIVE_USERS = "delete_inactive_users" class TaskCreate(BaseModel): @@ -43,6 +43,6 @@ class TaskResponse(BaseModel): TASK_TYPE_TO_FUNC = { - TaskType.EMAILING: warn_inactive_users, - TaskType.DELETION: delete_inactive_users, + TaskType.EMAIL_INACTIVE_USERS: warn_inactive_users, + TaskType.DELETE_INACTIVE_USERS: delete_inactive_users, } diff --git a/src/app/mork/celery/celery_app.py b/src/app/mork/celery/celery_app.py index eef5872..abf2d6c 100644 --- a/src/app/mork/celery/celery_app.py +++ b/src/app/mork/celery/celery_app.py @@ -3,7 +3,7 @@ from celery import Celery app = Celery( - "mork", include=["mork.celery.deletion_tasks", "mork.celery.emailing_tasks"] + "mork", include=["mork.celery.tasks.deletion", "mork.celery.tasks.emailing"] ) # Using a string here means the worker doesn't have to serialize diff --git a/src/app/mork/celery/tasks/__init__.py b/src/app/mork/celery/tasks/__init__.py new file mode 100644 index 0000000..6e03199 --- /dev/null +++ b/src/app/mork/celery/tasks/__init__.py @@ -0,0 +1 @@ +# noqa: D104 diff --git a/src/app/mork/celery/deletion_tasks.py b/src/app/mork/celery/tasks/deletion.py similarity index 92% rename from src/app/mork/celery/deletion_tasks.py rename to src/app/mork/celery/tasks/deletion.py index 24d6b42..8a8d641 100644 --- a/src/app/mork/celery/deletion_tasks.py +++ b/src/app/mork/celery/tasks/deletion.py @@ -31,7 +31,7 @@ def delete_inactive_users(): offset=batch_offset, limit=settings.EDX_QUERY_BATCH_SIZE, ) - delete_group = group([deletion_task.s(user.email) for user in inactive_users]) + delete_group = group([delete_user.s(user.email) for user in inactive_users]) delete_group.delay() @@ -39,10 +39,10 @@ def delete_inactive_users(): bind=True, retry_kwargs={"max_retries": settings.DELETE_MAX_RETRIES}, ) -def deletion_task(self, email: str): +def delete_user(self, email: str): """Celery task that delete a specified user.""" try: - delete_user(email) + delete_user_from_db(email) except UserDeleteError as exc: logger.exception(exc) raise self.retry(exc=exc) from exc @@ -51,7 +51,7 @@ def deletion_task(self, email: str): delete_email_status(email) -def delete_user(email): +def delete_user_from_db(email): """Delete user from edX database.""" db = OpenEdxDB() diff --git a/src/app/mork/celery/emailing_tasks.py b/src/app/mork/celery/tasks/emailing.py similarity index 91% rename from src/app/mork/celery/emailing_tasks.py rename to src/app/mork/celery/tasks/emailing.py index a239cf8..c968c56 100644 --- a/src/app/mork/celery/emailing_tasks.py +++ b/src/app/mork/celery/tasks/emailing.py @@ -34,7 +34,7 @@ def warn_inactive_users(): limit=settings.EDX_QUERY_BATCH_SIZE, ) send_email_group = group( - [send_email_task.s(user.email, user.username) for user in inactive_users] + [warn_user.s(user.email, user.username) for user in inactive_users] ) send_email_group.delay() @@ -44,8 +44,8 @@ def warn_inactive_users(): rate_limit=settings.EMAIL_RATE_LIMIT, retry_kwargs={"max_retries": settings.EMAIL_MAX_RETRIES}, ) -def send_email_task(self, email_address: str, username: str): - """Celery task that sends an email to the specified user.""" +def warn_user(self, email_address: str, username: str): + """Celery task that warns the specified user by sending an email.""" # Check that user has not already received a warning email if check_email_already_sent(email_address): raise EmailAlreadySent("An email has already been sent to this user") diff --git a/src/app/mork/tests/api/test_tasks.py b/src/app/mork/tests/api/test_tasks.py index 70d717f..323aa46 100644 --- a/src/app/mork/tests/api/test_tasks.py +++ b/src/app/mork/tests/api/test_tasks.py @@ -17,7 +17,7 @@ async def test_tasks_auth(http_client: AsyncClient): @pytest.mark.anyio -@pytest.mark.parametrize("task_type", ["emailing", "deletion"]) +@pytest.mark.parametrize("task_type", ["email_inactive_users", "delete_inactive_users"]) async def test_create_task( http_client: AsyncClient, auth_headers: dict, task_type: str ): @@ -74,7 +74,10 @@ async def test_get_available_tasks(http_client: AsyncClient, auth_headers: dict) response_data = response.json() assert response.status_code == 200 assert response.headers["allow"] == "POST" - assert sorted(response_data.get("task_types")) == ["deletion", "emailing"] + assert sorted(response_data.get("task_types")) == [ + "delete_inactive_users", + "email_inactive_users", + ] @pytest.mark.anyio diff --git a/src/app/mork/tests/celery/test_deletion_task.py b/src/app/mork/tests/celery/test_deletion_task.py index b37618f..671dd08 100644 --- a/src/app/mork/tests/celery/test_deletion_task.py +++ b/src/app/mork/tests/celery/test_deletion_task.py @@ -8,11 +8,11 @@ from sqlalchemy import select from sqlalchemy.exc import SQLAlchemyError -from mork.celery.deletion_tasks import ( +from mork.celery.tasks.deletion import ( delete_email_status, delete_inactive_users, delete_user, - deletion_task, + delete_user_from_db, ) from mork.edx import crud from mork.edx.factories.auth import EdxAuthUserFactory @@ -42,19 +42,19 @@ def test_delete_inactive_users(edx_db, monkeypatch): email="janedah2@example.com", ) - monkeypatch.setattr("mork.celery.deletion_tasks.OpenEdxDB", lambda *args: edx_db) + monkeypatch.setattr("mork.celery.tasks.deletion.OpenEdxDB", lambda *args: edx_db) mock_group = Mock() - monkeypatch.setattr("mork.celery.deletion_tasks.group", mock_group) - mock_deletion_task = Mock() - monkeypatch.setattr("mork.celery.deletion_tasks.deletion_task", mock_deletion_task) + monkeypatch.setattr("mork.celery.tasks.deletion.group", mock_group) + mock_delete_user = Mock() + monkeypatch.setattr("mork.celery.tasks.deletion.delete_user", mock_delete_user) delete_inactive_users() mock_group.assert_called_once_with( [ - mock_deletion_task.s(email="johndoe1@example.com"), - mock_deletion_task.s(email="johndoe2@example.com"), + mock_delete_user.s(email="johndoe1@example.com"), + mock_delete_user.s(email="johndoe2@example.com"), ] ) @@ -71,15 +71,15 @@ def test_delete_inactive_users_with_batch_size(edx_db, monkeypatch): email="johndoe2@example.com", ) - monkeypatch.setattr("mork.celery.deletion_tasks.OpenEdxDB", lambda *args: edx_db) + monkeypatch.setattr("mork.celery.tasks.deletion.OpenEdxDB", lambda *args: edx_db) mock_group = Mock() - monkeypatch.setattr("mork.celery.deletion_tasks.group", mock_group) - mock_deletion_task = Mock() - monkeypatch.setattr("mork.celery.deletion_tasks.deletion_task", mock_deletion_task) + monkeypatch.setattr("mork.celery.tasks.deletion.group", mock_group) + mock_delete_user = Mock() + monkeypatch.setattr("mork.celery.tasks.deletion.delete_user", mock_delete_user) # Set batch size to 1 - monkeypatch.setattr("mork.celery.deletion_tasks.settings.EDX_QUERY_BATCH_SIZE", 1) + monkeypatch.setattr("mork.celery.tasks.deletion.settings.EDX_QUERY_BATCH_SIZE", 1) delete_inactive_users() @@ -87,13 +87,13 @@ def test_delete_inactive_users_with_batch_size(edx_db, monkeypatch): [ call( [ - mock_deletion_task.s(email="johndoe1@example.com"), + mock_delete_user.s(email="johndoe1@example.com"), ] ), call().delay(), call( [ - mock_deletion_task.s(email="johndoe2@example.com"), + mock_delete_user.s(email="johndoe2@example.com"), ] ), call().delay(), @@ -101,40 +101,44 @@ def test_delete_inactive_users_with_batch_size(edx_db, monkeypatch): ) -def test_deletion_task(monkeypatch): - """Test the `deletion_task` function.""" - mock_delete_user = Mock() - monkeypatch.setattr("mork.celery.deletion_tasks.delete_user", mock_delete_user) +def test_delete_user(monkeypatch): + """Test the `delete_user` function.""" + mock_delete_user_from_db = Mock() + monkeypatch.setattr( + "mork.celery.tasks.deletion.delete_user_from_db", mock_delete_user_from_db + ) mock_delete_email_status = Mock() monkeypatch.setattr( - "mork.celery.deletion_tasks.delete_email_status", mock_delete_email_status + "mork.celery.tasks.deletion.delete_email_status", mock_delete_email_status ) email = "johndoe@example.com" - deletion_task(email) + delete_user(email) - mock_delete_user.assert_called_once_with(email) + mock_delete_user_from_db.assert_called_once_with(email) mock_delete_email_status.assert_called_once_with(email) -def test_deletion_task_delete_failure(monkeypatch): - """Test the `deletion_task` function with a delete failure.""" +def test_delete_user_failure(monkeypatch): + """Test the `delete_user` function with a delete failure.""" - def mock_delete(*args): + def mock_delete_user_from_db(*args): raise UserDeleteError("An error occurred") - monkeypatch.setattr("mork.celery.deletion_tasks.delete_user", mock_delete) + monkeypatch.setattr( + "mork.celery.tasks.deletion.delete_user_from_db", mock_delete_user_from_db + ) with pytest.raises(UserDeleteError, match="An error occurred"): - deletion_task("johndoe@example.com") + delete_user("johndoe@example.com") -def test_delete_user(edx_db, monkeypatch): - """Test the `delete_user` function.""" +def test_delete_user_from_db(edx_db, monkeypatch): + """Test the `delete_user_from_db` function.""" EdxAuthUserFactory._meta.sqlalchemy_session = edx_db.session EdxAuthUserFactory.create(email="johndoe1@example.com") EdxAuthUserFactory.create(email="johndoe2@example.com") - monkeypatch.setattr("mork.celery.deletion_tasks.OpenEdxDB", lambda *args: edx_db) + monkeypatch.setattr("mork.celery.tasks.deletion.OpenEdxDB", lambda *args: edx_db) assert crud.get_user( edx_db.session, @@ -145,7 +149,7 @@ def test_delete_user(edx_db, monkeypatch): email="johndoe2@example.com", ) - delete_user(email="johndoe1@example.com") + delete_user_from_db(email="johndoe1@example.com") assert not crud.get_user( edx_db.session, @@ -157,8 +161,8 @@ 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.""" +def test_delete_user_from_db_with_failure(edx_db, monkeypatch): + """Test the `delete_user_from_db` function with a commit failure.""" EdxAuthUserFactory._meta.sqlalchemy_session = edx_db.session EdxAuthUserFactory.create(email="johndoe1@example.com") @@ -166,10 +170,10 @@ def mock_session_commit(): raise SQLAlchemyError("An error occurred") edx_db.session.commit = mock_session_commit - monkeypatch.setattr("mork.celery.deletion_tasks.OpenEdxDB", lambda *args: edx_db) + monkeypatch.setattr("mork.celery.tasks.deletion.OpenEdxDB", lambda *args: edx_db) with pytest.raises(UserDeleteError, match="Failed to delete user."): - delete_user(email="johndoe1@example.com") + delete_user_from_db(email="johndoe1@example.com") def test_delete_email_status(db_session, monkeypatch): @@ -179,7 +183,7 @@ class MockMorkDB: session = db_session EmailStatusFactory._meta.sqlalchemy_session = db_session - monkeypatch.setattr("mork.celery.deletion_tasks.MorkDB", MockMorkDB) + monkeypatch.setattr("mork.celery.tasks.deletion.MorkDB", MockMorkDB) email = "johndoe1@example.com" EmailStatusFactory.create(email=email) @@ -203,7 +207,7 @@ class MockMorkDB: session = db_session EmailStatusFactory._meta.sqlalchemy_session = db_session - monkeypatch.setattr("mork.celery.deletion_tasks.MorkDB", MockMorkDB) + monkeypatch.setattr("mork.celery.tasks.deletion.MorkDB", MockMorkDB) email = "johndoe1@example.com" @@ -212,7 +216,7 @@ class MockMorkDB: delete_email_status(email) assert ( - "mork.celery.deletion_tasks", + "mork.celery.tasks.deletion", logging.WARNING, "Mork DB - No user found with email='johndoe1@example.com' for deletion", ) in caplog.record_tuples @@ -230,7 +234,7 @@ class MockMorkDB: session = db_session EmailStatusFactory._meta.sqlalchemy_session = db_session - monkeypatch.setattr("mork.celery.deletion_tasks.MorkDB", MockMorkDB) + monkeypatch.setattr("mork.celery.tasks.deletion.MorkDB", MockMorkDB) email = "johndoe1@example.com" EmailStatusFactory.create(email=email) @@ -240,7 +244,7 @@ class MockMorkDB: delete_email_status(email) assert ( - "mork.celery.deletion_tasks", + "mork.celery.tasks.deletion", logging.ERROR, "Mork DB - Failed to delete user with email='johndoe1@example.com':" " An error occurred", diff --git a/src/app/mork/tests/celery/test_emailing_tasks.py b/src/app/mork/tests/celery/test_emailing_tasks.py index 8cefdc6..481daa8 100644 --- a/src/app/mork/tests/celery/test_emailing_tasks.py +++ b/src/app/mork/tests/celery/test_emailing_tasks.py @@ -5,11 +5,11 @@ import pytest from faker import Faker -from mork.celery.emailing_tasks import ( +from mork.celery.tasks.emailing import ( check_email_already_sent, mark_email_status, - send_email_task, warn_inactive_users, + warn_user, ) from mork.edx.factories.auth import EdxAuthUserFactory from mork.exceptions import EmailAlreadySent, EmailSendError @@ -41,21 +41,19 @@ def test_warn_inactive_users(edx_db, monkeypatch): email="janedah2@example.com", ) - monkeypatch.setattr("mork.celery.emailing_tasks.OpenEdxDB", lambda *args: edx_db) + monkeypatch.setattr("mork.celery.tasks.emailing.OpenEdxDB", lambda *args: edx_db) mock_group = Mock() - monkeypatch.setattr("mork.celery.emailing_tasks.group", mock_group) - mock_send_email_task = Mock() - monkeypatch.setattr( - "mork.celery.emailing_tasks.send_email_task", mock_send_email_task - ) + monkeypatch.setattr("mork.celery.tasks.emailing.group", mock_group) + mock_warn_user = Mock() + monkeypatch.setattr("mork.celery.tasks.emailing.warn_user", mock_warn_user) warn_inactive_users() mock_group.assert_called_once_with( [ - mock_send_email_task.s(email="johndoe1@example.com", username="JohnDoe1"), - mock_send_email_task.s(email="johndoe2@example.com", username="JohnDoe2"), + mock_warn_user.s(email="johndoe1@example.com", username="JohnDoe1"), + mock_warn_user.s(email="johndoe2@example.com", username="JohnDoe2"), ] ) @@ -74,17 +72,15 @@ def test_warn_inactive_users_with_batch_size(edx_db, monkeypatch): email="johndoe2@example.com", ) - monkeypatch.setattr("mork.celery.emailing_tasks.OpenEdxDB", lambda *args: edx_db) + monkeypatch.setattr("mork.celery.tasks.emailing.OpenEdxDB", lambda *args: edx_db) mock_group = Mock() - monkeypatch.setattr("mork.celery.emailing_tasks.group", mock_group) - mock_send_email_task = Mock() - monkeypatch.setattr( - "mork.celery.emailing_tasks.send_email_task", mock_send_email_task - ) + monkeypatch.setattr("mork.celery.tasks.emailing.group", mock_group) + mock_warn_user = Mock() + monkeypatch.setattr("mork.celery.tasks.emailing.warn_user", mock_warn_user) # Set batch size to 1 - monkeypatch.setattr("mork.celery.emailing_tasks.settings.EDX_QUERY_BATCH_SIZE", 1) + monkeypatch.setattr("mork.celery.tasks.emailing.settings.EDX_QUERY_BATCH_SIZE", 1) warn_inactive_users() @@ -92,17 +88,13 @@ def test_warn_inactive_users_with_batch_size(edx_db, monkeypatch): [ call( [ - mock_send_email_task.s( - email="johndoe1@example.com", username="JohnDoe1" - ), + mock_warn_user.s(email="johndoe1@example.com", username="JohnDoe1"), ] ), call().delay(), call( [ - mock_send_email_task.s( - email="johndoe2@example.com", username="JohnDoe2" - ), + mock_warn_user.s(email="johndoe2@example.com", username="JohnDoe2"), ] ), call().delay(), @@ -110,42 +102,42 @@ def test_warn_inactive_users_with_batch_size(edx_db, monkeypatch): ) -def test_send_email_task(monkeypatch): - """Test the `send_email_task` function.""" +def test_warn_user(monkeypatch): + """Test the `warn_user` function.""" monkeypatch.setattr( - "mork.celery.emailing_tasks.check_email_already_sent", lambda x: False + "mork.celery.tasks.emailing.check_email_already_sent", lambda x: False ) - monkeypatch.setattr("mork.celery.emailing_tasks.send_email", lambda *args: None) - monkeypatch.setattr("mork.celery.emailing_tasks.mark_email_status", lambda x: None) + monkeypatch.setattr("mork.celery.tasks.emailing.warn_user", lambda *args: None) + monkeypatch.setattr("mork.celery.tasks.emailing.mark_email_status", lambda x: None) - send_email_task("johndoe@example.com", "JohnDoe") + warn_user("johndoe@example.com", "JohnDoe") -def test_send_email_task_already_sent(monkeypatch): - """Test the `send_email_task` function when email has already been sent.""" +def test_warn_user_already_sent(monkeypatch): + """Test the `warn_user` function when email has already been sent.""" monkeypatch.setattr( - "mork.celery.emailing_tasks.check_email_already_sent", lambda x: True + "mork.celery.tasks.emailing.check_email_already_sent", lambda x: True ) with pytest.raises( EmailAlreadySent, match="An email has already been sent to this user" ): - send_email_task("johndoe@example.com", "JohnDoe") + warn_user("johndoe@example.com", "JohnDoe") -def test_send_email_task_sending_failure(monkeypatch): - """Test the `send_email_task` function with email sending failure.""" +def test_warn_user_sending_failure(monkeypatch): + """Test the `warn_user` function with email sending failure.""" monkeypatch.setattr( - "mork.celery.emailing_tasks.check_email_already_sent", lambda x: False + "mork.celery.tasks.emailing.check_email_already_sent", lambda x: False ) def mock_send(*args): raise EmailSendError("An error occurred") - monkeypatch.setattr("mork.celery.emailing_tasks.send_email", mock_send) + monkeypatch.setattr("mork.celery.tasks.emailing.send_email", mock_send) with pytest.raises(EmailSendError, match="An error occurred"): - send_email_task("johndoe@example.com", "JohnDoe") + warn_user("johndoe@example.com", "JohnDoe") def test_check_email_already_sent(monkeypatch, db_session): @@ -156,7 +148,7 @@ class MockMorkDB: session = db_session EmailStatusFactory._meta.sqlalchemy_session = db_session - monkeypatch.setattr("mork.celery.emailing_tasks.MorkDB", MockMorkDB) + monkeypatch.setattr("mork.celery.tasks.emailing.MorkDB", MockMorkDB) EmailStatusFactory.create_batch(3) assert not check_email_already_sent(email_address) @@ -172,7 +164,7 @@ class MockMorkDB: session = db_session EmailStatusFactory._meta.sqlalchemy_session = db_session - monkeypatch.setattr("mork.celery.emailing_tasks.MorkDB", MockMorkDB) + monkeypatch.setattr("mork.celery.tasks.emailing.MorkDB", MockMorkDB) # Write new email status entry new_email = "test_email@example.com"