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.

Sign up for GitHub

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

Jump to bottom

Fix for issue 3074 #641

Merged
merged 2 commits into from May 30, 2012
Merged

Fix for issue 3074 #641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4fb84e1
Fix for issue 3074
talvalin May 22, 2012
aebeaad
Updated as per Campfire/Gcode comments
talvalin May 23, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 15 additions & 2 deletions app/controllers/stats_controller.rb
View file
Open in desktop
Expand Up @@ -25,8 +25,16 @@ def index

# NOTE: Because we are going to be eval'ing the @sort variable later we MUST make sure that its content is
# checked against the whitelist of valid options
sort_options = %w(hits date kudos.count comments.count bookmarks.count subscriptions.count word_count)
@sort = sort_options.include?(params[:sort_column]) ? params[:sort_column] : "hits"
sort_options = ""
@sort = ""
if current_user.preference.hide_hit_counts
sort_options = %w(kudos.count comments.count bookmarks.count subscriptions.count word_count)
@sort = sort_options.include?(params[:sort_column]) ? params[:sort_column] : "kudos.count"
else
sort_options = %w(hits date kudos.count comments.count bookmarks.count subscriptions.count word_count)
@sort = sort_options.include?(params[:sort_column]) ? params[:sort_column] : "hits"
end

@dir = params[:sort_direction] == "ASC" ? "ASC" : "DESC"
params[:sort_column] = @sort
params[:sort_direction] = @dir
Expand All @@ -44,6 +52,11 @@ def index
# @sort above, so this should never contain potentially dangerous user input)
works = work_query.all.sort_by {|w| @dir == "ASC" ? (eval("w.#{@sort}") || 0) : (0 - (eval("w.#{@sort}") || 0).to_i)}

# on the off-chance a new user decides to look at their stats and have no works
if works.blank?
render "no_stats" and return
end

# group by fandom or flat view
if params[:flat_view]
@works = {ts("All Fandoms") => works}
Expand Down
3 changes: 3 additions & 0 deletions app/models/preference.rb
View file
Open in desktop
Expand Up @@ -27,4 +27,7 @@ def fix_time_zone
self.time_zone = try if ActiveSupport::TimeZone[try]
end

def hide_hit_counts
Copy link
Contributor

Choose a reason for hiding this comment

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

Only bit of advice here would be that in Ruby, it's pretty standard for methods that return true or false to end in a question mark, like: def hide_hit_counts? That just makes it a little clearer to someone who's looking at the code that we're checking a value rather than performing an action.

self.try(:hide_all_hit_counts) || self.try(:hide_private_hit_count)
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, and you don't really need .try here - you're calling the methods on self, so you know it exists. try is most useful in situations where you're calling a method on an object that may or may not be nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Elz! I was going to make both of the changes you suggested but you've already committed the changes. Should I make them at a later date?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I figured you were probably asleep, and neither of them is critical, so whenever you get a chance!

end
end
12 changes: 8 additions & 4 deletions app/views/stats/index.html.erb
View file
Open in desktop
Expand Up @@ -17,10 +17,14 @@
<h3 class="heading"><%= ts("Totals") %></h3>
<div class="wrapper">
<dl class="work meta group" role="complementary">
<% [:author_subscriptions, :hits, :kudos, :comments, :bookmarks, :subscriptions, :word_count].each do |stat| %>
<% [:author_subscriptions, :kudos, :comments, :bookmarks, :subscriptions, :word_count].each do |stat| %>
<dt><%= stat.to_s.titleize %></dt>
<dd><%= @totals[stat] %></dd>
<% end %>
<% unless @user.preference.hide_hit_counts %>
<dt><%= ts("Hits") %></dt>
<dd><%= @totals[:hits] %></dd>
<% end %>
</dl>
</div>

Expand All @@ -31,8 +35,8 @@

<ul class="sorting actions" role="menu">
<li><h4 class="heading"><%= ts('Sort By') %></h4></li>
<li><%= sort_link ts("Date"), "date" %></li>
<% unless @user.preference.try(:hide_all_hit_counts) %>
<% unless @user.preference.hide_hit_counts %>
<li><%= sort_link ts("Date"), "date" %></li>
<li><%= sort_link ts('Hits'), "hits", {:desc_default => true} %></li>
<% end %>
<li><%= sort_link ts('Kudos'), "kudos.count", {:desc_default => true} %></li>
Expand Down Expand Up @@ -70,7 +74,7 @@
<% if (work_subscriber_count = Subscription.where(:subscribable_id => work.id, :subscribable_type => 'Work').count) > 0 %>
<dt><%= ts("Subscribers") %></dt><dd><%= work_subscriber_count %></dd>
<% end %>
<% unless @user.preference.try(:hide_all_hit_counts) %>
<% unless @user.preference.hide_hit_counts %>
<dt><%= ts("Hits") %></dt><dd><%= work.hits %></dd>
<% end %>
<dt><%= ts("Downloads") %></dt><dd><%= work.downloads %></dd>
Expand Down
3 changes: 3 additions & 0 deletions app/views/stats/no_stats.html.erb
View file
Open in desktop
@@ -0,0 +1,3 @@
<h2 class="heading"><%= ts("Statistics") %></h2>
<p><%= ts("You currently have no works posted to the archive. If you add some, you'll find information on this page about hits, kudos, comments, bookmarks and downloads of your works.") %></p>
<p><%= ts("Users can also see how many subscribers they have, but not the names of their subscribers or identifying information about other users who have viewed or downloaded their works.") %></p>