Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Fake data - subscriptions and payments #3596

Merged
merged 14 commits into from
Sep 21, 2016

Conversation

rorepo
Copy link
Contributor

@rorepo rorepo commented Jul 2, 2015

Added fake subscriptions and payments. Tests have been correspondingly updated.

Tips and transfers have been left as they are, for removal later once migration is complete.

This is ready for review.

Please note one change in particular that needs to be validated, made for week_tips and week_transfers (and the new week_subscriptions and week_payments):
week_tips = filter(lambda x: date <= x['ctime'] < end_date, tips)

was

week_tips = filter(lambda x: date < x['ctime'] < end_date, tips)

Seemed to me that transactions at min_date were being skipped in processing due to the 'date < x['ctime']' condition. This wasn't causing any issues, because this would correspondingly not have gone into exchanges as well.

if participant.username == team.owner:
direction = 'to-participant'
else:
direction = random.sample(['to-team','to-participant'],1)[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we don't have takes yet - If the participant isn't an owner, the direction will always be to-team.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, since teams don't carry a balance - we need the net payments to sum up to zero. I've added a db check for this in #3597 :) Maybe try rebasing on that to make sure?

@rorepo rorepo force-pushed the fake-data-subscriptions-and-payments branch from cb0655a to e2aaae7 Compare July 3, 2015 19:57
@rorepo
Copy link
Contributor Author

rorepo commented Jul 4, 2015

The requirement of payments adding up to zero for teams is unlikely to be met with the random values being created at the moment. Therefore, this will require the following approach:

  • Create 'to-team' payments corresponding to all subscriptions
  • Create 'to-participant' payments from each distinct team subscribed to in subscriptions to its owner, equal in value to the total of all subscriptions received

This will however have the effect of limiting the number of payments to no. of participants * no. of teams.

@rorepo
Copy link
Contributor Author

rorepo commented Jul 6, 2015

Logic modified as above.

The number of subscriptions is also now dependent on the number of participants and teams, unless we are to go for multiple subscriptions from the same participant to the same team.
With current defaults, now there are 495 subscriptions and 500 payments in fake data.

@chadwhitacre
Copy link
Contributor

@rorepo Quick note re: process: Please apply the "Review" label to PRs that are ready for review. (I've updated http://inside.gratipay.com/howto/label-github-issues; waiting for the change to propagate.)

@rorepo rorepo added the Review label Jul 7, 2015
@rohitpaulk
Copy link
Contributor

The teams are created, but I can't see them on the Explore page...

screenshot from 2015-07-07 20 30 53

@rohitpaulk
Copy link
Contributor

Oops, sorry - My bad. A server restart fixed it.

@rohitpaulk rohitpaulk force-pushed the fake-data-subscriptions-and-payments branch from 2633edc to 1d00bf4 Compare July 7, 2015 15:23
@rohitpaulk
Copy link
Contributor

Rebased on master.

@rohitpaulk
Copy link
Contributor

@techtonik - Any idea what's up with the tests?

make: env/bin/honcho: Command not found

@rohitpaulk
Copy link
Contributor

screenshot from 2015-07-07 21 02 30

How do participants end up with that much of a negative balance?

@rohitpaulk rohitpaulk force-pushed the fake-data-subscriptions-and-payments branch 2 times, most recently from 664db12 to ee0db71 Compare July 7, 2015 16:42
@rohitpaulk
Copy link
Contributor

Hmm, still can't figure out the negative balances.

@rohitpaulk
Copy link
Contributor

Looks like the issue is present on master too. This PR is definitely an improvement over what's in master, so I'd merge it in once we can figure out why the tests aren't passing. (#3596 (comment))

@chadwhitacre
Copy link
Contributor

Any idea what's up with the tests?

Weird. This PR doesn't even modify Makefile, just two test files. Can we pin down which file has the changes that introduce the bug? What happens if we temporarily back out the changes to test_fake_data.py? Do we still see the missing honcho?

@chadwhitacre
Copy link
Contributor

Is it something to do with build caching at Travis? I've restarted the build ...

@chadwhitacre
Copy link
Contributor

It's odd that we get Requirement already satisfied for honcho. How does the output of make env compare to a passing test run?

@chadwhitacre
Copy link
Contributor

(Restarted build failed, ftr.)

@rorepo
Copy link
Contributor Author

rorepo commented Jul 11, 2015

I've figured out the negative balance issue. This is down to participants corresponding to week_payments not having ctime < the end_date for the processing week. These payment values therefore don't get processed and thus cancelled out through exchanges. Happens because participant ctime is also random. Believe this situation would only exist in the dev environment.

Removing the ctime < end_date filter for week_participants (i.e. using participants collection as such) resolves the negative balances - except for one bit. The fee value from exchanges still remains. This is because this value has nothing in payments to cancel it out. Not sure how to handle this.

Please note the above is solely about payments, however. Residual balances will also be a problem as long as transfers remain, as now, because these are still random values. I suppressed transfers while figuring out the above.

@rorepo rorepo force-pushed the fake-data-subscriptions-and-payments branch from f1a009c to 5913628 Compare July 16, 2015 13:40
@chadwhitacre
Copy link
Contributor

What's up with this PR, @rorepo @rohitpaulk? What do we need to do to bring this in for a landing?

@rohitpaulk
Copy link
Contributor

As far as I remember, the tests were passing on my local. Haven't been able to figure out what's up with Travis

@chadwhitacre
Copy link
Contributor

Willing to pick up with this again, @rorepo? It's been hanging open for a while. I've restarted the latest Travis build in case it was a spurious error (missing honcho!?), but at this point we'll need to rebase on master and resolve conflicts before we can land this.

@kaguillera
Copy link
Contributor

kaguillera commented Aug 4, 2016

@whit537 Some of the changes here are not necessary since we have changed directions. There as things that we may want to remove and some that we can work with. I am going to take some time going through this and consider if it make sense to use this PR or scrape it and just start from scratch...it will take a little time.
Any suggestions are welcome.
At first glance we are no longer using subscriptions?!?!

P.S. And rebasing this with master is another task by itself. 😦

@chadwhitacre
Copy link
Contributor

we are no longer using subscriptions

I believe they were renamed to payment_instructions.

@kaguillera
Copy link
Contributor

oh ok...I need to keep that in mind

@kaguillera
Copy link
Contributor

rebased on master. next step is to see what we keep and what we remove. 😱

@kaguillera kaguillera mentioned this pull request Sep 15, 2016
1 task
@kaguillera kaguillera force-pushed the fake-data-subscriptions-and-payments branch from a571f98 to 961c720 Compare September 16, 2016 17:09
@kaguillera
Copy link
Contributor

kaguillera commented Sep 16, 2016

Also added fake_exchange_routes and fake_exchanges in the last commit
Now to remove communities from fake_data.py since we no longer use that.

@kaguillera
Copy link
Contributor

kaguillera commented Sep 16, 2016

I think that is good now...could someone review this. @rohitpaulk @aandis @whit537 etc.
well once travis is green.

@chadwhitacre chadwhitacre merged commit 1c3aa6a into master Sep 21, 2016
@chadwhitacre chadwhitacre deleted the fake-data-subscriptions-and-payments branch September 21, 2016 18:37
@chadwhitacre
Copy link
Contributor

!m @kaguillera

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

Successfully merging this pull request may close these issues.

4 participants