Skip to content
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

feat(): Add Splitbee analytics for pass downloads #33

Closed
wants to merge 2 commits into from

Conversation

luisivan
Copy link
Contributor

PS: On a different note, we should add Prettifier config files to all projects to avoid noise in PRs

@vercel
Copy link

vercel bot commented Jun 29, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
mobile-passport ✅ Ready (Inspect) Visit Preview Jul 1, 2022 at 2:01PM (UTC)

Copy link
Member

@aahna-ashina aahna-ashina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luisivan This PR is both adding Splitbee and doing a lot of refactoring at the same time, so it's not easy for code reviewers to read the Splitbee code diff. Can you either 1) create one PR per change (one PR for refactoring, and another for Splitbee), or 2) create one PR with one commit per change (one commit for refactoring, and another for Splitbee)? Thanks! 😃

(Here is a great pull request philosophy: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#pull-request-philosophy)

@luisivan
Copy link
Contributor Author

Yeah... that's what I was mentioning above, since we didn't have a Prettifier config, just doing a save on my editor refactored the whole thing 😄

Maybe it's best to first merge #34, and then this one after (and then the changes will only affect Splitbee)

@luisivan
Copy link
Contributor Author

luisivan commented Jul 1, 2022

@aahna-ashina ready to review again!

Copy link
Member

@aahna-ashina aahna-ashina left a 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 (though I have not tested the changes on localhost yet).

Out of curiosity: Where will the collected analytics be made available? On the Splitbee website I'm assuming?

server/pages/api/downloadPass.ts Show resolved Hide resolved
@aahna-ashina
Copy link
Member

@luisivan This PR will be closed when we merge #73 because we can get the same pass details by querying the downloads table that was added: 90afce2

select time_of_download, platform, address, passport_id from downloads;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants