ckan / ckanext-googleanalytics

CKAN extension to integrate Google Analytics data into CKAN. Gives download stats on package pages, list of most popular packages, etc.
GNU Affero General Public License v3.0
35 stars 81 forks source link

Extension does not respect custom uploader implementations #28

Closed gleb-rudenko closed 7 years ago

camfindlay commented 7 years ago

Can you explain the issue this solves in a bit more detail? :)

gleb-rudenko commented 7 years ago

@camfindlay , here is the issue https://github.com/ckan/ckanext-googleanalytics/issues/16

So basically, consider we added googleanalytics extension into our .ini file. Now when you upload a file into a resource people would be able to download the resource, all cool, google-analytics tracking the download and you can see this info in your GA acc. Works like a charm :)

Now, what if we want to modify the uploader, by enabling ANY custom uploader extension. For example: s3filestore and you set extensions order like this: s3filestore googleanalytics Still ppl can download newly uploaded resources with our custom uploader, BUT googleanalytics stops tracking these download! (First issue).

But what if we want to change the order? like this googleanalytics s3filestore

Google analytics will start to track again! BUT, now when you try to download the file (uploaded by custom uploader) you will get 404! (Second issue)

That's because googleanalytics ignores custom uploader and their route mapping you can take a look at current resource_download method: https://github.com/ckan/ckanext-googleanalytics/blob/master/ckanext/googleanalytics/controller.py#L143

As you can see, it always uses PackageController.resource_download, no matter what.

So i forced googleanalytics to look at route's controller in the first place and do stuff with it.

I hope it helped :)

smotornyuk commented 7 years ago

It looks slightly cumbersome, but i see three points for accepting:

  1. It not worse that original code
  2. It fixes problem
  3. It works(as i checked)

So if there are any other suggestions - anyone can express them in PR over this. Merged