Moved analytics config into the database and added it to the admin interface #551
Conversation
… into CirculationManager as much as possible.
| @@ -117,7 +117,7 @@ def log_import(integration_or_setting): | |||
|
|
|||
| library_url = adobe_conf.get('library_uri') | |||
| ConfigurationSetting.for_library( | |||
| Library.WEBSITE_KEY, library).value = library_url | |||
| Configuration.WEBSITE_URL, library).value = library_url | |||
There was a problem hiding this comment.
I noticed this wasn't working.
|
|
||
| integration = EI(protocol=u"api.google_analytics_provider", goal=EI.ANALYTICS_GOAL) | ||
| _db.add(integration) | ||
| integration.url = "http://www.google-analytics.com/collect" |
There was a problem hiding this comment.
The url wasn't part of the configuration before, but I think it should be.
There was a problem hiding this comment.
Agreed. This would let us hook it up to the GA proxy or similar.
api/google_analytics_provider.py
Outdated
| integration = get_one(_db, ExternalIntegration, id=self.integration_id) | ||
| tracking_id = ConfigurationSetting.for_library_and_externalintegration( | ||
| _db, self.TRACKING_ID, library, integration, | ||
| ).value |
There was a problem hiding this comment.
The tracking id is now a per-library setting, so different libraries can set up their own Google Analytics accounts if they want to. I have not yet done anything to add the library to the info in the analytics events.
There was a problem hiding this comment.
I think you'd get better performance (and simplify the API somewhat) if you followed the model we use for Authenticator. That is, create a separate LibraryAnalytics object for every Library and have Analytics keep them in a dictionary keyed by ID. Is that feasible?
I think you could also avoid a lot of changes to method signatures if Library had a .analytics that accessed a cached-on-demand LibraryAnalytics object.
There was a problem hiding this comment.
It's a little different than Authenticator, because not all the Analytics integrations are associated with a Library. The LocalAnalyticsProvider is not - once you turn it on it stores events for all libraries in the db.
The comment also made me realize I'm currently calling all the analytics providers for every event, even though some of them might not be set up for every Library.
There was a problem hiding this comment.
I changed Analytics to keep a dictionary of providers by library id, as well as a list of sitewide providers. I didn't create a LibraryAnalytics class since there wasn't anything for it to do except call the provider.
| self._db, license_pool, CirculationEvent.DISTRIBUTOR_AVAILABILITY_NOTIFY, license_pool.last_checked) | ||
| for library in self.collection.libraries: | ||
| self.analytics.collect_event( | ||
| library, license_pool, CirculationEvent.DISTRIBUTOR_TITLE_ADD, license_pool.last_checked) |
There was a problem hiding this comment.
All these places that were calling Analytics.collect_event are missing test coverage.
Also, for some reason this one was using the wrong event type. I think AVAILABILITY_NOTIFY is when a patron has something on hold and it becomes available for checkout. I'm not completely sure what's going on here, but TITLE_ADD is usually the event type for a new license pool.
There was a problem hiding this comment.
Yes, this should be TITLE_ADD. Sounds like someone was thinking the "availability" meant availability in general.
|
👍 |
This fixes #539
It uses NYPL-Simplified/circulation-web#93 and NYPL-Simplified/server_core#551
It moves the analytics configuration into the database, and changes Analytics to be initialized by the CirculationManager, instead of a global singleton. When scripts need it, they have to create their own. It also adds an admin route for editing analytics configuration.