-
Notifications
You must be signed in to change notification settings - Fork 124
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
Analytics #5089
Analytics #5089
Conversation
Matomo:: Downloads See merge request notch8/od_analytics!4557
I have reviewed how this is functioning on the two Notch8 demo sites with Google Analytics and Matomo providing data and I think the visualization of this data with the adjustable date ranges is definitely an improvement over the analytics view provided right now out of the box in Hyrax. Both of those sites appear to be running Hyrax 3.0.2 so it is not the latest release (3.1.0) or code from the main branch. I haven't been able to test this running Hyrax locally since I don't have a GA account or Matomo account to connect for testing. |
end | ||
alias google_analytics_id? google_analytics_id | ||
|
||
# Defaulting analytic start date to whenever the file was uploaded by leaving it blank | ||
attr_writer :analytic_start_date |
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.
what's the story with these? are they replaced by the newer #analytics_start_date
?
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.
I didn't want to remove any existing code because I was afraid it would break for people with google analytics already installed and working in their applications. I am not totally clear on what is needed for that to work, and what is not.
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.
We discussed keeping the existing statistics code side by side with the new analytics code because although the new analytics code has a way better dashboard experience and fixes the views and downloads on works and filetypes, it does not do anything for the user statistics found in the user profile and there is still some unique value in the statistics dashboard option.
Since they both essential run off of the same database entries (populated by provider) they shouldn't be in conflict with one another. I am testing this theory this week, since we have statistics working here at UC.
So it is is the recommendation of the tech call attendees to merge in the analytics PR and create a separate issue for removing the statistics, updating the user profile stats, and consolidating the dashboard views.
end | ||
|
||
def paginate(results_array, rows: 10) | ||
return if results_array.nil? |
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.
does results_array
sometimes return nil
? could we make it give an empty array in this case instead?
Co-authored-by: tamsin johnson <tomjohnson@ucsb.edu>
Co-authored-by: tamsin johnson <tomjohnson@ucsb.edu>
Co-authored-by: tamsin johnson <tomjohnson@ucsb.edu>
app/controllers/hyrax/admin/analytics/collection_reports_controller.rb
Outdated
Show resolved
Hide resolved
Co-authored-by: tamsin johnson <tomjohnson@ucsb.edu>
Co-authored-by: tamsin johnson <tomjohnson@ucsb.edu>
Co-authored-by: tamsin johnson <tomjohnson@ucsb.edu>
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.
After reading through the code and discussing it with the Samvera Tech call, I think we can merge this Analytics PR without removing the Statistics code. Although they both work from the same data, they are both doing different jobs.
`@collections` cannot be set with `Collection.order` when `Collection` is `Hyrax::PcdmCollection`. The `order` method is not defined. Use of `all` was suggested, but it is also not defined for `Hyrax::PcdmCollection`. There is an alternative approach to get all collections, but it is not clear that this is necessary. * `@collections` does not appear to be used anywhere (added in PR #5089) which was the PR that added the new Analytics * `@collections` does not appear to have been used prior to this PR in any `views` or `presenters` * getting all collections is very expensive and should be avoided unless really necessary * if collections are required, it is better to get them from solr instead of the persistent store. This will also enforce permissions if search builders are used. For all these reasons, setting `@collections` is being removed.
`@collections` cannot be set with `Collection.order` when `Collection` is `Hyrax::PcdmCollection`. The `order` method is not defined. Use of `all` was suggested, but it is also not defined for `Hyrax::PcdmCollection`. There is an alternative approach to get all collections, but it is not clear that this is necessary. * `@collections` does not appear to be used anywhere (added in PR #5089) which was the PR that added the new Analytics * `@collections` does not appear to have been used prior to this PR in any `views` or `presenters` * getting all collections is very expensive and should be avoided unless really necessary * if collections are required, it is better to get them from solr instead of the persistent store. This will also enforce permissions if search builders are used. For all these reasons, setting `@collections` is being removed.
Fixes #4564 #4530 #3551 #2542 #2566 Maybe others?
This work was sponsored by Oregon Digital as part of the IMLS National Leadership Grant to support its project, Open Impact: Developing Robust Analytics for Open Source Solution Bundle Hyrax.
This code overhauls the built in display of reports and metrics for Hyrax. It makes it compatible with both GA v3 and Matomo and lays ground work for other adapters in the future.
Changes proposed in this pull request: