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

Issue-361 : Implement Caching with Redis #1163

Merged
merged 13 commits into from
Mar 24, 2025

Conversation

rishabmamgai
Copy link
Contributor

Contributor checklist

Description

  • The PR implements per_view caching with Redis for all the viewsets used to list/retrieve the details.
  • Tested caching mechanism and invalidation through API calls.

Related issue

Copy link

netlify bot commented Mar 17, 2025

Deploy Preview for activist-org canceled.

Name Link
🔨 Latest commit 85ac00a
🔍 Latest deploy log https://app.netlify.com/sites/activist-org/deploys/67e1ba8c6415720008bef473

Copy link
Contributor

Thank you for the pull request! ❤️

The activist team will do our best to address your contribution as soon as we can. If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Development rooms once you're in. Also consider attending our bi-weekly Saturday developer syncs! It'd be great to meet you 😊

Copy link
Contributor

github-actions bot commented Mar 17, 2025

Maintainer Checklist

The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

  • The TypeScript, pytest and formatting workflows within the PR checks do not indicate new errors in the files changed

  • The Playwright end to end and Zap penetration tests have been ran and are passing (if necessary)

  • The changelog has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

First PR Commit Check

  • The commit messages for the remote branch of a new contributor should be checked to make sure their email is set up correctly so that they receive credit for their contribution
    • The contributor's name and icon in remote commits should be the same as what appears in the PR
    • If there's a mismatch, the contributor needs to make sure that the email they use for GitHub matches what they have for git config user.email in their local activist repo (can be set with git config --global user.email "GITHUB_EMAIL")

@andrewtavis
Copy link
Member

Would be great if you could check the backend workflow errors and do the edits to your files to add the license headers so that the license header workflow is fixed. All files need a machine readable one line comment that indicates what the license is.

Please let us know if you have any questions!

@rishabmamgai
Copy link
Contributor Author

I have fixed most of it. Any suggestions for ruff unused imports in signals.py. Meanwhile I'll try to work on units tests.

@andrewtavis
Copy link
Member

For those you should be able to just remove the packages that aren't used, but are being imported.

@rishabmamgai
Copy link
Contributor Author

Sorry my bad, it's in apps.py. The import is just made to load the signals for the app but not being used in apps.py.

@andrewtavis
Copy link
Member

Should be good to remove :)

@andrewtavis
Copy link
Member

The merge of main should fix many of the tests, @rishabmamgai :) Let's work to figure out any of the remaining issues 😊

@andrewtavis
Copy link
Member

Would be great if you could pull the most recent changes and then fix the license header issues as well as the ruff and mypy errors, @rishabmamgai :) See this comment above. We can work on the failing tests after that.

@rishabmamgai
Copy link
Contributor Author

Should be good to remove :)

Removing it will not register the cache invalidation signals. But # noqa works, helps in suppressing the warning.

Copy link
Member

@andrewtavis andrewtavis left a comment

Choose a reason for hiding this comment

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

@rishabmamgai Ultimately I owe you an apology here as the main maintainer. In discussing these changes in the most recent developer sync we made the decision that now is not the time to add in Redis. Ultimately the service was seen to be too much for a yet to be released platform, and overhead that we as a community can't afford to maintain right now.

The decision was made to document that this implementation was done in the now included Markdown file that will point to this PR as the basis of the eventual implementation of Redis. We will also commit the changes directly so that you get credit for all of the work that you put into this PR.

Again my apologies that your changes will not be integrated into the platform. I should have groomed the issues better and communicated with the community earlier when this level of work began.

You'd be more than welcome to continue to contribute. Really do appreciate the detail and persistence that you put into this PR. Hard to not merge it in, but ultimately we did need to make this decision.

Really thanks for the work here ❤️

@andrewtavis andrewtavis merged commit d039e5c into activist-org:main Mar 24, 2025
7 checks passed
@rishabmamgai
Copy link
Contributor Author

Hi @andrewtavis , thanks for the update. I was curious about if the build will run successfully after my last changes. Anyways, hope Redis will be there someday, will be happy to pick it up when it's finally a go go again :).

@andrewtavis
Copy link
Member

I'll definitely reach out when we're ready to work on this again, @rishabmamgai! Hopefully within the year we'll start adding further systems on to it all 😊 Please let us know if there's another issue that'd be of interest. Would be very happy to be able to work with you again :)

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

Successfully merging this pull request may close these issues.

2 participants