-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Issue #3538] Modify the backend notification script to handle saved searches #3774
[Issue #3538] Modify the backend notification script to handle saved searches #3774
Conversation
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For grouping like-queries, we should drop the pagination part of the search object first - it'll be overriden by the search_opportunities_id
method anyways to a static value.
This lets two users who both saved the same thing with different sorts together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a util to cover this. I think this would cover the scenario where we remove the pagination.
# Run the notification task | ||
result = cli_runner.invoke(args=["task", "generate-notifications"]) | ||
assert result.exit_code == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can run these with the task itself, that would give you more direct access to checking the map if we wanted to verify anything in that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I adapted this over.
Co-authored-by: Michael Chouinard <46358556+chouinar@users.noreply.github.com>
assert saved_search2.last_notified_at > datetime_util.utcnow() - timedelta(minutes=1) | ||
|
||
|
||
def test_search_notifications_on_index_change( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chouinar any idea why these tests all pass locally but fail when running in CI/CD? Maybe running the full suite puts the db/search index in a failing state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, worked around this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops - thought I approved this one yesterday - LGTM!
Summary
Fixes #3538
Time to review: 20 mins
Changes proposed
Update
generate_notifications
task to use saved searches. Run those saved searches against the search index to compare opportunity ID changes.Context for reviewers
The job does not actually notify. That will be done in a follow-up ticket.
Additional information
See attached unit tests