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

Session cache fix #143

Merged
merged 3 commits into from
Mar 2, 2017
Merged

Session cache fix #143

merged 3 commits into from
Mar 2, 2017

Conversation

pcantrell
Copy link
Contributor

Alex noted a bug where sessions would show as favorited (Minnestar icon next to them) even though the user hadn't favorited them. I believe this fixes the issue.

I also threw in a fix for a server error when logging out redundantly (log out, back, log out again).

@coveralls
Copy link

Coverage Status

Coverage remained the same at 55.638% when pulling 03a0809 on session-cache-fix into b9f0579 on master.

@@ -1,4 +1,4 @@
<%= cache Session.maximum(:updated_at), expires_in: 20.seconds do %>
<%= cache [Session.maximum(:updated_at), current_participant], expires_in: 20.seconds do %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you add a comment here about why we're using current participant as part of the cache key

@@ -1,7 +1,7 @@
<% title('All Sessions') %>
(most recently added first)

<% cache @sessions do %>
<% cache [@sessions, current_participant] do %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Also add a comment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment’s just going to say “fragment contains user-specific info,” which I feel like the cache key itself already conveys. But I’ll add it.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 55.638% when pulling d3699d0 on session-cache-fix into b9f0579 on master.

@jcoyne jcoyne merged commit 2885e02 into master Mar 2, 2017
@pcantrell
Copy link
Contributor Author

Thanks for the speedy turnaround!

@pcantrell pcantrell deleted the session-cache-fix branch March 2, 2017 04:35
@@ -1,4 +1,5 @@
<%= cache Session.maximum(:updated_at), expires_in: 20.seconds do %>
<%# Note that fragment contains user-specific info, must be cached per user %>
<%= cache [Session.maximum(:updated_at), current_participant], expires_in: 20.seconds do %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is the user-specific info? I can't find it. What partial?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here: https://github.com/minnestar/sessionizer/blob/master/src/app/views/sessions/_session.html.erb#L1

attending_class(session) gives results based on the current user.

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.

3 participants