Rewrite #33

fangpenlin · 2013-08-18T06:21:16Z

Restart from a solid ground up

Basic models are built (core business logic and RESTful API are not implemented yet), still not in functional stage. However, we have 97% testing coverage currently. I hope I can keep this through and eventually deliver with 100% testing coverage.

Name                        Stmts   Miss  Cover   Missing
---------------------------------------------------------
billy                           0      0   100%
billy.models                   19      9    53%   19-36
billy.models.company           37      0   100%
billy.models.customer          36      0   100%
billy.models.plan              48      0   100%
billy.models.subscription      45      1    98%   89
billy.models.tables            81      1    99%   35
billy.models.transaction       42      0   100%
billy.utils                     0      0   100%
billy.utils.generic            23      0   100%
---------------------------------------------------------
TOTAL                         331     11    97%
----------------------------------------------------------------------
Ran 39 tests in 1.282s

OK

To install and run testing

virtualenv --no-site-packages env
source env/bin/activate
python setup.py develop
python setup.py nosetests

Only merge this branch back when it is good to go.

mjallday · 2013-08-18T18:16:56Z

billy/models/subscription.py

+    """
+
+
+class SubscriptionModel(object):


@victorlin what's your thinking behind suffixing all the models with Model (e.g. SubscriptionModel) and then also including the type in the method name?

This seems very verbose:

from billy.models import SubscriptionModel subscription = SubscriptionModel.create_subscription(...) subscription = SubscriptionModel.get_subscription_by_guid(xxx) subscription.update_subscription(...)

To me, calling the model Subscription and the methods get and update respectively seems more concise and it doesn't detract from readability.

@mjallday I like to provide expressive naming in most cases, however, yeah, you're right, this is a overkill here. In certain model context, this cannot help readability afterall. I will correct them later.

But for Model suffixing, I think this is fine. The whole model here is different from what it used to be. I try to isolate business logic from SQLAlchemy object and operations. I don't want to expose too much SQLAlchemy details as when one day we like to switch NoSQL or something like that, it would be difficult to get rid of SQLAlchemy everywhere in the project, as I explained in #24. Also, there is table class named Subscription, I don't want them to be confusing. And from the big picture, it would be a MVC structure, the naming might look like this

[Subscription (table)] <---- [SubscriptionModel (business logic)] <---- [API web controller]

So, I will keep Model suffixing. Until I make this system workable, if you don't like naming style, I can make some change here.

mahmoudimus · 2013-08-19T01:56:44Z

@victorlin going to address one of your comments here:

You wrote -

I got reasons to pass only GUID. Passing SQLALchemy table class instance around is error-prone in certain situations. For example, when you are dealing with multi-threading, passing object instance around causes problem. (emphasis added)

Since you're using a scoped_session and a sessionmaker, you shouldn't have to worry about multi-threading issues.

def process(records):
for record in records:
with transaction.manager:
record.num += 1
session.add(record)

pool = threading_pool()
for thread in pool:
records = get_records(100)
thread.run(process, records)

This is a really inefficient way of doing bulk updates or insertions. If you want to do a bulk-update, using threading is typically a really poor way of doing it. There are constructs in SQLAlchemy that allow you to very easily do bulk updates or insertions. In one call, using one transaction.

Let's say, we are processing transactions and we like to speed it up by using multi-threading or multi-processor. The code we wrote above looks fine, but it is not. You cannot create SQLALchemy object in one thread and pass it to another (exception will be raised). There is some thread-local stuff Session object is dealing with (sessions are bound to local threads). So, in this case, I prefer to make model only accept GUID argument rather than object.

There's other ways to do this if you want to pass a SQLAlchemy mapped object between threads. Simply, tell the ORM to stop tracking and instrumenting this object while you're passing it. Use Session.expunge()

Threads in Python are typically used for I/O heavy applications, so in that case, why not just pass the model, then do a Session.merge() and then do a Session.refresh().

Passing in objects is a very nice way to work with relationships. Not using them really limits the power of SQLAlchemy.

mahmoudimus · 2013-08-19T01:58:19Z

billy/manage.py

@@ -14,6 +14,7 @@ def create_tables():
    """
    Creates the tables if they dont exists
    """
+    print '#'*10, 'Base.metadata', Base.metadata
    Base.metadata.create_all(DB_ENGINE)


We should be using alembic for our database migrations.

fangpenlin · 2013-08-19T02:18:27Z

@mahmoudimus

What I mean is not actually doing updates or insertions with threading. But for more complex business logic. For example, currently, we have yield_transactions method of SubscriptionModel. It is for yielding transactions from subscriptions. If we have a tons of subscription to process, it would be slow to do it in single thread. This kind of logic cannot be done with plain SQL expressions like bulk updates or insertions (it may could be, but might be too complex, or that needs to be optimized later).

I know there is a way I can leverage session to deals with cross thread issue. However, I prefer to keep things simple and stupid (KISS). The Session.refresh() will actually get the record from database by its primary key, just like what you do by passing GUID. So, in this case, for simplicity and consistency, l prefer passing primary keys. However, if passing object is more comfortable for youguys, I will refactory the model to do it the way later (When we reach the stable stage).

mahmoudimus · 2013-08-19T02:18:29Z

billy/models/transaction.py

+        if transaction_type not in self.TYPE_ALL:
+            raise ValueError('Invalid transaction_type {}'.format(transaction_type))
+        transaction = tables.Transaction(
+            guid='TX' + make_guid(),


This pattern should be moved to a base model.

I know we have duplicated code there. However, I will keep them there for a while. I would like to see what are those common elements shared by all modes, and extract them to a base class or something later. Sometimes by doing OO design too early, it would be over engineering or modeling it wrong. A little deferred refactorying would be helpful.

We use this same pattern. I can share it here.

@victorlin - definitely agree with you regarding OO design too early making it over engineering or modeling it wrong, but we've been working on this for almost 3 years now. We've got a couple of patterns that we can share w/ you that we've really nailed down over time.

One flaw I see with your method is if you ever have to override any of the uuid creation schemes, it's very hard to do so (you don't expose a method anywhere to do this).

I'll fork your branch of billy and make a pull request so I can demonstrate what I'm talking about. I'll also add you to our internal Balanced project so you can see how advanced it is.

You're right. I am actually building billy with my experience and my practices. I have done things with this kind of structure and patterns for tons of projects. Although, I am doing this for years, I am still improving it gradually.

The reason we have so many discussions here is that we have very different development experience and practices, surely it would take some time to get used to. And yes it would be nice if I can see how you do in the backend systems. I am sure I can learn something from that and apply on billy.

But for now, I can do it with the best productivity in my favor manners (in this week, maybe). So, I will keep working, and in the mean time, I will try get familiar with your style, then gradually refactory and transform into Balanced style. With good testing coverage, we can always switch between different implementation approaches easily. Does it make sense?

Absolutely :)

mahmoudimus · 2013-08-19T03:12:16Z

Also, are you going to recurring payout support? This is probably the most important distinguishing feature for Billy. See #15.

fangpenlin · 2013-08-19T03:25:36Z

@mahmoudimus

Also, are you going to recurring payout support? This is probably the most important distinguishing feature for Billy. See #15.

Yes, recurring payout is included in this initial development cycle.

fangpenlin · 2013-08-19T04:29:47Z

The features I am going to implement in this cycle are posted in #25 as usage scenario form. If you see anything that is needed badly but not there, please added it in a reply.

fangpenlin · 2013-08-23T11:46:37Z

Okay, for now, there are still details to take care, however, we have reached a rough workable version with 100% testing coverage.

Name                                        Stmts   Miss  Cover   Missing
-------------------------------------------------------------------------
billy                                          12      0   100%
billy.api                                       7      0   100%
billy.api.auth                                 36      0   100%
billy.api.company                               4      0   100%
billy.api.company.views                        25      0   100%
billy.api.customer                              4      0   100%
billy.api.customer.views                       27      0   100%
billy.api.plan                                  4      0   100%
billy.api.plan.views                           33      0   100%
billy.api.subscription                          4      0   100%
billy.api.subscription.views                   37      0   100%
billy.api.transaction                           3      0   100%
billy.api.transaction.views                    16      0   100%
billy.models                                   15      0   100%
billy.models.company                           42      0   100%
billy.models.customer                          36      0   100%
billy.models.plan                              50      0   100%
billy.models.processors                         0      0   100%
billy.models.processors.balanced_payments      63      0   100%
billy.models.processors.base                   10      0   100%
billy.models.schedule                          20      0   100%
billy.models.subscription                      97      0   100%
billy.models.tables                            91      0   100%
billy.models.transaction                       91      0   100%
billy.renderers                                31      0   100%
billy.request                                   7      0   100%
billy.scripts                                   0      0   100%
billy.scripts.initializedb                     19      0   100%
billy.scripts.process_transactions             32      0   100%
billy.utils                                     0      0   100%
billy.utils.generic                            26      0   100%
-------------------------------------------------------------------------
TOTAL                                         842      0   100%
----------------------------------------------------------------------
Ran 136 tests in 7.081s

OK

To run the test

virtualenv --no-site-packages env
source env/bin/activate
python setup.py develop
pip install -r test_requirements.txt
python setup.py nosetests
# or you prefer 
nosetests

Create tables database

initialize_billy_db development.ini

Start the server

pserve development.ini --reload

Create a company

# create company
curl http://127.0.0.1:6543/v1/companies/ -X POST \
    -d "processor_key=YOUR_BALANCED_API_KEY"

result

{
    "guid": "CPPEfEKswV4eywR67He2ZkFu", 
    "api_key": "DCwfL7zJi48EesUCLSiyemCuxxdSiB7T1Bha9icTBN4d", 
    "created_at": "2013-08-23T06:39:04.427220", 
    "updated_at": "2013-08-23T06:39:04.427232"
}

Create a customer

# create customer
curl http://127.0.0.1:6543/v1/customers/ \
    -u DCwfL7zJi48EesUCLSiyemCuxxdSiB7T1Bha9icTBN4d: \
    -d "external_id=/v1/customers/AC1jqOF9TocQXGIXjuMVrpMu"

result

{
    "guid": "CUJn4Us9HqQmkEazeKDTsoYo", 
    "company_guid": "CPPEfEKswV4eywR67He2ZkFu", 
    "external_id": "/v1/customers/AC1jqOF9TocQXGIXjuMVrpMu", 
    "created_at": "2013-08-23T08:18:16.782940", 
    "updated_at": "2013-08-23T08:18:16.782950"
}

Create a plan

# create plan
curl http://127.0.0.1:6543/v1/plans/ \
    -u DCwfL7zJi48EesUCLSiyemCuxxdSiB7T1Bha9icTBN4d: \
    -d "plan_type=charge" \
    -d "amount=55.66" \
    -d "frequency=daily"

result

{
    "guid": "PLWZH4N9mB5wTLwEDTSgaPpf",
    "company_guid": "CPPEfEKswV4eywR67He2ZkFu", 
    "interval": 1, 
    "plan_type": "charge", 
    "amount": "55.66", 
    "frequency": "daily", 
    "created_at": "2013-08-23T08:20:56.820769", 
    "updated_at": "2013-08-23T08:20:56.820785"
}

Create a subscription

# create subscription
curl http://127.0.0.1:6543/v1/subscriptions/ \
    -u DCwfL7zJi48EesUCLSiyemCuxxdSiB7T1Bha9icTBN4d: \
    -d "customer_guid=CUJn4Us9HqQmkEazeKDTsoYo" \
    -d "plan_guid=PLWZH4N9mB5wTLwEDTSgaPpf"

result

{
    "guid": "SUBSHs7KijcmEmuqUFPEkNiP",
    "customer_guid": "CUJn4Us9HqQmkEazeKDTsoYo", 
    "plan_guid": "PLWZH4N9mB5wTLwEDTSgaPpf", 
    "period": 0, 
    "canceled": false, 
    "amount": "None", 
    "started_at": "2013-08-23T08:23:46.534374", 
    "next_transaction_at": "2013-08-23T08:23:46.534374", 
    "created_at": "2013-08-23T08:23:46.536058", 
    "updated_at": "2013-08-23T08:23:46.536065"
}

Process transactions

We have a subscription in the database, and it should yield a transaction right away. To process them, here you call

process_billy_tx development.ini

A crontab setting or similar scheduler can be used to process all transactions periodically.

What's next?

There are still some design decisions to make, such as should we allow modifying subscription, should we process transaction immediately when a subscription is added, I will think about them. I may create issues for them, then we can talk about it and see what kind of design is better.

Although the testing coverage is 100%, doesn't mean it is bug-free nor perfect, I am sure there are many edge cases aren't tested.

And for testing, we have unit tests and functional tests, they are all on SQLite. I will create a Vagrant environment and writes some integration tests there with PostgreSQL.

In the mean time, I may do some refactory, but the priority here are still the tests. With good testing coverage (include edge cases), we can move faster.

mjallday · 2013-08-23T15:36:43Z

@victorlin you are amazing :)

fangpenlin · 2013-08-26T10:23:30Z

@mjallday
Thank you :)

Well, I think this branch is good to merge back now. It is stable enough and also functional. I will create issues for remaining task later.

…sion

… than uri bug

(thread local is used in Balanced-Python)

… without permission

mjallday · 2013-09-05T16:55:39Z

this pull-request is big and old. can we merge it so we don't have to look at 10k line pull requests ever again?

Rewrite

mahmoudimus · 2013-09-05T17:59:24Z

this looks great +1

mjallday reviewed Aug 18, 2013
View reviewed changes

mahmoudimus reviewed Aug 19, 2013
View reviewed changes

fangpenlin mentioned this pull request Aug 26, 2013

Decide the API style #40

Open

fangpenlin added 11 commits August 27, 2013 08:03

Fix balanced#14, add setup.py and python distribute files

c7eb8c6

Use a better import style

0120ae7

Fix tons of import issue

7de6f6f

Rollback some API descrpition in readme file

d90fd5a

Remove unused file

8403639

Add new model and tables module

6a58756

Update utile

708b459

Clean up code

ee99d29

Clean old database code (refectory and add them back later)

5f002f1

Clean requirements up

f55b656

Remove old code

915b063

fangpenlin added 25 commits August 28, 2013 00:31

Fix a transaction view test failure

bab0811

Add Chef submodule

270da99

Add vagrant file

f7f2c0d

Add password for postgresql database

4628c87

Update vagrant file to solve a deploying issue caused by old Chef ver…

9fbbed1

…sion

Fix balanced processor API key is not configured and return id rather…

d47e3b7

… than uri bug

Process transaction right away when subscription is created

60ff67a

Clear race condition concern in balanced payment processor

5abdb2b

(thread local is used in Balanced-Python)

Add missing payment URI for subscription API

004d5f1

Create card for basic integration test scenario

d9eb88a

Add transaction check in basic integration testing scenario

aabdf28

Add chef submodule folder

11b6b93

Fix SQLAlchemy Unicode warning

e38e826

Change meta tag naming of Balanced resource object

50073b3

Make sure only charge transaction can be refunded

d134d1e

Implement refund of processor

c505294

Add test for refund of processor

9563d79

Fix prorated refund rate bug (the refund rate was inverted)

b12cfac

Fix subscription zero prorated refund test failure

72c3823

Implement subscription cancel

e6d3dda

Add test for making sure cancel subscription API will not be accessed…

da53f7d

… without permission

Remove unnecessary type check in process_one of transaction model

b0d22c2

Add refund_amount argument to cancel method of subscription model

b1913f5

Implement cancel subscription with refund_amount

b3e9d31

Mark not finished transactions as canceled when canceling a subscription

8a9237e

mahmoudimus added a commit that referenced this pull request Sep 5, 2013

Merge pull request #33 from victorlin/rewrite

95e52e4

Rewrite

mahmoudimus merged commit 95e52e4 into balanced:master Sep 5, 2013

fangpenlin deleted the rewrite branch September 5, 2013 23:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite #33

Rewrite #33

fangpenlin commented Aug 18, 2013

mjallday Aug 18, 2013

fangpenlin Aug 19, 2013

mahmoudimus commented Aug 19, 2013

mahmoudimus Aug 19, 2013

fangpenlin commented Aug 19, 2013

mahmoudimus Aug 19, 2013

fangpenlin Aug 19, 2013

mahmoudimus Aug 19, 2013

mahmoudimus Aug 19, 2013

fangpenlin Aug 19, 2013

mahmoudimus Aug 19, 2013

mahmoudimus commented Aug 19, 2013

fangpenlin commented Aug 19, 2013

fangpenlin commented Aug 19, 2013

fangpenlin commented Aug 23, 2013

mjallday commented Aug 23, 2013

fangpenlin commented Aug 26, 2013

mjallday commented Sep 5, 2013

mahmoudimus commented Sep 5, 2013

Rewrite #33

Rewrite #33

Conversation

fangpenlin commented Aug 18, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahmoudimus commented Aug 19, 2013

Choose a reason for hiding this comment

fangpenlin commented Aug 19, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahmoudimus commented Aug 19, 2013

fangpenlin commented Aug 19, 2013

fangpenlin commented Aug 19, 2013

fangpenlin commented Aug 23, 2013

To run the test

Create tables database

Start the server

Create a company

Create a customer

Create a plan

Create a subscription

Process transactions

What's next?

mjallday commented Aug 23, 2013

fangpenlin commented Aug 26, 2013

mjallday commented Sep 5, 2013

mahmoudimus commented Sep 5, 2013