Conversation
✅ Deploy Preview for everse-rsqkit-testing ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
shraddha-bajare
changed the title
✅ Deploy Preview for rsqkit-testing ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
sparkslabs
left a comment
sparkslabs
left a comment
There was a problem hiding this comment.
Recommend merge. :-)
Lots of little comments inline (hopefully useful - some feel a bit nit-picky). Some actionable, some to consider if you agree or not.
If you agree with them, I'd suggest a follow on PR for the minor changes, rather than block this PR, and merge this PR for this release.
| title: Site Statistics | ||
| --- | ||
|
|
||
| This page provides an overview of how the site is used and accessed. The statistics are collected using Matomo, an open-source analytics platform designed with privacy in mind. |
There was a problem hiding this comment.
I wasn't aware of Matamo before this PR. As a result, I googled Matamo, and discovered that while the code is open source the hosting is a paid for platform. It took a fair amount of digging around to identify that we're using a hosted instance of Matamo. (hosted by one of the partners but hosted nonetheless)
As a result, I think an extra sentence to state that we are using a non-commercial/private instance of Matamo - rather than the commercial service - would be a good idea)
There's several reasons:
- Clarity on "what data is going where and being processed where" . (Someone who looks will care and appreciate the detail
- Makes it clearer that we have an extra dependency - and noting who owns it. (for when we need to maintain this)
This can be done as a follow on PR.
| import requests | ||
| from datetime import date | ||
|
|
||
| MATOMO_URL = os.environ.get("MATOMO_URL", "https://matomo.research.software/") |
There was a problem hiding this comment.
A comment here (or somewhere) to state who "owns" https://matomo.research.software/ would be a good idea and who to contact if there's a problem would be useful. (I mistakenly assumed this was run by Matamo, not by who actually runs it)
| from datetime import date | ||
|
|
||
| MATOMO_URL = os.environ.get("MATOMO_URL", "https://matomo.research.software/") | ||
| SITE_ID = os.environ.get("MATOMO_SITE_ID", "7") |
There was a problem hiding this comment.
Similar - about where the 7 comes from.
| return data | ||
|
|
||
|
|
||
| def fetch_live(): |
There was a problem hiding this comment.
Nitpick -
fetch_live ... what?
I'd probably call this "fetch_live_stats_from_matamo"
| } | ||
|
|
||
|
|
||
| def api(method, extra=""): |
There was a problem hiding this comment.
General/nitpick - I tend to expect function names to be active. (Methods can be passive if they're returning data, but I still active methods to be active). something like "call_api" - or even "call_matamo_api"
(not a blocker, more an observation)
| ], | ||
| "referrers": [ | ||
| {"type": r["label"], "visits": r["nb_visits"]} | ||
| for r in (referrers if isinstance(referrers, list) else []) |
There was a problem hiding this comment.
This - (referrers if isinstance(referrers, list) else [] is convoluted here. I'd personally put that above, directly below
referrers = api(
"Referrers.getReferrerType",
"&period=range&date=last365"
)
ie something more like
referrers = call_matamo_api(
"Referrers.getReferrerType",
"&period=range&date=last365"
)
if not referrers:
referrers = []
Similar comment for countries.
| output = DUMMY_DATA | ||
|
|
||
| out_path = os.path.join( | ||
| os.path.dirname(__file__), "../../_data/matomo_stats.json" |
There was a problem hiding this comment.
A comment here as to why this is writing upwards in the tree would be welcome. ( I get the reason, but seeing ".." in scripts always makes me pause :-) )
| - name: Install dependencies | ||
| run: pip install requests | ||
|
|
||
| - name: Fetch Matomo Stats |
There was a problem hiding this comment.
I would suggest renaming this to include where the matamo stats are likely pulled from ("Fetch Matamo Stats from eScience Center NL's servers"). Or probably better: just putting that as a comment. (YAML allows comments after all).
4 participants
Fixes #588 #615
Changes proposed in this pull request:
Approach
Fetch data at build time from Matomo APIs via a python script, write it to a
_data/matomo_stats.jsonfile, then render it on a new RSQKit page -Site Statisticsvia Liquid templates.Schedule the run for monthly stats.
It is consistent with how RSQKit already uses _data/ for YAML data files.
Env variables to set :
MATOMO_URL,SITE_ID,TOKENImplementation
Site StatisticsNotes for reviewers:
For now I have not set TOKEN, so for testing I have fed stats default values to show. Once I get token, real stats values would start appearing on Site Statistics page.