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

Delete notifications older than 90 days #1328

Conversation

yaswanthsaivendra
Copy link
Contributor

@yaswanthsaivendra yaswanthsaivendra commented May 30, 2023

Proposed Changes

  • created a periodic task to delete notifications that are older than 90 days.

Associated Issue

Architecture changes

@coronasafe/code-reviewers

Merge Checklist

  • Tests added/fixed
  • Update docs in /docs
  • Linting Complete

@yaswanthsaivendra yaswanthsaivendra requested a review from a team as a code owner May 30, 2023 12:31
@sonarcloud
Copy link

sonarcloud bot commented May 30, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@yaswanthsaivendra
Copy link
Contributor Author

@rithviknishad , @vigneshhari
Please review!!

@vigneshhari
Copy link
Member

Can you create a test for this as well? Once you have the tests added we can merge.

@yaswanthsaivendra
Copy link
Contributor Author

Can you create a test for this as well? Once you have the tests added we can merge.

Yes, added the tests. Please check
Yesterday for the first I got to know about mocking . So Can u share ur opinion on mocking database interactions. Is it good or bad. but I observed that it decreases the run time of tests alot.

@vigneshhari
Copy link
Member

You could have gotten away without mocking in these tests by using the update method of the django queryset ( they do not invoke the save methods and the created timestamps can be added manually "to my knowledge" ), even in this case, we are not mocking the database interactions, we are just mocking the time method.
In almost all cases we would not want to mock the database, the database is what we are usually testing against. mocking is useful when there is an external service involved, like an SMS API or an integration with an API, in our unit tests we mock all the responses they can give ( success, failure, exception ) and test if the function we wrote handles every case correctly.

@vigneshhari vigneshhari merged commit c282517 into coronasafe:master Jun 8, 2023
2 checks passed
@yaswanthsaivendra
Copy link
Contributor Author

You could have gotten away without mocking in these tests by using the update method of the django queryset ( they do not invoke the save methods and the created timestamps can be added manually "to my knowledge" ), even in this case, we are not mocking the database interactions, we are just mocking the time method. In almost all cases we would not want to mock the database, the database is what we are usually testing against. mocking is useful when there is an external service involved, like an SMS API or an integration with an API, in our unit tests we mock all the responses they can give ( success, failure, exception ) and test if the function we wrote handles every case correctly.

Yaa ik that i haven't mocked any database interactions in the pushed code . I just asked it coz I first wrote the test by mocking database interactions, later i realised that isn't a gud idea. So i changed it. Thanks for sharing ur opinion. 😄

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

Successfully merging this pull request may close these issues.

Periodically hard delete Notification records that are older than 90 days.
4 participants