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

Feature Request: Set an expiration date on texterroles Redis keys #2278

Open
crayolakat opened this issue Apr 10, 2023 · 4 comments
Open

Feature Request: Set an expiration date on texterroles Redis keys #2278

crayolakat opened this issue Apr 10, 2023 · 4 comments
Assignees
Labels
C-enhancement Type: enhancement

Comments

@crayolakat
Copy link
Collaborator

Problem
The texterroles Redis keys don't have an expiration date and stay in the cache forever.

Solution
It would be nice to have an expiration date so that old and outdated texterroles will get purged. Maybe 365 days?

@crayolakat crayolakat added the C-enhancement Type: enhancement label Apr 10, 2023
@Arique1104
Copy link
Collaborator

Hey @crayolakat,

Where would this edit need to be? @ninesky00 is interested in working on this as a Backend Engineer. Got any pointers on where he could start?

@Arique1104 Arique1104 added this to To do in Spoke Community Coalition via automation May 4, 2023
@crayolakat
Copy link
Collaborator Author

@Arique1104 @ninesky00 Sorry I was on vacation and just got back today! Let me dig around and get back to you

@crayolakat
Copy link
Collaborator Author

The changes would be in the file src/server/models/cacheable_queries/user.js

If you look at line 124-129, you will see that the texterauth keys are set to expire after 12 hours (43,200 seconds):
https://github.com/MoveOnOrg/Spoke/blob/4538f006b7be658d378d09b5cbc33f5db15479b0/src/server/models/cacheable_queries/user.js#L124-L129

Lines 82-86 is where the texterroles keys are set:
https://github.com/MoveOnOrg/Spoke/blob/4538f006b7be658d378d09b5cbc33f5db15479b0/src/server/models/cacheable_queries/user.js#L82-L86

Notice that there is no .expire(...) method called in lines 82-86. For this feature request, you would need to add an .expire(...) to lines 82-86, similar to how it is in lines 124-L129.

@ninesky00
Copy link
Collaborator

ninesky00 commented Sep 1, 2023

Hello @crayolakat @Arique1104, sorry it took me so long to get to this. I made the following change a while ago.
Screen Shot 2023-08-31 at 10 30 29 PM
However, I was trying to write a test for it. After some digging around, I wasn't able to find any tests for Redis in user.js, texter.test/texter.test.js, cacheable_queries/user.test.js, or test/test_helpers.js. I'm wondering if I should proceed as is. Also I was thinking if there is a necessity to expire the existing cache that currently has no expiration with EXPIRE NX -- Set expiry only when the key has no expiry or to override them with EXPIRE GT -- Set expiry only when the new expiry is greater than current one. Please let me know if you get a chance, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Type: enhancement
Development

No branches or pull requests

3 participants