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.

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

Add since=<date> to notifcations fetching #3346

Merged
merged 58 commits into from Sep 29, 2023
Merged

Conversation

CarolineDenis
Copy link
Contributor

Fixes #2606

@CarolineDenis CarolineDenis changed the base branch from production to v7.9-dev August 16, 2023 17:26
@CarolineDenis CarolineDenis removed the request for review from melton-jason August 17, 2023 14:36
@CarolineDenis CarolineDenis marked this pull request as ready for review August 29, 2023 14:01
@CarolineDenis CarolineDenis added this to the 7.9.1 milestone Aug 29, 2023
Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

looking good, though the logic is complicated, and important, thus I would encourage to take a bit of time to write tests for this (Feel free to ask chatgpt or Jason or me for help) - would also be good practice for the future

i.e mock the network requests using overrideAjax(), also mock INITIAL_INTERVAL to replace it with a smaller duration (i.e 1ms - so that the test doesn't take long time to run) - then inspect that the state is updated on first fetch and on second fetch like you would expect

@CarolineDenis CarolineDenis requested a review from a team September 21, 2023 15:13
Base automatically changed from v7.9-dev to production September 25, 2023 18:42
Copy link
Contributor

@grantfitzsimmons grantfitzsimmons left a comment

Choose a reason for hiding this comment

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

@bronwyncombs discovered that the JSON responses were the same as in v7.9.

I found this to be strange, given that when I call the API when entering a valid date, I only see notifications added since that date.

That's when I realized that the notification was fetching notifications since this day, last month. It is one month behind...

https://fwri6123-issue-2606.test.specifysystems.org/notifications/messages/?since=2023-8-27+15%3A37%3A48
image

🤨

Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

looking great!

(except for the fact that older notifications are not fetched - thanks for catching that - that would have not been nice)

Copy link
Contributor

@grantfitzsimmons grantfitzsimmons left a comment

Choose a reason for hiding this comment

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

Screen.Recording.2023-09-29.at.9.16.26.AM.mp4

Audio quality has never been worse, I apologize.

One issue is #4050

the other issue is that all notifications before the current time are lost, even though they are stored in the database.

Copy link
Contributor

@grantfitzsimmons grantfitzsimmons left a comment

Choose a reason for hiding this comment

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

Looks like the issue with all notifications being fetched was fixed after redeploying.
#4050 is still a big prob– the fix might end up changing how this is implemented.

@CarolineDenis CarolineDenis merged commit 98a7f90 into production Sep 29, 2023
9 checks passed
@CarolineDenis CarolineDenis deleted the issue-2606 branch September 29, 2023 15:06
mergeAndSortNotifications(existingNotifications, newNotifications)
);

lastFetchedTimestamp = startFetchTimestamp;
Copy link
Member

Choose a reason for hiding this comment

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

this should probably also be stored in a ref - right now lastFetchedTimestamp value is lost on useEffect re-executions (happens every time the user opens/closes the notifications component)

Copy link
Member

@maxpatiiuk maxpatiiuk Oct 3, 2023

Choose a reason for hiding this comment

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

Longer explanation:

lastFetchedTimestamp stores the timestamp of last fetch
yet this is a variable you created in useEffect
so every time useEffect re-runs, the value of this variable is lost, and new variable is created
and useEffect has [isOpen] dependency array, thus it re-runs every time user opens or closes notifications panel

but it is not right to clear the value of that variable on user open/close of notifications as that shouldn't be related to lastFetchedTimestamp. Instead, lastFetchedTimestamp is related to "notifications" state (since lastFetchedTimestamp roughly describes when notifications state was last updated), so this variable should have the same lifecycle as the "notifications" hook

@maxpatiiuk
Copy link
Member

maxpatiiuk commented Sep 30, 2023

Two issues. Can you please open a small follow-up pr for this?

Also, if needed, I would be happy to explain in more detail why these things are problem - so that hopefully you see these things in the future

@CarolineDenis
Copy link
Contributor Author

Two issues. Can you please open a small follow-up pr for this?

Also, if needed, I would be happy to explain in more detail why these things are problem - so that hopefully you see these things in the future

new issue created called issue-2606-bis

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Make Notifications fetching and stats reporting more efficient
5 participants