search

tomdyson / wagalytics

A Google Analytics dashboard in your Wagtail admin
MIT License
217 stars 42 forks source link

Set Up Handling for Wagalytics Multisite #35

Closed jake-kent closed 4 years ago

jake-kent commented 5 years ago

@tomdyson I'd appreciate your feedback on this PR. Please let me know if there's another way you'd approach this or alternatively any blockers you see for merging this into the next release.

KalobTaulien commented 4 years ago

Hey @jake-kent

Thanks for your contribution!

I opened a new PR for some extended support for multiple GA instances, see #39 for my implementation. I found a lot of inspiration in your code so I've added you to the Contributors list in the README file

Once #39 is merged in I'll close this PR. Thanks again for the awesome work you did in here!

jake-kent commented 4 years ago

Hey @KalobTaulien

Understood, thanks for pulling in some of my code to the new and adding me to the contributors list.

I'd be curious to hear why you chose to have the sites be declared manually in a settings file rather than be data-driven (set through the CMS). Is it for compatibility reasons?

Thanks for your work in getting this up and running nonetheless! I'm happy I could provide some inspiration.

KalobTaulien commented 4 years ago

That's a good question. Mainly just because it's currently less to maintain and for people who are reading their ga-key.json file if they are given an upload option they may not entirely have a secure setup and that could potentially expose their API information. Someone might also wonder about permissions and who should see/update/delete that data and who shouldn't, so there'd need to be a permission manager probably (open for discussion, of course).

Although in hindsight that's not really the responsibility of this package, is it (file security; also open for discussio). That'd be a good feature to have as well.