-
Notifications
You must be signed in to change notification settings - Fork 309
Conversation
According the way that fake_data.py works a payment_instruction is made for each participant for each team except for the team owned by that participant. Just realized this is limited by ntips. |
Ok I think that I will have to go down the 🐇 ⚫ and rework |
Is "Received From" breaking anonymity here? |
I was thinking that just did not have any idea what would be appropriate On Oct 4, 2016 10:19 AM, "mattbk" [email protected] wrote:
|
8f42461
to
6326c89
Compare
Once #4150 is merged I will be able to finish this and put this for review |
6326c89
to
88b2669
Compare
Please review this. I still have to do the export link that would produce the data for export via csv or excel but I think that this can be merged and the export aspect can be done in another PR. Oh that is once it passes in travis. |
Oh and @whit537 I think that I got the same 150+ commit issue due to programming on the bus. I forced the push so that it would not ruin the history of the logs but I really need to find the root of this problem because I think that it will eventually come back to bite me. |
Sorry, @kaguillera. :-( Working on the build in #4173 ... |
88b2669
to
64b9dae
Compare
No Problem...I don't think there is any need to rush this. |
7ee296f
to
118d18b
Compare
Ready for review again. Rebased (at least I think so) |
Rebased, was 118d18b. |
118d18b
to
390b609
Compare
@kaguillera I'm seeing trailing whitespace. Can you configure your editor to remove that when saving? |
This needs to be updated in light of https://gratipay.news/from-teams-to-projects-45c46718507b. |
Will do |
Conflicts: gratipay/testing/harness.py
Ready for review and merging @mattbk @whit537 @nobodxbodon et. al |
I'd be glad to merge as soon as I see a screenshot that shows a demo page (or first thing on Monday without a screenshot), even if I can't fully review it before that. I don't think it's worthwhile to postpone this even more, given the fact that this is a standalone feature and shouldn't impact existing critical features. |
gratipay/utils/team_history.py
Outdated
if not payments: | ||
return payments | ||
|
||
#return payments |
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.
Looks like this can be removed?
gratipay/utils/team_history.py
Outdated
from decimal import Decimal as D | ||
|
||
|
||
def get_end_of_year_totals(db, team, year): |
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.
Might be a good idea to be clear on what format we're expecting arguments in: Is team
supposed to be a Team
object, or just the team slug? (The latter is the correct answer 🥇 )
I'd suggest using a docstring to describe what the arguments should be, and try passing Team
objects around as far as possible.
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.
ok will do.
gratipay/utils/team_history.py
Outdated
AND direction='to-team'; | ||
""", locals() ) | ||
|
||
if received is 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.
Since we're using COALESCE
, I don't think received
will ever be None
. Will it?
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.
True.
gratipay/utils/team_history.py
Outdated
AND direction='to-participant'; | ||
""", locals()) | ||
|
||
if distributed is 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.
gratipay/utils/team_history.py
Outdated
return received, distributed | ||
|
||
|
||
def iter_team_payday_events(db, team, year=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.
I don't think we should keep year
as an optional here, it already defaults to current_year
in the template ( year = int(request.qs.get('year', current_year))
).
Not that this is wrong, just that it is one more thing to maintain :)
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.
Fair enough.
gratipay/utils/team_history.py
Outdated
payments = db.all(""" | ||
SELECT payments.*, paydays.ts_start | ||
FROM payments, paydays | ||
WHERE payments.payday = paydays.id |
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.
This looks like a use case for JOIN
query.
ie. Instead of
SELECT payments.*, paydays.ts_start
FROM payments, paydays
WHERE payments.payday = paydays.id
use a single JOIN
:
SELECT payments.*, paydays.ts_start
FROM payments JOIN paydays ON payments.payday = paydays.id
I find that easier to read (the intent of having multiple tables + their relationship is clear), and from what I've read - databases can do more intelligent optimizations with JOIN
s compared to multiple SELECT
s.
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.
Will try.
gratipay/utils/team_history.py
Outdated
|
||
events = [] | ||
payday_id = payments[0]['payday'] | ||
payday_date = payments[0]['ts_start'] |
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.
I'd suggest naming ts_start
as payday_start
so that it is clear that it is the start of the payday
and not the payment
.
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.
ok
gratipay/utils/team_history.py
Outdated
#return payments | ||
|
||
for payment in payments: | ||
if payment['payday'] != payday_id: |
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.
I think this can be simplified using itertools.groupby
, it does exactly what we're doing here.
Here's what I think this will look like with using groupby
:
Before:
events = []
payday_id = payments[0]['payday']
payday_date = payments[0]['ts_start']
payday_events = []
if not payments:
return payments
#return payments
for payment in payments:
if payment['payday'] != payday_id:
events.append (dict(id=payday_id, date=payment['ts_start'], events=payday_events))
payday_id = payment['payday']
payday_date = payment['ts_start']
payday_events = []
payday_events.append(payment)
events.append (dict(id=payday_id, date=payday_date, events=payday_events))
After:
events = []
grouped_payments = groupby(payments, lambda x: x['payday_id'])
for payday_id, payments in grouped_payments:
events.append (dict(id=payday_id, date=payments[0]['payday_start'], events=payday_events))
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.
the entire day I have been trying to unpack that structure that you suggested @rohitpaulk and it is only giving me the last event i.e. the payout to the owner, every time I try to iterate over the events
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.
The following code (this is just an attempt at understanding the structure of the resulting object) should in theory produce a list with the payday_id
, payday_date
and list out all the transactions for that payday (unformatted)
{% for payday in events %}
{% set id = payday['id'] %}
{% set paydate = payday['date'] %}
{% set pay_events = payday['events'] %}
{% print id %}
<br/>
{% print paydate %}
<br/>
{% for each in pay_events %}
{% print each %}
{% endfor %}
<br/>
<br/>
{% endfor %}
But it does not it produces the following
Any suggestions???
Am I doing something wrong that I can't see.
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 - I'll try to take a look at this tonight
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.
Going to give this one another try...
raise Response(400, "Bad Year") | ||
years = list(range(current_year, team.ctime.year-1, -1)) | ||
|
||
if year not in years: |
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.
I think this check would be clearer if we explicitly checked for if year > current_year
, rather than relying on the fact that if x > y
, range(y, x, -1)
would not contain x
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.
Idea here was to ensure that a person will not put a year in the URL and that year could be greater than the current year or less than the year the team/project 'ctime`
www/%team/history/index.html.spt
Outdated
<td class="debits"></td> | ||
{% else %} | ||
<td class="credits"></td> | ||
<td class="notes"> Anonymous </td> |
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.
Should the taker
be anonymous here?
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.
Changed in d54faa7.
Had a quick glance through, left comments |
!m @rohitpaulk |
Travis timeout is likely #4359. |
Ready for final review once again. |
LGTM |
puts his finger on the button... |
This is the stub so far...so it is nowhere ready for review