Log downloads #1533

cldambrosio · 2018-06-11T10:46:36Z

Logging and displaying download statistics independently from Google Analytics

Refs Hyrax#2539

Changes proposed in this pull request:

In the FileDownloadStats database table, add a title and change the date field to an array; to enable filtering the data by Work and by Date.
Create and update records when a file is downloaded
Render these stats in the dashboard

@samvera/hyrax-code-reviewers

change db table to prep for report filtering remove admin_stats_presenter_decorator correct title field

jcoyne · 2018-06-11T14:02:02Z

app/decorators/controllers/hyrax/downloads_controller_decorator.rb

@@ -0,0 +1,30 @@
+module Hyrax
+  DownloadsController.class_eval do


This is an unusual way to do this. I like to do this same sort of thing by making a module and include it in the DownloadsController

in config/initializers/hyku.rb (or something) add:
Hyrax::DownloadsController.include DownloadsControllerDecorator

Sounds good, I'll try this 👍

jcoyne · 2018-06-11T14:04:59Z

app/decorators/controllers/hyrax/downloads_controller_decorator.rb

+      update_download_stats(asset)
+    end
+
+    def download_stats(current_file)


Because these methods have nothing to do with HTTP interaction they could move out of the controller to a service class, so that the only addition on line 6 needs to be:

DownloadStats.update(asset)

jcoyne · 2018-06-11T14:06:08Z

app/decorators/controllers/hyrax/downloads_controller_decorator.rb

+
+    def download_stats(current_file)
+      if FileDownloadStat.find_by(file_id: current_file.id)
+        FileDownloadStat.find_by(file_id: current_file.id)


One common practice to avoid long if/else blocks is do to:

stat = FileDownloadStat.find_by(file_id: current_file.id) return stat if stat ... (the stuff that was in the else block)...

jcoyne · 2018-06-11T14:08:17Z

app/decorators/controllers/hyrax/admin/stats_controller_decorator.rb

+  Admin::StatsController.class_eval do
+    def show
+      super
+      @works_to_display ||= ::FileDownloadStat.all.select do |e|


This will load all database records into memory and iterate over them trying to find records that match this condition. But if you do ::FileDownloadStat.where(user_id: current_user.id) then the database will do all the work and you'll only load into memory the records you care about.

💯 thanks again!!

jcoyne · 2018-06-11T14:09:25Z

app/decorators/controllers/hyrax/admin/stats_controller_decorator.rb

+      @works_to_display ||= ::FileDownloadStat.all.select do |e|
+        e[:user_id] == current_user.id
+      end
+      @total_downloads ||= @works_to_display.map(&:downloads).reduce(:+)


why are you using ||= in this method? Is there somewhere else that could have already set @total_downloads? If so, can you add a comment about where that might occur? Otherwise just use simple assignment (=)

jcoyne · 2018-06-11T14:10:26Z

app/views/hyrax/admin/stats/_downloads.html.erb

+<h2>Downloads per Work</h2>
+<ul>
+  <% @works_to_display.each do |dwnld| %>
+    <li><%= dwnld.title %></li><span class="count">(<%= dwnld.downloads %>)</span>


This markup seems odd. I'm not sure ul can have any children besides li. Did you want to tuck the span within the li?

Yes span within li is nicer thanks. I'll see how else we may want this styled.

jcoyne · 2018-06-11T14:15:03Z

db/migrate/20180606101810_change_file_download_stats_date_to_array.rb

+class ChangeFileDownloadStatsDateToArray < ActiveRecord::Migration[5.1]
+  def change
+    remove_column :file_download_stats, :date, :datetime
+    add_column :file_download_stats, :date, :datetime, array: true, default: []


I think this change may end up breaking this code: https://github.com/samvera/hyrax/blob/be223ff41debc86bac9a5e74ac4151102513e9ac/app/models/hyrax/statistic.rb#L53

Are you sure you don't want to just make a new database table rather than trying to reuse one?

Good catch thank you! We think we won't use GA for our fork but better make a change in line with this version. I'll follow your suggestion.

cldambrosio · 2018-06-11T16:00:28Z

I'll work on it again tomorrow. @jcoyne thanks a ton for helping me improve this 👍

Update `title` of WorkDownloadStat if Work is edited Minor change in display

cldambrosio · 2018-06-25T09:41:42Z

Style changed to:

orangewolf · 2018-06-27T23:24:33Z

Note the 4 failures here are the same 4 from @cjcolvar experiments in #1523 and #1522. They appear to be a problem with forks and not a real code change issue.

orangewolf · 2018-10-10T20:19:41Z

@cldambrosio looks like this needs to have master merged in to it and that will likely help with these test failures.

this review is stale

cldambrosio · 2018-10-12T08:06:26Z

Thank you @orangewolf I will do that soon!

…g-downloads

log downloads

a7e917a

change db table to prep for report filtering remove admin_stats_presenter_decorator correct title field

jcoyne previously requested changes Jun 11, 2018

View reviewed changes

code review improvements

1ac0419

Create database table separate from existing GA stats

54fc746

cldambrosio force-pushed the log-downloads branch 3 times, most recently from 568dbcb to d93ff5a Compare June 18, 2018 15:01

Move logic to model

b28522e

Update `title` of WorkDownloadStat if Work is edited Minor change in display

cldambrosio force-pushed the log-downloads branch from d93ff5a to b28522e Compare June 18, 2018 15:19

Unit tests for WorkDownloadStat model

b8f026e

cldambrosio changed the title ~~[WIP] log downloads~~ Log downloads Jun 25, 2018

cldambrosio mentioned this pull request Jun 25, 2018

Log downloads ubiquitypress/hyku#9

Merged

Merge branch 'master' into log-downloads

eb1350e

cldambrosio added 2 commits October 30, 2018 16:55

Merge branch 'master' of https://github.com/samvera-labs/hyku into lo…

1f2ae05

…g-downloads

Merge branch 'log-downloads' of github.com:ubiquitypress/hyku into lo…

81bfea4

…g-downloads

mankind deleted the log-downloads branch January 18, 2019 09:28

orangewolf closed this Jul 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log downloads #1533

Log downloads #1533

cldambrosio commented Jun 11, 2018 •

edited

Loading

jcoyne Jun 11, 2018

cldambrosio Jun 11, 2018

jcoyne Jun 11, 2018

jcoyne Jun 11, 2018

cldambrosio Jun 11, 2018

jcoyne Jun 11, 2018

cldambrosio Jun 11, 2018

jcoyne Jun 11, 2018

jcoyne Jun 11, 2018

cldambrosio Jun 11, 2018

jcoyne Jun 11, 2018

cldambrosio Jun 11, 2018

cldambrosio commented Jun 11, 2018

cldambrosio commented Jun 25, 2018

orangewolf commented Jun 27, 2018

orangewolf commented Oct 10, 2018

cldambrosio commented Oct 12, 2018

		@@ -0,0 +1,30 @@
		module Hyrax
		DownloadsController.class_eval do

Log downloads #1533

Log downloads #1533

Conversation

cldambrosio commented Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cldambrosio commented Jun 11, 2018

cldambrosio commented Jun 25, 2018

orangewolf commented Jun 27, 2018

orangewolf commented Oct 10, 2018

cldambrosio commented Oct 12, 2018

cldambrosio commented Jun 11, 2018 •

edited

Loading