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 statistics pages #851
Add statistics pages #851
Conversation
This pull request introduces 1 alert when merging 29553b5 into cc618eb - view on LGTM.com new alerts:
|
29553b5
to
752ada3
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.
@samuelcambien : I gave this a quick code review today & tested it with the Backend PR (DSpace/DSpace#2782). Overall, the code looks OK, but I'm not able to get it working as expected (it's possible some of the issues are backend bugs...I'm looking into it).
- The site-level statistics page is always empty for me (the page is entirely blank, with just a page heading). Even after browsing around to items, I cannot get anything to appear at http://localhost:4000/statistics/
- The "Statistics" menu item (at the top of each page) appears to be caching its path & not updating as I browse from Site to Community to Collection to Item. If you start at the Site level and browse down, it always remains http://localhost:4000/statistics/ However, if I reload a Collection page in the browser, then that "Statistics" link is updated to the full Collection statistics path.
- On Community, Collection and Item statistics the results shown are always zero. This might be a backend bug though, as it appears that is what is returned by my REST API even though I see successful
viewevents
sent each time I browse to a new object.
I've added a few more comments inline below.
templateUrl: '../statistics-page/statistics-page.component.html', | ||
styleUrls: ['./collection-statistics-page.component.scss'] | ||
}) | ||
export class CollectionStatisticsPageComponent extends StatisticsPageComponent<Collection> { |
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.
Can we move all these new statistics components/pages under src/app/statistics-page
? There's no need for the "+" in the path. See this long standing issue #327
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.
Ok, renamed the directory
|
||
@Injectable() | ||
@dataService(USAGE_REPORT) | ||
export class UsageReportService extends DataService<UsageReport> { |
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.
Please add TypeDocs for this new class and its methods
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.
Added typedocs
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 think that this class has been wrongly created in core/submission
folder, shouldn't it stay in the core/statistics
folder ?
|
||
@typedObject | ||
@inheritSerialization(HALResource) | ||
export class UsageReport extends HALResource { |
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.
Please add TypeDocs for this new class, and the Point
interface in this same file
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.
Added typedocs
329bdc1
to
cb3194c
Compare
Hi Tim, Thanks for your feedback. One way to solve this would be
But I think this would decrease performance, so instead I simply renamed the menu id's so they are all unique :) |
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.
@samuelcambien : I've retested this today after I figured out a more reliable way to generate some viewevents
statistics in the backend PR.
I'm still seeing the following issues:
- The site-level statistics page is still always empty for me (the page is entirely blank, with just a page heading). I cannot get anything to appear at http://localhost:4000/statistics/ I even tried to generate a
viewevent
on the Site object itself by running thiscurl
command:curl -X POST 'http://localhost:8080/server/api/statistics/viewevents' -H "Content-Type:application/json" -H "X-Forwarded-For:23.249.35.27" --data '{"targetId":"6d65c6a2-3fe7-44dd-bacb-79271257c35d", "targetType":"site"}'
I still do not see any data on that main page, even though I can verify (via REST and other UI pages) that I have statistics generated. - The "Statistics" menu item (at the top of each page) appears to be caching its path & not updating as I browse from Site to Community to Collection to Item. I'm not sure of the best way to solve this issue, but I mentioned it to @artlowel , so hopefully he can give you some advice.
The Community/Collection/Item pages appear to work (after I generated some statistics locally). I now see report information pertaining to "Total Visits", "Total visits per month", "Top country views" and "Top city views".
As a sidenote, it looks like there's also a small merge conflict in this PR now.
fbfd494
to
5a3a195
Compare
Codecov Report
@@ Coverage Diff @@
## main #851 +/- ##
==========================================
+ Coverage 79.74% 79.77% +0.03%
==========================================
Files 999 1008 +9
Lines 21875 21993 +118
Branches 2280 2291 +11
==========================================
+ Hits 17444 17546 +102
- Misses 3427 3443 +16
Partials 1004 1004
Continue to review full report at Codecov.
|
c29ac36
to
a7bf1ad
Compare
a7bf1ad
to
c39b7ff
Compare
c39b7ff
to
6ff9429
Compare
Hi Tim,
I've set up the backend branch locally, but I ran into an authorization issue on the site statistics page. Afterwards, I was able to generate some item stats with the request you mentioned (but with uuid of an item).
|
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.
thanks @samuelcambien for this PR
Generally code looks good, I've only added an inline comment.
I've also tested new statistics pages and they seem to work properly.
I've only a comment, it should be nice if the statistic page show a loading spinner while retrieving statistics from server and if any the message No data available
. What happens now is that the page show immediately the No data available
message that is replaced by statistics later
|
||
@Injectable() | ||
@dataService(USAGE_REPORT) | ||
export class UsageReportService extends DataService<UsageReport> { |
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 think that this class has been wrongly created in core/submission
folder, shouldn't it stay in the core/statistics
folder ?
Hi Guiseppe, |
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.
👍 This looks good to me now. I also ran some tests and it appears to be working. (Note though that I hit bug DSpace/DSpace#2965 again in my testing...so, other testers need to be aware you must have a dbfile installed for stats to be logged). I expect we'll want to enhance these (very basic) stats reports, but they are a good initial version
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.
@samuelcambien thanks for addressed my feedback
however making more test, I probably found a bug. Indeed if I show statistics for a community and than trying to show statistics for other community I always get statistics of the first community I've visited. This is valid also between collections and items. Could you check it?
Hi Guiseppe, thanks for pointing this out. |
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.
@samuelcambien thanks for the fix, it works properly now.
@tdonohue I have a suggestion that should be nice to improve in a follow-up PR.
Basically the breadcrumb in the statistics should include also the link of the related dspace objects, this should allow to easily navigate back to the resource for which statistics are being displayed
References
https://github.com/DSpace/Rest7Contract/blob/main/statistics-reports.md
Fixes #731
Description
This PR implements the statistics pages for DSpace 7.
Instructions for Reviewers
List of changes in this PR:
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
yarn run lint
package.json
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.