From 830dd048c7eb3596bc6d2c27695f708443a95c12 Mon Sep 17 00:00:00 2001 From: Michael Huneke Date: Thu, 30 Jan 2025 17:43:12 -0500 Subject: [PATCH 01/20] Add saved searches to generate_notifications task --- .../notifications/generate_notifications.py | 79 ++++++++++++-- .../test_generate_notifications.py | 102 ++++++++++++++++++ 2 files changed, 170 insertions(+), 11 deletions(-) diff --git a/api/src/task/notifications/generate_notifications.py b/api/src/task/notifications/generate_notifications.py index 073100fd5..cb420eeed 100644 --- a/api/src/task/notifications/generate_notifications.py +++ b/api/src/task/notifications/generate_notifications.py @@ -8,7 +8,7 @@ import src.adapters.db as db import src.adapters.db.flask_db as flask_db from src.db.models.opportunity_models import OpportunityChangeAudit -from src.db.models.user_models import UserNotificationLog, UserSavedOpportunity +from src.db.models.user_models import UserNotificationLog, UserSavedOpportunity, UserSavedSearch from src.task.ecs_background_task import ecs_background_task from src.task.task import Task from src.task.task_blueprint import task_blueprint @@ -30,6 +30,7 @@ def run_notification_task(db_session: db.Session) -> None: class NotificationConstants: OPPORTUNITY_UPDATES = "opportunity_updates" + SEARCH_UPDATES = "search_updates" @dataclass @@ -37,8 +38,7 @@ class NotificationContainer: """Container for collecting notifications for a single user""" saved_opportunities: list[UserSavedOpportunity] = field(default_factory=list) - # TODO: Change from str to something else - updated_searches: list[str] = field(default_factory=list) + saved_searches: list[UserSavedSearch] = field(default_factory=list) class NotificationTask(Task): @@ -93,16 +93,54 @@ def _collect_opportunity_notifications(self) -> None: ) def _collect_search_notifications(self) -> None: - """Collect notifications for changed saved searches - To be implemented in future ticket - """ - logger.info("Search notification collection not yet implemented") - pass + """Collect notifications for changed saved searches""" + # Get all saved searches that haven't been checked since last notification + stmt = select(UserSavedSearch).where( + UserSavedSearch.last_notified_at < datetime_util.utcnow() + ) + saved_searches = self.db_session.execute(stmt).scalars() + + # Group searches by query to minimize search index calls + query_map: dict[str, list[UserSavedSearch]] = {} + for saved_search in saved_searches: + query_key = str(saved_search.search_query) + if query_key not in query_map: + query_map[query_key] = [] + query_map[query_key].append(saved_search) + + # For each unique query, check if results have changed + for _, searches in query_map.items(): + # TODO: Replace with actual search index query + # current_results = search_index.search(search_query) + current_results: set[int] = set() # Placeholder for actual search results + + for saved_search in searches: + previous_results = set(saved_search.searched_opportunity_ids) + if current_results != previous_results: + user_id = saved_search.user_id + if user_id not in self.user_notification_map: + self.user_notification_map[user_id] = NotificationContainer() + self.user_notification_map[user_id].saved_searches.append(saved_search) + + # Update the saved search with new results + saved_search.searched_opportunity_ids = list(current_results) + + logger.info(f"User notification map: {self.user_notification_map}") + logger.info( + "Collected search notifications", + extra={ + "user_count": len(self.user_notification_map), + "total_searches": sum( + len(container.saved_searches) + for container in self.user_notification_map.values() + ), + }, + ) def _send_notifications(self) -> None: """Send collected notifications to users""" for user_id, container in self.user_notification_map.items(): - if not container.saved_opportunities and not container.updated_searches: + if not container.saved_opportunities and not container.saved_searches: continue # TODO: Implement actual notification sending in future ticket @@ -111,7 +149,7 @@ def _send_notifications(self) -> None: extra={ "user_id": user_id, "opportunity_count": len(container.saved_opportunities), - "search_count": len(container.updated_searches), + "search_count": len(container.saved_searches), }, ) @@ -123,6 +161,14 @@ def _send_notifications(self) -> None: ) self.db_session.add(notification_log) + if container.saved_searches: + notification_log = UserNotificationLog( + user_id=user_id, + notification_reason=NotificationConstants.SEARCH_UPDATES, + notification_sent=True, + ) + self.db_session.add(notification_log) + # Update last_notified_at for all opportunities we just notified about opportunity_ids = [ saved_opp.opportunity_id for saved_opp in container.saved_opportunities @@ -136,7 +182,18 @@ def _send_notifications(self) -> None: .values(last_notified_at=datetime_util.utcnow()) ) + # Update last_notified_at for all searches we just notified about + if container.saved_searches: + search_ids = [ + saved_search.saved_search_id for saved_search in container.saved_searches + ] + self.db_session.execute( + update(UserSavedSearch) + .where(UserSavedSearch.saved_search_id.in_(search_ids)) + .values(last_notified_at=datetime_util.utcnow()) + ) + self.increment(self.Metrics.OPPORTUNITIES_TRACKED, len(container.saved_opportunities)) - self.increment(self.Metrics.SEARCHES_TRACKED, len(container.updated_searches)) + self.increment(self.Metrics.SEARCHES_TRACKED, len(container.saved_searches)) self.increment(self.Metrics.NOTIFICATIONS_SENT) self.increment(self.Metrics.USERS_NOTIFIED) diff --git a/api/tests/src/task/notifications/test_generate_notifications.py b/api/tests/src/task/notifications/test_generate_notifications.py index 525740637..22a008f14 100644 --- a/api/tests/src/task/notifications/test_generate_notifications.py +++ b/api/tests/src/task/notifications/test_generate_notifications.py @@ -5,6 +5,7 @@ import tests.src.db.models.factories as factories from src.db.models.user_models import UserNotificationLog from src.task.notifications.generate_notifications import NotificationConstants +from src.util import datetime_util @pytest.fixture @@ -130,3 +131,104 @@ def test_no_notification_log_when_no_updates( # Verify no notification log was created notification_logs = db_session.query(UserNotificationLog).all() assert len(notification_logs) == 0 + + +def test_search_notifications_cli( + cli_runner, db_session, enable_factory_create, user, caplog, clear_notification_logs +): + """Test that verifies we can collect and send search notifications via CLI""" + # Create a saved search that needs notification + saved_search = factories.UserSavedSearchFactory.create( + user=user, + search_query={"keywords": "test"}, + name="Test Search", + last_notified_at=datetime_util.utcnow() - timedelta(days=1), + searched_opportunity_ids=[1, 2, 3], + ) + + result = cli_runner.invoke(args=["task", "generate-notifications"]) + + assert result.exit_code == 0 + + # Verify expected log messages + assert "Collected search notifications" in caplog.text + assert "Would send notification to user" in caplog.text + + # Verify the log contains the correct metrics + log_records = [r for r in caplog.records if "Would send notification to user" in r.message] + assert len(log_records) == 1 + extra = log_records[0].__dict__ + assert extra["user_id"] == user.user_id + assert extra["opportunity_count"] == 0 + assert extra["search_count"] == 1 + + # Verify notification log was created + notification_logs = ( + db_session.query(UserNotificationLog) + .filter(UserNotificationLog.notification_reason == NotificationConstants.SEARCH_UPDATES) + .all() + ) + assert len(notification_logs) == 1 + + # Verify last_notified_at was updated + db_session.refresh(saved_search) + assert saved_search.last_notified_at > datetime_util.utcnow() - timedelta(minutes=1) + + +def test_combined_notifications_cli( + cli_runner, db_session, enable_factory_create, user, caplog, clear_notification_logs +): + """Test that verifies we can handle both opportunity and search notifications together""" + # Create a saved opportunity that needs notification + opportunity = factories.OpportunityFactory.create() + saved_opportunity = factories.UserSavedOpportunityFactory.create( + user=user, + opportunity=opportunity, + last_notified_at=opportunity.updated_at - timedelta(days=1), + ) + factories.OpportunityChangeAuditFactory.create( + opportunity=opportunity, + updated_at=saved_opportunity.last_notified_at + timedelta(minutes=1), + ) + + # Create a saved search that needs notification + saved_search = factories.UserSavedSearchFactory.create( + user=user, + search_query={"keywords": "test"}, + name="Test Search", + last_notified_at=datetime_util.utcnow() - timedelta(days=1), + searched_opportunity_ids=[1, 2, 3], + ) + + result = cli_runner.invoke(args=["task", "generate-notifications"]) + + assert result.exit_code == 0 + + # Verify expected log messages + assert "Collected opportunity notifications" in caplog.text + assert "Collected search notifications" in caplog.text + assert "Would send notification to user" in caplog.text + + # Verify the log contains the correct metrics + log_records = [r for r in caplog.records if "Would send notification to user" in r.message] + assert len(log_records) == 1 + extra = log_records[0].__dict__ + assert extra["user_id"] == user.user_id + assert extra["opportunity_count"] == 1 + assert extra["search_count"] == 1 + + # Verify notification logs were created for both types + notification_logs = db_session.query(UserNotificationLog).all() + assert len(notification_logs) == 2 + + notification_reasons = {log.notification_reason for log in notification_logs} + assert notification_reasons == { + NotificationConstants.OPPORTUNITY_UPDATES, + NotificationConstants.SEARCH_UPDATES, + } + + # Verify last_notified_at was updated for both + db_session.refresh(saved_opportunity) + db_session.refresh(saved_search) + assert saved_opportunity.last_notified_at > datetime_util.utcnow() - timedelta(minutes=1) + assert saved_search.last_notified_at > datetime_util.utcnow() - timedelta(minutes=1) From b5397969bb699565df9a8eb2c229fa36dc2a156e Mon Sep 17 00:00:00 2001 From: Michael Huneke Date: Fri, 31 Jan 2025 17:30:18 -0500 Subject: [PATCH 02/20] Add test for multiuser --- .../test_generate_notifications.py | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/api/tests/src/task/notifications/test_generate_notifications.py b/api/tests/src/task/notifications/test_generate_notifications.py index 22a008f14..b34674ac9 100644 --- a/api/tests/src/task/notifications/test_generate_notifications.py +++ b/api/tests/src/task/notifications/test_generate_notifications.py @@ -232,3 +232,55 @@ def test_combined_notifications_cli( db_session.refresh(saved_search) assert saved_opportunity.last_notified_at > datetime_util.utcnow() - timedelta(minutes=1) assert saved_search.last_notified_at > datetime_util.utcnow() - timedelta(minutes=1) + + +def test_grouped_search_queries_cli( + cli_runner, db_session, enable_factory_create, clear_notification_logs +): + """Test that verifies we properly handle multiple users with the same search query""" + # Create two users with the same search query + user1 = factories.UserFactory.create() + user2 = factories.UserFactory.create() + + same_search_query = {"keywords": "shared search"} + + # Create saved searches with the same query but different results + saved_search1 = factories.UserSavedSearchFactory.create( + user=user1, + search_query=same_search_query, + name="User 1 Search", + last_notified_at=datetime_util.utcnow() - timedelta(days=1), + searched_opportunity_ids=[1, 2, 3], + ) + + saved_search2 = factories.UserSavedSearchFactory.create( + user=user2, + search_query=same_search_query, + name="User 2 Search", + last_notified_at=datetime_util.utcnow() - timedelta(days=1), + searched_opportunity_ids=[4, 5, 6], + ) + + result = cli_runner.invoke(args=["task", "generate-notifications"]) + + assert result.exit_code == 0 + + # Verify notification logs were created for both users + notification_logs = ( + db_session.query(UserNotificationLog) + .filter(UserNotificationLog.notification_reason == NotificationConstants.SEARCH_UPDATES) + .all() + ) + assert len(notification_logs) == 2 + + # Verify each user got their own notification + user_ids = {log.user_id for log in notification_logs} + assert user_ids == {user1.user_id, user2.user_id} + + # Verify both searches were updated with the same new results + db_session.refresh(saved_search1) + db_session.refresh(saved_search2) + + assert saved_search1.searched_opportunity_ids == saved_search2.searched_opportunity_ids + assert saved_search1.last_notified_at > datetime_util.utcnow() - timedelta(minutes=1) + assert saved_search2.last_notified_at > datetime_util.utcnow() - timedelta(minutes=1) From 8bb53967aa6d06be2f0519fe3ab97e85a989a652 Mon Sep 17 00:00:00 2001 From: Michael Huneke Date: Tue, 4 Feb 2025 13:55:07 -0500 Subject: [PATCH 03/20] Update tests --- .../notifications/generate_notifications.py | 19 +++++++---- .../test_generate_notifications.py | 34 +++++++++++++++++-- 2 files changed, 43 insertions(+), 10 deletions(-) diff --git a/api/src/task/notifications/generate_notifications.py b/api/src/task/notifications/generate_notifications.py index cb420eeed..f8dc301d5 100644 --- a/api/src/task/notifications/generate_notifications.py +++ b/api/src/task/notifications/generate_notifications.py @@ -7,8 +7,11 @@ import src.adapters.db as db import src.adapters.db.flask_db as flask_db +import src.adapters.search as search +import src.adapters.search.flask_opensearch as flask_opensearch from src.db.models.opportunity_models import OpportunityChangeAudit from src.db.models.user_models import UserNotificationLog, UserSavedOpportunity, UserSavedSearch +from src.services.opportunities_v1.search_opportunities import search_opportunities_id from src.task.ecs_background_task import ecs_background_task from src.task.task import Task from src.task.task_blueprint import task_blueprint @@ -21,10 +24,11 @@ "generate-notifications", help="Send notifications for opportunity and search changes" ) @ecs_background_task("generate-notifications") +@flask_opensearch.with_search_client() @flask_db.with_db_session() -def run_notification_task(db_session: db.Session) -> None: +def run_notification_task(db_session: db.Session, search_client: search.SearchClient) -> None: """Run the daily notification task""" - task = NotificationTask(db_session) + task = NotificationTask(db_session, search_client) task.run() @@ -51,9 +55,10 @@ class Metrics(StrEnum): SEARCHES_TRACKED = "searches_tracked" NOTIFICATIONS_SENT = "notifications_sent" - def __init__(self, db_session: db.Session) -> None: + def __init__(self, db_session: db.Session, search_client: search.SearchClient) -> None: super().__init__(db_session) self.user_notification_map: dict[uuid.UUID, NotificationContainer] = {} + self.search_client = search_client def run_task(self) -> None: """Main task logic to collect and send notifications""" @@ -110,13 +115,13 @@ def _collect_search_notifications(self) -> None: # For each unique query, check if results have changed for _, searches in query_map.items(): - # TODO: Replace with actual search index query - # current_results = search_index.search(search_query) - current_results: set[int] = set() # Placeholder for actual search results + current_results: list[int] = search_opportunities_id( + self.search_client, searches[0].search_query + ) for saved_search in searches: previous_results = set(saved_search.searched_opportunity_ids) - if current_results != previous_results: + if set(current_results) != previous_results: user_id = saved_search.user_id if user_id not in self.user_notification_map: self.user_notification_map[user_id] = NotificationContainer() diff --git a/api/tests/src/task/notifications/test_generate_notifications.py b/api/tests/src/task/notifications/test_generate_notifications.py index b34674ac9..552d395de 100644 --- a/api/tests/src/task/notifications/test_generate_notifications.py +++ b/api/tests/src/task/notifications/test_generate_notifications.py @@ -4,9 +4,23 @@ import tests.src.db.models.factories as factories from src.db.models.user_models import UserNotificationLog +from src.api.opportunities_v1.opportunity_schemas import OpportunityV1Schema from src.task.notifications.generate_notifications import NotificationConstants from src.util import datetime_util +from tests.src.api.opportunities_v1.test_opportunity_route_search import OPPORTUNITIES + + +@pytest.fixture +def setup_search_data(opportunity_index, opportunity_index_alias, search_client): + # Load into the search index + schema = OpportunityV1Schema() + json_records = [schema.dump(opportunity) for opportunity in OPPORTUNITIES] + search_client.bulk_upsert(opportunity_index, json_records, "opportunity_id") + + # Swap the search index alias + search_client.swap_alias_index(opportunity_index, opportunity_index_alias) + @pytest.fixture def clear_notification_logs(db_session): @@ -134,7 +148,13 @@ def test_no_notification_log_when_no_updates( def test_search_notifications_cli( - cli_runner, db_session, enable_factory_create, user, caplog, clear_notification_logs + cli_runner, + db_session, + enable_factory_create, + user, + caplog, + clear_notification_logs, + setup_search_data, ): """Test that verifies we can collect and send search notifications via CLI""" # Create a saved search that needs notification @@ -176,7 +196,12 @@ def test_search_notifications_cli( def test_combined_notifications_cli( - cli_runner, db_session, enable_factory_create, user, caplog, clear_notification_logs + cli_runner, + db_session, + enable_factory_create, + user, + caplog, + clear_notification_logs, ): """Test that verifies we can handle both opportunity and search notifications together""" # Create a saved opportunity that needs notification @@ -235,7 +260,10 @@ def test_combined_notifications_cli( def test_grouped_search_queries_cli( - cli_runner, db_session, enable_factory_create, clear_notification_logs + cli_runner, + db_session, + enable_factory_create, + clear_notification_logs, ): """Test that verifies we properly handle multiple users with the same search query""" # Create two users with the same search query From 38a6e6f0d7f4eb62b1d48c5e1cc75d389dafed73 Mon Sep 17 00:00:00 2001 From: Michael Huneke Date: Tue, 4 Feb 2025 14:40:13 -0500 Subject: [PATCH 04/20] Add test --- .../test_generate_notifications.py | 70 ++++++++++++++++++- 1 file changed, 68 insertions(+), 2 deletions(-) diff --git a/api/tests/src/task/notifications/test_generate_notifications.py b/api/tests/src/task/notifications/test_generate_notifications.py index 552d395de..e251a3925 100644 --- a/api/tests/src/task/notifications/test_generate_notifications.py +++ b/api/tests/src/task/notifications/test_generate_notifications.py @@ -3,11 +3,10 @@ import pytest import tests.src.db.models.factories as factories -from src.db.models.user_models import UserNotificationLog from src.api.opportunities_v1.opportunity_schemas import OpportunityV1Schema +from src.db.models.user_models import UserNotificationLog from src.task.notifications.generate_notifications import NotificationConstants from src.util import datetime_util - from tests.src.api.opportunities_v1.test_opportunity_route_search import OPPORTUNITIES @@ -312,3 +311,70 @@ def test_grouped_search_queries_cli( assert saved_search1.searched_opportunity_ids == saved_search2.searched_opportunity_ids assert saved_search1.last_notified_at > datetime_util.utcnow() - timedelta(minutes=1) assert saved_search2.last_notified_at > datetime_util.utcnow() - timedelta(minutes=1) + + +def test_search_notifications_on_index_change( + cli_runner, + db_session, + enable_factory_create, + user, + opportunity_index, + search_client, + clear_notification_logs, +): + """Test that verifies notifications are generated when search results change due to index updates""" + # Create a saved search with initial results + saved_search = factories.UserSavedSearchFactory.create( + user=user, + search_query={"keywords": "test"}, + name="Test Search", + last_notified_at=datetime_util.utcnow() - timedelta(days=1), + searched_opportunity_ids=[1, 2], # Initial results + ) + + # Update the search index with new data that will change the results + schema = OpportunityV1Schema() + new_opportunity = factories.OpportunityFactory.create( + opportunity_id=999, + opportunity_title="New Test Opportunity", + ) + factories.OpportunitySummaryFactory.build( + opportunity=new_opportunity, + summary_description="This should appear in test search results", + ) + json_record = schema.dump(new_opportunity) + search_client.bulk_upsert(opportunity_index, [json_record], "opportunity_id") + + # Run the notification task + result = cli_runner.invoke(args=["task", "generate-notifications"]) + assert result.exit_code == 0 + + # Verify notification log was created due to changed results + notification_logs = ( + db_session.query(UserNotificationLog) + .filter( + UserNotificationLog.user_id == user.user_id, + UserNotificationLog.notification_reason == NotificationConstants.SEARCH_UPDATES, + ) + .all() + ) + assert len(notification_logs) == 1 + + # Verify the saved search was updated with new results + db_session.refresh(saved_search) + assert 999 in saved_search.searched_opportunity_ids # New opportunity should be in results + assert saved_search.last_notified_at > datetime_util.utcnow() - timedelta(minutes=1) + + # Run the task again - should not generate new notifications since results haven't changed + result = cli_runner.invoke(args=["task", "generate-notifications"]) + assert result.exit_code == 0 + + notification_logs = ( + db_session.query(UserNotificationLog) + .filter( + UserNotificationLog.user_id == user.user_id, + UserNotificationLog.notification_reason == NotificationConstants.SEARCH_UPDATES, + ) + .all() + ) + assert len(notification_logs) == 1 # Should still only be one notification From 6f6f5f6d60ce6736f907c5a7fa5a65605891e413 Mon Sep 17 00:00:00 2001 From: Michael Huneke Date: Tue, 4 Feb 2025 14:58:38 -0500 Subject: [PATCH 05/20] Try removing? --- .../src/task/notifications/test_generate_notifications.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/api/tests/src/task/notifications/test_generate_notifications.py b/api/tests/src/task/notifications/test_generate_notifications.py index e251a3925..5be922817 100644 --- a/api/tests/src/task/notifications/test_generate_notifications.py +++ b/api/tests/src/task/notifications/test_generate_notifications.py @@ -152,10 +152,11 @@ def test_search_notifications_cli( enable_factory_create, user, caplog, - clear_notification_logs, setup_search_data, ): """Test that verifies we can collect and send search notifications via CLI""" + db_session.query(UserNotificationLog).delete() + # Create a saved search that needs notification saved_search = factories.UserSavedSearchFactory.create( user=user, From cab4ac13f085dc74cb59e969a31bf0c46156ee88 Mon Sep 17 00:00:00 2001 From: Michael Huneke Date: Tue, 4 Feb 2025 15:33:42 -0500 Subject: [PATCH 06/20] Remove logging --- api/src/task/notifications/generate_notifications.py | 1 - 1 file changed, 1 deletion(-) diff --git a/api/src/task/notifications/generate_notifications.py b/api/src/task/notifications/generate_notifications.py index f8dc301d5..c01f70055 100644 --- a/api/src/task/notifications/generate_notifications.py +++ b/api/src/task/notifications/generate_notifications.py @@ -130,7 +130,6 @@ def _collect_search_notifications(self) -> None: # Update the saved search with new results saved_search.searched_opportunity_ids = list(current_results) - logger.info(f"User notification map: {self.user_notification_map}") logger.info( "Collected search notifications", extra={ From b266366e379da11a8830f47d20974f5874bd7cd0 Mon Sep 17 00:00:00 2001 From: Mike H Date: Tue, 4 Feb 2025 15:34:34 -0500 Subject: [PATCH 07/20] Update api/src/task/notifications/generate_notifications.py Co-authored-by: Michael Chouinard <46358556+chouinar@users.noreply.github.com> --- api/src/task/notifications/generate_notifications.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/task/notifications/generate_notifications.py b/api/src/task/notifications/generate_notifications.py index c01f70055..dc704f95b 100644 --- a/api/src/task/notifications/generate_notifications.py +++ b/api/src/task/notifications/generate_notifications.py @@ -114,7 +114,7 @@ def _collect_search_notifications(self) -> None: query_map[query_key].append(saved_search) # For each unique query, check if results have changed - for _, searches in query_map.items(): + for searches in query_map.values(): current_results: list[int] = search_opportunities_id( self.search_client, searches[0].search_query ) From 5c892177f247bc0c15f9dbf12de488b78177bda5 Mon Sep 17 00:00:00 2001 From: Michael Huneke Date: Tue, 4 Feb 2025 15:45:39 -0500 Subject: [PATCH 08/20] Add test for stripped pagination from search --- .../notifications/generate_notifications.py | 12 ++++++++++- .../test_generate_notifications.py | 20 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/api/src/task/notifications/generate_notifications.py b/api/src/task/notifications/generate_notifications.py index dc704f95b..da08417a0 100644 --- a/api/src/task/notifications/generate_notifications.py +++ b/api/src/task/notifications/generate_notifications.py @@ -108,7 +108,10 @@ def _collect_search_notifications(self) -> None: # Group searches by query to minimize search index calls query_map: dict[str, list[UserSavedSearch]] = {} for saved_search in saved_searches: - query_key = str(saved_search.search_query) + # Remove pagination parameters before using as key + search_query = _strip_pagination_params(saved_search.search_query) + query_key = str(search_query) + if query_key not in query_map: query_map[query_key] = [] query_map[query_key].append(saved_search) @@ -201,3 +204,10 @@ def _send_notifications(self) -> None: self.increment(self.Metrics.SEARCHES_TRACKED, len(container.saved_searches)) self.increment(self.Metrics.NOTIFICATIONS_SENT) self.increment(self.Metrics.USERS_NOTIFIED) + + +def _strip_pagination_params(search_query: dict) -> dict: + """Remove pagination parameters from a search query""" + search_query = search_query.copy() + search_query.pop("pagination", None) + return search_query diff --git a/api/tests/src/task/notifications/test_generate_notifications.py b/api/tests/src/task/notifications/test_generate_notifications.py index 5be922817..c4f1a69a1 100644 --- a/api/tests/src/task/notifications/test_generate_notifications.py +++ b/api/tests/src/task/notifications/test_generate_notifications.py @@ -8,6 +8,7 @@ from src.task.notifications.generate_notifications import NotificationConstants from src.util import datetime_util from tests.src.api.opportunities_v1.test_opportunity_route_search import OPPORTUNITIES +from src.task.notifications.generate_notifications import _strip_pagination_params @pytest.fixture @@ -379,3 +380,22 @@ def test_search_notifications_on_index_change( .all() ) assert len(notification_logs) == 1 # Should still only be one notification + + +def test_pagination_params_are_stripped_from_search_query( + cli_runner, db_session, enable_factory_create, user, clear_notification_logs +): + """Test that pagination parameters are stripped from search queries""" + saved_search = factories.UserSavedSearchFactory.create( + user=user, + search_query={ + "query": "test", + "pagination": {"page": 1, "per_page": 10}, + }, + name="Test Search", + last_notified_at=datetime_util.utcnow() - timedelta(days=1), + searched_opportunity_ids=[1, 2], + ) + + params = _strip_pagination_params(saved_search.search_query) + assert params.keys() == {"query"} From 92e0f4b14b2eb8d6772bc04d2b4a1086d70a1573 Mon Sep 17 00:00:00 2001 From: Michael Huneke Date: Tue, 4 Feb 2025 15:48:08 -0500 Subject: [PATCH 09/20] Run the task directly instead of CLI --- .../task/notifications/test_generate_notifications.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/api/tests/src/task/notifications/test_generate_notifications.py b/api/tests/src/task/notifications/test_generate_notifications.py index c4f1a69a1..2a61481a1 100644 --- a/api/tests/src/task/notifications/test_generate_notifications.py +++ b/api/tests/src/task/notifications/test_generate_notifications.py @@ -5,7 +5,7 @@ import tests.src.db.models.factories as factories from src.api.opportunities_v1.opportunity_schemas import OpportunityV1Schema from src.db.models.user_models import UserNotificationLog -from src.task.notifications.generate_notifications import NotificationConstants +from src.task.notifications.generate_notifications import NotificationConstants, NotificationTask from src.util import datetime_util from tests.src.api.opportunities_v1.test_opportunity_route_search import OPPORTUNITIES from src.task.notifications.generate_notifications import _strip_pagination_params @@ -348,8 +348,8 @@ def test_search_notifications_on_index_change( search_client.bulk_upsert(opportunity_index, [json_record], "opportunity_id") # Run the notification task - result = cli_runner.invoke(args=["task", "generate-notifications"]) - assert result.exit_code == 0 + task = NotificationTask(db_session, search_client) + task.run() # Verify notification log was created due to changed results notification_logs = ( @@ -368,8 +368,8 @@ def test_search_notifications_on_index_change( assert saved_search.last_notified_at > datetime_util.utcnow() - timedelta(minutes=1) # Run the task again - should not generate new notifications since results haven't changed - result = cli_runner.invoke(args=["task", "generate-notifications"]) - assert result.exit_code == 0 + task_rerun = NotificationTask(db_session, search_client) + task_rerun.run() notification_logs = ( db_session.query(UserNotificationLog) From 52c3b13fb148682c8d02f01eff3972c9e2c35642 Mon Sep 17 00:00:00 2001 From: Michael Huneke Date: Tue, 4 Feb 2025 18:05:26 -0500 Subject: [PATCH 10/20] Fix tests --- .../src/task/notifications/test_generate_notifications.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/api/tests/src/task/notifications/test_generate_notifications.py b/api/tests/src/task/notifications/test_generate_notifications.py index 2a61481a1..16f1f89cb 100644 --- a/api/tests/src/task/notifications/test_generate_notifications.py +++ b/api/tests/src/task/notifications/test_generate_notifications.py @@ -4,7 +4,7 @@ import tests.src.db.models.factories as factories from src.api.opportunities_v1.opportunity_schemas import OpportunityV1Schema -from src.db.models.user_models import UserNotificationLog +from src.db.models.user_models import UserNotificationLog, UserSavedOpportunity, UserSavedSearch from src.task.notifications.generate_notifications import NotificationConstants, NotificationTask from src.util import datetime_util from tests.src.api.opportunities_v1.test_opportunity_route_search import OPPORTUNITIES @@ -26,6 +26,8 @@ def setup_search_data(opportunity_index, opportunity_index_alias, search_client) def clear_notification_logs(db_session): """Clear all notification logs""" db_session.query(UserNotificationLog).delete() + db_session.query(UserSavedOpportunity).delete() + db_session.query(UserSavedSearch).delete() def test_via_cli(cli_runner, db_session, enable_factory_create, user): @@ -153,10 +155,10 @@ def test_search_notifications_cli( enable_factory_create, user, caplog, + clear_notification_logs, setup_search_data, ): """Test that verifies we can collect and send search notifications via CLI""" - db_session.query(UserNotificationLog).delete() # Create a saved search that needs notification saved_search = factories.UserSavedSearchFactory.create( From 3831f63cae05d30512a97f7b23cf9610b94e87b6 Mon Sep 17 00:00:00 2001 From: Michael Huneke Date: Tue, 4 Feb 2025 18:09:54 -0500 Subject: [PATCH 11/20] Fix imports --- .../src/task/notifications/test_generate_notifications.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/api/tests/src/task/notifications/test_generate_notifications.py b/api/tests/src/task/notifications/test_generate_notifications.py index 16f1f89cb..c14fd067a 100644 --- a/api/tests/src/task/notifications/test_generate_notifications.py +++ b/api/tests/src/task/notifications/test_generate_notifications.py @@ -5,10 +5,13 @@ import tests.src.db.models.factories as factories from src.api.opportunities_v1.opportunity_schemas import OpportunityV1Schema from src.db.models.user_models import UserNotificationLog, UserSavedOpportunity, UserSavedSearch -from src.task.notifications.generate_notifications import NotificationConstants, NotificationTask +from src.task.notifications.generate_notifications import ( + NotificationConstants, + NotificationTask, + _strip_pagination_params, +) from src.util import datetime_util from tests.src.api.opportunities_v1.test_opportunity_route_search import OPPORTUNITIES -from src.task.notifications.generate_notifications import _strip_pagination_params @pytest.fixture From 303e6cfc32deb65370df7b36fcbef57674122922 Mon Sep 17 00:00:00 2001 From: Michael Huneke Date: Wed, 5 Feb 2025 15:51:29 -0500 Subject: [PATCH 12/20] Update tests / generate notifications with email service --- .../notifications/generate_notifications.py | 54 +++++- .../test_generate_notifications.py | 161 +++++++++++------- 2 files changed, 153 insertions(+), 62 deletions(-) diff --git a/api/src/task/notifications/generate_notifications.py b/api/src/task/notifications/generate_notifications.py index da08417a0..a3d855e02 100644 --- a/api/src/task/notifications/generate_notifications.py +++ b/api/src/task/notifications/generate_notifications.py @@ -3,23 +3,36 @@ from dataclasses import dataclass, field from enum import StrEnum +import botocore.client +from pydantic import Field from sqlalchemy import select, update import src.adapters.db as db import src.adapters.db.flask_db as flask_db import src.adapters.search as search import src.adapters.search.flask_opensearch as flask_opensearch +from src.adapters.aws.pinpoint_adapter import send_pinpoint_email_raw from src.db.models.opportunity_models import OpportunityChangeAudit -from src.db.models.user_models import UserNotificationLog, UserSavedOpportunity, UserSavedSearch +from src.db.models.user_models import ( + LinkExternalUser, + UserNotificationLog, + UserSavedOpportunity, + UserSavedSearch, +) from src.services.opportunities_v1.search_opportunities import search_opportunities_id from src.task.ecs_background_task import ecs_background_task from src.task.task import Task from src.task.task_blueprint import task_blueprint from src.util import datetime_util +from src.util.env_config import PydanticBaseEnvConfig logger = logging.getLogger(__name__) +class GenerateNotificationsConfig(PydanticBaseEnvConfig): + app_id: str = Field(alias="PINPOINT_APP_ID") + + @task_blueprint.cli.command( "generate-notifications", help="Send notifications for opportunity and search changes" ) @@ -55,10 +68,20 @@ class Metrics(StrEnum): SEARCHES_TRACKED = "searches_tracked" NOTIFICATIONS_SENT = "notifications_sent" - def __init__(self, db_session: db.Session, search_client: search.SearchClient) -> None: + def __init__( + self, + db_session: db.Session, + search_client: search.SearchClient, + pinpoint_client: botocore.client.BaseClient | None = None, + pinpoint_app_id: str | None = None, + ) -> None: super().__init__(db_session) + self.config = GenerateNotificationsConfig() + self.user_notification_map: dict[uuid.UUID, NotificationContainer] = {} self.search_client = search_client + self.pinpoint_client = pinpoint_client + self.app_id = pinpoint_app_id def run_task(self) -> None: """Main task logic to collect and send notifications""" @@ -147,12 +170,35 @@ def _collect_search_notifications(self) -> None: def _send_notifications(self) -> None: """Send collected notifications to users""" for user_id, container in self.user_notification_map.items(): + print(user_id) + print(container) if not container.saved_opportunities and not container.saved_searches: continue - # TODO: Implement actual notification sending in future ticket + # Get user email from LinkExternalUser using select pattern + stmt = select(LinkExternalUser).where(LinkExternalUser.user_id == user_id) + user = self.db_session.execute(stmt).scalar_one_or_none() + + if not user or not user.email: + logger.warning("No email found for user", extra={"user_id": user_id}) + continue + + # Send email via Pinpoint + subject = "Updates to Your Saved Opportunities" + message = ( + f"You have updates to {len(container.saved_opportunities)} saved opportunities" + ) + + send_pinpoint_email_raw( + to_address=user.email, + subject=subject, + message=message, + pinpoint_client=self.pinpoint_client, + app_id=self.config.app_id, + ) + logger.info( - "Would send notification to user", + "Sending notification to user", extra={ "user_id": user_id, "opportunity_count": len(container.saved_opportunities), diff --git a/api/tests/src/task/notifications/test_generate_notifications.py b/api/tests/src/task/notifications/test_generate_notifications.py index c14fd067a..97d36c414 100644 --- a/api/tests/src/task/notifications/test_generate_notifications.py +++ b/api/tests/src/task/notifications/test_generate_notifications.py @@ -1,8 +1,10 @@ from datetime import timedelta import pytest +from sqlalchemy import select import tests.src.db.models.factories as factories +from src.adapters.aws.pinpoint_adapter import _get_mock_responses from src.api.opportunities_v1.opportunity_schemas import OpportunityV1Schema from src.db.models.user_models import UserNotificationLog, UserSavedOpportunity, UserSavedSearch from src.task.notifications.generate_notifications import ( @@ -14,6 +16,13 @@ from tests.src.api.opportunities_v1.test_opportunity_route_search import OPPORTUNITIES +@pytest.fixture +def user_with_email(db_session, user, monkeypatch): + monkeypatch.setenv("PINPOINT_APP_ID", "test-app-id") + factories.LinkExternalUserFactory.create(user=user, email="test@example.com") + return user + + @pytest.fixture def setup_search_data(opportunity_index, opportunity_index_alias, search_client): # Load into the search index @@ -33,14 +42,91 @@ def clear_notification_logs(db_session): db_session.query(UserSavedSearch).delete() -def test_via_cli(cli_runner, db_session, enable_factory_create, user): +def test_via_cli(cli_runner, db_session, enable_factory_create, user, user_with_email): """Simple test that verifies we can invoke the notification task via CLI""" result = cli_runner.invoke(args=["task", "generate-notifications"]) assert result.exit_code == 0 -def test_collect_notifications_cli(cli_runner, db_session, enable_factory_create, user, caplog): +def test_search_notifications_cli( + cli_runner, + db_session, + enable_factory_create, + user, + user_with_email, + caplog, + clear_notification_logs, + setup_search_data, +): + """Test that verifies we can collect and send search notifications via CLI""" + + # Create a saved search that needs notification + saved_search = factories.UserSavedSearchFactory.create( + user=user, + search_query={"keywords": "test"}, + name="Test Search", + last_notified_at=datetime_util.utcnow() - timedelta(days=1), + searched_opportunity_ids=[1, 2, 3], + ) + + notification_logs_count = ( + db_session.query(UserNotificationLog) + .filter(UserNotificationLog.notification_reason == NotificationConstants.SEARCH_UPDATES) + .count() + ) + + result = cli_runner.invoke(args=["task", "generate-notifications"]) + + assert result.exit_code == 0 + + # Verify expected log messages + assert "Collected search notifications" in caplog.text + assert "Sending notification to user" in caplog.text + + # Verify the log contains the correct metrics + log_records = [r for r in caplog.records if "Sending notification to user" in r.message] + assert len(log_records) == 1 + extra = log_records[0].__dict__ + assert extra["user_id"] == user.user_id + assert extra["opportunity_count"] == 0 + assert extra["search_count"] == 1 + + # Verify notification log was created + notification_logs = ( + db_session.query(UserNotificationLog) + .filter(UserNotificationLog.notification_reason == NotificationConstants.SEARCH_UPDATES) + .all() + ) + assert len(notification_logs) == notification_logs_count + 1 + + # Verify last_notified_at was updated + db_session.refresh(saved_search) + assert saved_search.last_notified_at > datetime_util.utcnow() - timedelta(minutes=1) + + # Verify email was sent via Pinpoint + mock_responses = _get_mock_responses() + assert len(mock_responses) == 1 + + request = mock_responses[0][0] + assert request["MessageRequest"]["Addresses"] == {"test@example.com": {"ChannelType": "EMAIL"}} + + # Verify notification log was created + notification_logs = ( + db_session.execute( + select(UserNotificationLog).where(UserNotificationLog.user_id == user.user_id) + ) + .scalars() + .all() + ) + + assert len(notification_logs) == 2 + assert notification_logs[0].notification_sent is True + + +def test_collect_notifications_cli( + cli_runner, db_session, enable_factory_create, user, user_with_email, caplog +): """Simple test that verifies we can invoke the notification task via CLI""" # Create a saved opportunity that needs notification opportunity = factories.OpportunityFactory.create() @@ -60,10 +146,10 @@ def test_collect_notifications_cli(cli_runner, db_session, enable_factory_create # Verify expected log messages assert "Collected opportunity notifications" in caplog.text - assert "Would send notification to user" in caplog.text + assert "Sending notification to user" in caplog.text # Verify the log contains the correct metrics - log_records = [r for r in caplog.records if "Would send notification to user" in r.message] + log_records = [r for r in caplog.records if "Sending notification to user" in r.message] assert len(log_records) == 1 extra = log_records[0].__dict__ assert extra["user_id"] == user.user_id @@ -71,7 +157,9 @@ def test_collect_notifications_cli(cli_runner, db_session, enable_factory_create assert extra["search_count"] == 0 -def test_last_notified_at_updates(cli_runner, db_session, enable_factory_create, user): +def test_last_notified_at_updates( + cli_runner, db_session, enable_factory_create, user, user_with_email +): """Test that last_notified_at gets updated after sending notifications""" # Create an opportunity that was updated after the last notification opportunity = factories.OpportunityFactory.create() @@ -101,7 +189,7 @@ def test_last_notified_at_updates(cli_runner, db_session, enable_factory_create, def test_notification_log_creation( - cli_runner, db_session, enable_factory_create, clear_notification_logs, user + cli_runner, db_session, enable_factory_create, clear_notification_logs, user, user_with_email ): """Test that notification logs are created when notifications are sent""" # Create a saved opportunity that needs notification @@ -132,7 +220,7 @@ def test_notification_log_creation( def test_no_notification_log_when_no_updates( - cli_runner, db_session, enable_factory_create, clear_notification_logs, user + cli_runner, db_session, enable_factory_create, clear_notification_logs, user, user_with_email ): """Test that no notification log is created when there are no updates""" # Create a saved opportunity that doesn't need notification @@ -152,60 +240,12 @@ def test_no_notification_log_when_no_updates( assert len(notification_logs) == 0 -def test_search_notifications_cli( - cli_runner, - db_session, - enable_factory_create, - user, - caplog, - clear_notification_logs, - setup_search_data, -): - """Test that verifies we can collect and send search notifications via CLI""" - - # Create a saved search that needs notification - saved_search = factories.UserSavedSearchFactory.create( - user=user, - search_query={"keywords": "test"}, - name="Test Search", - last_notified_at=datetime_util.utcnow() - timedelta(days=1), - searched_opportunity_ids=[1, 2, 3], - ) - - result = cli_runner.invoke(args=["task", "generate-notifications"]) - - assert result.exit_code == 0 - - # Verify expected log messages - assert "Collected search notifications" in caplog.text - assert "Would send notification to user" in caplog.text - - # Verify the log contains the correct metrics - log_records = [r for r in caplog.records if "Would send notification to user" in r.message] - assert len(log_records) == 1 - extra = log_records[0].__dict__ - assert extra["user_id"] == user.user_id - assert extra["opportunity_count"] == 0 - assert extra["search_count"] == 1 - - # Verify notification log was created - notification_logs = ( - db_session.query(UserNotificationLog) - .filter(UserNotificationLog.notification_reason == NotificationConstants.SEARCH_UPDATES) - .all() - ) - assert len(notification_logs) == 1 - - # Verify last_notified_at was updated - db_session.refresh(saved_search) - assert saved_search.last_notified_at > datetime_util.utcnow() - timedelta(minutes=1) - - def test_combined_notifications_cli( cli_runner, db_session, enable_factory_create, user, + user_with_email, caplog, clear_notification_logs, ): @@ -238,10 +278,10 @@ def test_combined_notifications_cli( # Verify expected log messages assert "Collected opportunity notifications" in caplog.text assert "Collected search notifications" in caplog.text - assert "Would send notification to user" in caplog.text + assert "Sending notification to user" in caplog.text # Verify the log contains the correct metrics - log_records = [r for r in caplog.records if "Would send notification to user" in r.message] + log_records = [r for r in caplog.records if "Sending notification to user" in r.message] assert len(log_records) == 1 extra = log_records[0].__dict__ assert extra["user_id"] == user.user_id @@ -270,11 +310,15 @@ def test_grouped_search_queries_cli( db_session, enable_factory_create, clear_notification_logs, + user, + user_with_email, ): """Test that verifies we properly handle multiple users with the same search query""" # Create two users with the same search query user1 = factories.UserFactory.create() user2 = factories.UserFactory.create() + factories.LinkExternalUserFactory.create(user=user1, email="user1@example.com") + factories.LinkExternalUserFactory.create(user=user2, email="user2@example.com") same_search_query = {"keywords": "shared search"} @@ -325,6 +369,7 @@ def test_search_notifications_on_index_change( db_session, enable_factory_create, user, + user_with_email, opportunity_index, search_client, clear_notification_logs, From 0828da3b6724551fdb10a272bcad492fcafd8cfa Mon Sep 17 00:00:00 2001 From: Michael Huneke Date: Thu, 6 Feb 2025 09:55:31 -0500 Subject: [PATCH 13/20] Remove print --- api/src/task/notifications/generate_notifications.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/api/src/task/notifications/generate_notifications.py b/api/src/task/notifications/generate_notifications.py index a3d855e02..31801720d 100644 --- a/api/src/task/notifications/generate_notifications.py +++ b/api/src/task/notifications/generate_notifications.py @@ -170,8 +170,6 @@ def _collect_search_notifications(self) -> None: def _send_notifications(self) -> None: """Send collected notifications to users""" for user_id, container in self.user_notification_map.items(): - print(user_id) - print(container) if not container.saved_opportunities and not container.saved_searches: continue From d841ddaa6410184fc3c2fe1adad6e1c24da807ac Mon Sep 17 00:00:00 2001 From: Michael Huneke Date: Thu, 6 Feb 2025 13:56:02 -0500 Subject: [PATCH 14/20] PR feedback --- api/src/db/models/user_models.py | 49 +++++++++++------ .../notifications/generate_notifications.py | 53 +++++++++++++------ 2 files changed, 68 insertions(+), 34 deletions(-) diff --git a/api/src/db/models/user_models.py b/api/src/db/models/user_models.py index 70091869c..76c7275b3 100644 --- a/api/src/db/models/user_models.py +++ b/api/src/db/models/user_models.py @@ -14,6 +14,28 @@ from src.util import datetime_util +class LinkExternalUser(ApiSchemaTable, TimestampMixin): + __tablename__ = "link_external_user" + + link_external_user_id: Mapped[uuid.UUID] = mapped_column(primary_key=True, default=uuid.uuid4) + + external_user_id: Mapped[str] = mapped_column(index=True, unique=True) + + external_user_type: Mapped[ExternalUserType] = mapped_column( + "external_user_type_id", + LookupColumn(LkExternalUserType), + ForeignKey(LkExternalUserType.external_user_type_id), + index=True, + ) + + user_id: Mapped[uuid.UUID] = mapped_column(ForeignKey("User.user_id"), index=True) + user: Mapped["User"] = relationship( + "User", primaryjoin="LinkExternalUser.user_id==foreign(User.user_id)" + ) + + email: Mapped[str] + + class User(ApiSchemaTable, TimestampMixin): __tablename__ = "user" @@ -30,25 +52,18 @@ class User(ApiSchemaTable, TimestampMixin): "UserSavedSearch", back_populates="user", uselist=True, cascade="all, delete-orphan" ) - -class LinkExternalUser(ApiSchemaTable, TimestampMixin): - __tablename__ = "link_external_user" - - link_external_user_id: Mapped[uuid.UUID] = mapped_column(primary_key=True, default=uuid.uuid4) - - external_user_id: Mapped[str] = mapped_column(index=True, unique=True) - - external_user_type: Mapped[ExternalUserType] = mapped_column( - "external_user_type_id", - LookupColumn(LkExternalUserType), - ForeignKey(LkExternalUserType.external_user_type_id), - index=True, + linked_external_user: Mapped[LinkExternalUser | None] = relationship( + "LinkExternalUser", + primaryjoin="LinkExternalUser.user_id==foreign(User.user_id)", + uselist=False, + overlaps="user", ) - user_id: Mapped[uuid.UUID] = mapped_column(ForeignKey(User.user_id), index=True) - user: Mapped[User] = relationship(User) - - email: Mapped[str] + @property + def email(self) -> str | None: + if self.linked_external_user is not None: + return self.linked_external_user.email + return None class UserTokenSession(ApiSchemaTable, TimestampMixin): diff --git a/api/src/task/notifications/generate_notifications.py b/api/src/task/notifications/generate_notifications.py index 31801720d..1682a751b 100644 --- a/api/src/task/notifications/generate_notifications.py +++ b/api/src/task/notifications/generate_notifications.py @@ -14,7 +14,7 @@ from src.adapters.aws.pinpoint_adapter import send_pinpoint_email_raw from src.db.models.opportunity_models import OpportunityChangeAudit from src.db.models.user_models import ( - LinkExternalUser, + User, UserNotificationLog, UserSavedOpportunity, UserSavedSearch, @@ -173,9 +173,9 @@ def _send_notifications(self) -> None: if not container.saved_opportunities and not container.saved_searches: continue - # Get user email from LinkExternalUser using select pattern - stmt = select(LinkExternalUser).where(LinkExternalUser.user_id == user_id) - user = self.db_session.execute(stmt).scalar_one_or_none() + user = self.db_session.execute( + select(User).where(User.user_id == user_id) + ).scalar_one_or_none() if not user or not user.email: logger.warning("No email found for user", extra={"user_id": user_id}) @@ -187,14 +187,6 @@ def _send_notifications(self) -> None: f"You have updates to {len(container.saved_opportunities)} saved opportunities" ) - send_pinpoint_email_raw( - to_address=user.email, - subject=subject, - message=message, - pinpoint_client=self.pinpoint_client, - app_id=self.config.app_id, - ) - logger.info( "Sending notification to user", extra={ @@ -204,21 +196,48 @@ def _send_notifications(self) -> None: }, ) - # Create notification log entry notification_log = UserNotificationLog( user_id=user_id, notification_reason=NotificationConstants.OPPORTUNITY_UPDATES, - notification_sent=True, + notification_sent=False, # Default to False, update on success ) self.db_session.add(notification_log) + try: + send_pinpoint_email_raw( + to_address=user.email, + subject=subject, + message=message, + pinpoint_client=self.pinpoint_client, + app_id=self.config.app_id, + ) + notification_log.notification_sent = True + logger.info( + "Successfully sent notification to user", + extra={ + "user_id": user_id, + "opportunity_count": len(container.saved_opportunities), + "search_count": len(container.saved_searches), + }, + ) + except Exception: + # Notification log will be updated in the finally block + logger.exception( + "Failed to send notification email", + extra={"user_id": user_id, "email": user.email}, + ) + + self.db_session.add(notification_log) + if container.saved_searches: - notification_log = UserNotificationLog( + search_notification_log = UserNotificationLog( user_id=user_id, notification_reason=NotificationConstants.SEARCH_UPDATES, - notification_sent=True, + notification_sent=False, # Default to False, update if email was successful ) - self.db_session.add(notification_log) + self.db_session.add(search_notification_log) + if notification_log.notification_sent: + search_notification_log.notification_sent = True # Update last_notified_at for all opportunities we just notified about opportunity_ids = [ From 23bfee1bfc14d98466328d03b63109520d1445ee Mon Sep 17 00:00:00 2001 From: Michael Huneke Date: Thu, 6 Feb 2025 14:27:09 -0500 Subject: [PATCH 15/20] Update model refs --- api/src/db/models/user_models.py | 50 ++++++++++++++++---------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/api/src/db/models/user_models.py b/api/src/db/models/user_models.py index 76c7275b3..83b0f0c03 100644 --- a/api/src/db/models/user_models.py +++ b/api/src/db/models/user_models.py @@ -14,28 +14,6 @@ from src.util import datetime_util -class LinkExternalUser(ApiSchemaTable, TimestampMixin): - __tablename__ = "link_external_user" - - link_external_user_id: Mapped[uuid.UUID] = mapped_column(primary_key=True, default=uuid.uuid4) - - external_user_id: Mapped[str] = mapped_column(index=True, unique=True) - - external_user_type: Mapped[ExternalUserType] = mapped_column( - "external_user_type_id", - LookupColumn(LkExternalUserType), - ForeignKey(LkExternalUserType.external_user_type_id), - index=True, - ) - - user_id: Mapped[uuid.UUID] = mapped_column(ForeignKey("User.user_id"), index=True) - user: Mapped["User"] = relationship( - "User", primaryjoin="LinkExternalUser.user_id==foreign(User.user_id)" - ) - - email: Mapped[str] - - class User(ApiSchemaTable, TimestampMixin): __tablename__ = "user" @@ -52,11 +30,11 @@ class User(ApiSchemaTable, TimestampMixin): "UserSavedSearch", back_populates="user", uselist=True, cascade="all, delete-orphan" ) - linked_external_user: Mapped[LinkExternalUser | None] = relationship( + linked_external_user: Mapped["LinkExternalUser | None"] = relationship( "LinkExternalUser", - primaryjoin="LinkExternalUser.user_id==foreign(User.user_id)", + primaryjoin=lambda: LinkExternalUser.user_id == User.user_id, uselist=False, - overlaps="user", + viewonly=True, ) @property @@ -66,6 +44,28 @@ def email(self) -> str | None: return None +class LinkExternalUser(ApiSchemaTable, TimestampMixin): + __tablename__ = "link_external_user" + + link_external_user_id: Mapped[uuid.UUID] = mapped_column(primary_key=True, default=uuid.uuid4) + + external_user_id: Mapped[str] = mapped_column(index=True, unique=True) + + external_user_type: Mapped[ExternalUserType] = mapped_column( + "external_user_type_id", + LookupColumn(LkExternalUserType), + ForeignKey(LkExternalUserType.external_user_type_id), + index=True, + ) + + user_id: Mapped[uuid.UUID] = mapped_column(ForeignKey(User.user_id), index=True) + user: Mapped["User"] = relationship( + "User", primaryjoin=lambda: LinkExternalUser.user_id == User.user_id + ) + + email: Mapped[str] + + class UserTokenSession(ApiSchemaTable, TimestampMixin): __tablename__ = "user_token_session" From a76c17d00ba30e24bb952e6d7ce167bc398ea637 Mon Sep 17 00:00:00 2001 From: Michael Huneke Date: Thu, 6 Feb 2025 16:30:36 -0500 Subject: [PATCH 16/20] Clear responses before running test --- api/src/adapters/aws/pinpoint_adapter.py | 5 +++++ .../src/task/notifications/test_generate_notifications.py | 4 +++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/api/src/adapters/aws/pinpoint_adapter.py b/api/src/adapters/aws/pinpoint_adapter.py index 346429a40..3d1df2157 100644 --- a/api/src/adapters/aws/pinpoint_adapter.py +++ b/api/src/adapters/aws/pinpoint_adapter.py @@ -116,5 +116,10 @@ def _handle_mock_response(request: dict, to_address: str) -> PinpointResponse: return response +def _clear_mock_responses() -> None: + global _mock_responses + _mock_responses = [] + + def _get_mock_responses() -> list[tuple[dict, PinpointResponse]]: return _mock_responses diff --git a/api/tests/src/task/notifications/test_generate_notifications.py b/api/tests/src/task/notifications/test_generate_notifications.py index 97d36c414..970a84d65 100644 --- a/api/tests/src/task/notifications/test_generate_notifications.py +++ b/api/tests/src/task/notifications/test_generate_notifications.py @@ -4,7 +4,7 @@ from sqlalchemy import select import tests.src.db.models.factories as factories -from src.adapters.aws.pinpoint_adapter import _get_mock_responses +from src.adapters.aws.pinpoint_adapter import _clear_mock_responses, _get_mock_responses from src.api.opportunities_v1.opportunity_schemas import OpportunityV1Schema from src.db.models.user_models import UserNotificationLog, UserSavedOpportunity, UserSavedSearch from src.task.notifications.generate_notifications import ( @@ -76,6 +76,8 @@ def test_search_notifications_cli( .count() ) + _clear_mock_responses() + result = cli_runner.invoke(args=["task", "generate-notifications"]) assert result.exit_code == 0 From 30dbf89ff6450d082a6c792748f3acc21ae16dee Mon Sep 17 00:00:00 2001 From: Michael Huneke Date: Fri, 7 Feb 2025 09:56:13 -0500 Subject: [PATCH 17/20] Revert relationship to original. update primaryjoin --- api/src/db/models/user_models.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/api/src/db/models/user_models.py b/api/src/db/models/user_models.py index 83b0f0c03..8416029b0 100644 --- a/api/src/db/models/user_models.py +++ b/api/src/db/models/user_models.py @@ -1,7 +1,7 @@ import uuid from datetime import datetime -from sqlalchemy import BigInteger, ForeignKey +from sqlalchemy import and_, BigInteger, ForeignKey from sqlalchemy.dialects.postgresql import ARRAY, JSONB, UUID from sqlalchemy.orm import Mapped, mapped_column, relationship from sqlalchemy.sql.functions import now as sqlnow @@ -32,7 +32,10 @@ class User(ApiSchemaTable, TimestampMixin): linked_external_user: Mapped["LinkExternalUser | None"] = relationship( "LinkExternalUser", - primaryjoin=lambda: LinkExternalUser.user_id == User.user_id, + primaryjoin=lambda: and_( + LinkExternalUser.user_id == User.user_id, + LinkExternalUser.external_user_type == ExternalUserType.LOGIN_GOV, + ), uselist=False, viewonly=True, ) @@ -59,9 +62,7 @@ class LinkExternalUser(ApiSchemaTable, TimestampMixin): ) user_id: Mapped[uuid.UUID] = mapped_column(ForeignKey(User.user_id), index=True) - user: Mapped["User"] = relationship( - "User", primaryjoin=lambda: LinkExternalUser.user_id == User.user_id - ) + user: Mapped[User] = relationship(User) email: Mapped[str] From 2f9208000223f5f7e9f29b96ce0cf3c535bc5d28 Mon Sep 17 00:00:00 2001 From: Michael Huneke Date: Fri, 7 Feb 2025 10:00:40 -0500 Subject: [PATCH 18/20] Update imports --- api/src/db/models/user_models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/db/models/user_models.py b/api/src/db/models/user_models.py index 8416029b0..180cad610 100644 --- a/api/src/db/models/user_models.py +++ b/api/src/db/models/user_models.py @@ -1,7 +1,7 @@ import uuid from datetime import datetime -from sqlalchemy import and_, BigInteger, ForeignKey +from sqlalchemy import BigInteger, ForeignKey, and_ from sqlalchemy.dialects.postgresql import ARRAY, JSONB, UUID from sqlalchemy.orm import Mapped, mapped_column, relationship from sqlalchemy.sql.functions import now as sqlnow From 23adc2dcc8c5d4e209296282a0750f39600b9b8b Mon Sep 17 00:00:00 2001 From: Michael Huneke Date: Fri, 7 Feb 2025 16:16:54 -0500 Subject: [PATCH 19/20] Update ref name --- api/src/db/models/user_models.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/src/db/models/user_models.py b/api/src/db/models/user_models.py index 180cad610..bf4f82f80 100644 --- a/api/src/db/models/user_models.py +++ b/api/src/db/models/user_models.py @@ -30,7 +30,7 @@ class User(ApiSchemaTable, TimestampMixin): "UserSavedSearch", back_populates="user", uselist=True, cascade="all, delete-orphan" ) - linked_external_user: Mapped["LinkExternalUser | None"] = relationship( + linked_login_gov_external_user: Mapped["LinkExternalUser | None"] = relationship( "LinkExternalUser", primaryjoin=lambda: and_( LinkExternalUser.user_id == User.user_id, @@ -42,8 +42,8 @@ class User(ApiSchemaTable, TimestampMixin): @property def email(self) -> str | None: - if self.linked_external_user is not None: - return self.linked_external_user.email + if self.linked_login_gov_external_user is not None: + return self.linked_login_gov_external_user.email return None From 4f62dfde2eae3e9c70fc1359d72fe7fb55f0a933 Mon Sep 17 00:00:00 2001 From: Michael Huneke Date: Fri, 7 Feb 2025 16:27:44 -0500 Subject: [PATCH 20/20] Fix merge --- .../src/task/notifications/test_generate_notifications.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/api/tests/src/task/notifications/test_generate_notifications.py b/api/tests/src/task/notifications/test_generate_notifications.py index acb48d5a5..970a84d65 100644 --- a/api/tests/src/task/notifications/test_generate_notifications.py +++ b/api/tests/src/task/notifications/test_generate_notifications.py @@ -22,6 +22,8 @@ def user_with_email(db_session, user, monkeypatch): factories.LinkExternalUserFactory.create(user=user, email="test@example.com") return user + +@pytest.fixture def setup_search_data(opportunity_index, opportunity_index_alias, search_client): # Load into the search index schema = OpportunityV1Schema() @@ -47,8 +49,7 @@ def test_via_cli(cli_runner, db_session, enable_factory_create, user, user_with_ assert result.exit_code == 0 -def -( +def test_search_notifications_cli( cli_runner, db_session, enable_factory_create,