Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update the fetch for notifications to take the correct datetimes into account #2302

Merged
merged 2 commits into from
Sep 26, 2024
Merged

Update the fetch for notifications to take the correct datetimes into account #2302

merged 2 commits into from
Sep 26, 2024

Conversation

jzbahrai
Copy link
Collaborator

@jzbahrai jzbahrai commented Sep 25, 2024 •
edited
Loading

Summary | Résumé

  • We want query the facts table for 6 days prev to the current day
  • We want to query notifications for todays data. due to timezones/ and when the job runs to populate facts, we will take the max existing date in facts from the last 6 days and use that as a starting point to query notifications for
  • We added a filter for notification_type as there is a part in the frontend that fetches this query only for a singular notification type

Related Issues | Cartes liées

Tests:

Updated and added unit tests

@jzbahrai jzbahrai force-pushed the task/fix-query branch 2 times, most recently from f06bc14 to e471d38 Compare September 25, 2024 20:50
andrewleith
andrewleith approved these changes Sep 26, 2024
View reviewed changes
Copy link
Member

@andrewleith andrewleith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I like the way you've refactored things, it is much easier to follow 🏅

I committed 2 extra tests to help cover more scenarios!

tests/app/dao/test_fact_notification_status_dao.py Outdated Show resolved Hide resolved
@jzbahrai
Change the datetimes
194b06f 
- We want query the facts table for 6 days prev to the current day
- We want to query notifications for todays data. due to timezones/ and
  when the job runs to populate facts, we will take the max existing
  date in facts from the last 6 days and use that as a starting point to
  query notifications for
- We added a filter for notification_type as there is a part in the
  frontend that fetches this query only for a singular notification type
@jzbahrai
Pr comment
577bc68
@jzbahrai jzbahrai merged commit 53a413e into main Sep 26, 2024
4 checks passed
@jzbahrai jzbahrai deleted the task/fix-query branch September 26, 2024 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewleith andrewleith andrewleith approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jzbahrai @andrewleith