-
Notifications
You must be signed in to change notification settings - Fork 21
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 icon for interestgroups in frontpageview #2594
base: master
Are you sure you want to change the base?
Conversation
6902fe4
to
ce88f3f
Compare
5f38ea3
to
e7bcc7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, just a bit of nitpicking then this is good!
I also do agree that this should be a generic endpoint. I do understand that you don't want to introduce more logic, even though I fear that it won't happen if you don't do it now (that might just be a good thing if we don't ever have a need for it though 🤷)
I did think about this a bit as well and came up with a method to do this both generic and fix the "stable" randomness that we need.
I figured that we can generate a hash on the client, f.ex. using their username and the current day/hour/week (however long we want to keep the current values), and send this hash to the endpoint. We can then create a random list if ids, and store it in redis with this hash as a key. Then, we can just fetch the existing list of ids from redis the next time around, and fetch those objects
Not sure if this is needed, but if you do want to this more generic and also simplify the client code, this might be an option.
Also please squash the commits, as the first one does not contribute anything anymore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The new frontpage (webkom/lego-webapp#2311) shows three randomly selected interestgroups I need them:)
Note: This was the only required change in the backend, which is why I'm adding it straight into production