-
-
Notifications
You must be signed in to change notification settings - Fork 775
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
Gitcoin Grants CLR Graphs - breakdown of contribution volume over time #5687
Conversation
… each gitcoin grant
… each gitcoin grant
Codecov Report
@@ Coverage Diff @@
## master #5687 +/- ##
==========================================
- Coverage 30.08% 30.05% -0.03%
==========================================
Files 249 249
Lines 21455 21652 +197
Branches 3113 3213 +100
==========================================
+ Hits 6454 6508 +54
- Misses 14725 14863 +138
- Partials 276 281 +5
Continue to review full report at Codecov.
|
@danlipert just finished this up.. moving back to review |
</div> | ||
<script> | ||
document.max_graph = {{max_graph}}; | ||
document.history = {{history|safe}}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update it to ?
const max_graph = {{ max_graph }} ;
const history = {{ history|safe }};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm i was anchoring it off the document to give it global scope so i could reuse in the JS file.. let me see if that'll work still
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
app/assets/v2/js/grants/stats.js
Outdated
if (document.max_graph > 1000) { | ||
increment = 1000; | ||
} | ||
if (document.max_graph > 10000) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else if
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thatll change the logical execution of this. i could move line 21 above line 18 + add an else if tho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
app/assets/v2/js/grants/stats.js
Outdated
google.charts.setOnLoadCallback(drawChart); | ||
|
||
function drawChart() { | ||
var target = document.getElementById('grant_chart'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use $('#grant_chart')
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that seems to break the graph. i think that the google chart code doesnt expect jquery elements..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's cause we are loading it before jquery 😅
no worries
@property | ||
def history_by_month(self): | ||
# gets the history of contributions to this grant month over month so they can be shown o grant details | ||
# returns [["", "Subscription Billing", "New Subscriptions", "One-Time Contributions", "CLR Matching Funds"], ["December 2017", 5534, 2011, 0, 0], ["January 2018", 10396, 0 , 0, 0 ], ... for each monnth in which this grant has contribution history]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... are you sure? im trying to get in the habit of documenting my code more..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments are good! I know we have a lot of docstring style comments which I think are overkill and lead to boilerplate copy-and-pasting but I think comments are something we should definitely encourage, especially if we want outside contributors to grok what we are doing more easily and hence be able to effectively dogfood more @thelostone-mc @owocki
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright alright! majority wins :P
the comments stay
app/grants/models.py
Outdated
subkey = 'New-Recurring' | ||
else: | ||
subkey = 'Recurring-Recurring' | ||
if contrib.subscription.contributor_profile.handle in ['vs77bb', 'gitcoinbot', 'notscottmoore', 'owocki']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thoughts on moving this to a constant defined above ?
['vs77bb', 'gitcoinbot', 'notscottmoore', 'owocki']`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
@owocki Also given that folks have pointed out that the right side is empty! Would it make sense to combine this with the activity tab as opposed to being it's own ? ideally the graph is build form the same data source as activity |
i dont follow "right side is empty" - what does that mean? |
derp my bad ! hence the ping asking if it made sense to combine the graph with activity or nah |
ok thanks; im indifferent to whether it should be its own tab or on the activity tab |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
78e793e
turns around aditya's code review.. would lub feedback from others tho
app/assets/v2/js/grants/stats.js
Outdated
google.charts.setOnLoadCallback(drawChart); | ||
|
||
function drawChart() { | ||
var target = document.getElementById('grant_chart'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that seems to break the graph. i think that the google chart code doesnt expect jquery elements..
app/assets/v2/js/grants/stats.js
Outdated
if (document.max_graph > 1000) { | ||
increment = 1000; | ||
} | ||
if (document.max_graph > 10000) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
app/grants/models.py
Outdated
subkey = 'New-Recurring' | ||
else: | ||
subkey = 'Recurring-Recurring' | ||
if contrib.subscription.contributor_profile.handle in ['vs77bb', 'gitcoinbot', 'notscottmoore', 'owocki']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
</div> | ||
<script> | ||
document.max_graph = {{max_graph}}; | ||
document.history = {{history|safe}}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really stoked for this - just one comment regarding DB queries
app/grants/models.py
Outdated
# returns [["", "Subscription Billing", "New Subscriptions", "One-Time Contributions", "CLR Matching Funds"], ["December 2017", 5534, 2011, 0, 0], ["January 2018", 10396, 0 , 0, 0 ], ... for each monnth in which this grant has contribution history]; | ||
CLR_PAYOUT_HANDLES = ['vs77bb', 'gitcoinbot', 'notscottmoore', 'owocki'] | ||
month_to_contribution_numbers = {} | ||
for sub in self.subscriptions.all(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefetch_related('subscription_contribution')
will save us a bunch of DB lookups I think. Then filter success=True
in python like for contrib in [sc for sc in sub.subscription_contribution if sc.success=True]
. What do you think? Cost some CPU but greatly reduces DB lookups so should be faster to execute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hows this? i tested locally 3f49ec6
Description
Adds a grants graph showing the breakdown of fund source over time to each Gitcoin Grant. Details at https://twitter.com/owocki/status/1210242826329632769
Refers/Fixes
User Research/Intution
Testing
on my local