-
-
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
clr: update estimate_clr command to query by grant type #5639
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5639 +/- ##
==========================================
+ Coverage 30.07% 30.08% +0.01%
==========================================
Files 248 248
Lines 21395 21404 +9
Branches 3103 3105 +2
==========================================
+ Hits 6434 6440 +6
- Misses 14685 14688 +3
Partials 276 276
Continue to review full report at Codecov.
|
- estimate_clr.py -> new arguments clr_type, network - clr logic updated to query grants by network + grant_type + not hidden - grant str updated to also print grant_type - crontab updated to run both clr for media and tech grants
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.
So clean! Approved with just one question
app/grants/clr.py
Outdated
# setup | ||
clr_calc_start_time = timezone.now() | ||
|
||
# get all the eligible contributions and calculate total | ||
contributions = Contribution.objects.prefetch_related('subscription').filter(created_on__gte=CLR_START_DATE, created_on__lte=from_date) | ||
debug_output = [] | ||
grants = Grant.objects.all() | ||
|
||
grants = Grant.objects.none() |
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.
is this line some kind of fallback? It seems the following if else block would assign grants to something without needing this line
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.
Yeah ! Just kept it as fallback -> Ideally we would never need this
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.
@thelostone-mc I think from a programming logic standpoint there is no case where this would be undefined after the if else block, so I think it should be removed
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.
make's sense. removed ^_^
Description
Refers/Fixes
Gets in after #5637
Testing