Skip to content
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

Idea: include abstract models and helper functions for easier CIM #25

Merged
merged 47 commits into from
Jul 28, 2013

Conversation

treyhunner
Copy link
Collaborator

Thoughts on this:

  1. Include BaseCustomerProfile and BaseCustomerPaymentProfile abstract models (based on this) with authorizenet app with sane default fields
  2. Include helper functions that create a customer profile and payment profiles on Authorize.NET and create the corresponding models on success.

@treyhunner
Copy link
Collaborator Author

I think the helper functions would actually work very well as model manager functions.

This couples the ORM to Authorize.NET for those models, but I think that's a good thing because those models should only be a reflection of the data that is associated on Authorize.NET.

If coupled to the ORM this also has the added benefit of allowing Authorize.NET customer management from the admin interface for free (maybe?). The save and delete model methods and the create and delete manager methods would automatically update on Authorize.NET.

If this isn't something you'd like to see in this app I may make a separate app for this.

@zen4ever
Copy link
Owner

This is a great idea. Yes, I agree that the Django models should be a reflection of Authorize.NET data. If somebody would need them to be decoupled (maybe if they use several different payment processors, and have more generic customer profile), they could still use basic CIM api and create their own models.

@treyhunner
Copy link
Collaborator Author

Great.

I'm starting to think these shouldn't be abstract models. I think the only information that should be stored in them would be Authorize.NET-related so they shouldn't need to be extended. If they aren't abstract models then the model managers would be linked already and the admin interface would exist by default.

The only downside to creating non-abstract models is that users who don't use them will be stuck with model references in their admin interface that they don't use.

Maybe this should be a concrete app provided separate from authorizenet but in the same package? Maybe something like authorizenet.customers?

@zen4ever
Copy link
Owner

According to this thread, nesting Django apps is possible.
https://groups.google.com/forum/?fromgroups#!topic/django-users/8LMDL74mDzY

The only thing is that if we choose 'authorizenet.customers' as a name for the app,
it might clash with any 'customers' app, our users might have in their project.

Maybe we should go with something like 'authorizenet.authnet_customers'.

@treyhunner
Copy link
Collaborator Author

That's a good point... and unfortunate. I think authorizenet.authnet_customers looks ugly and repetitive, but I can't think of a better name at the moment.

I'll use authorizenet.customers as a working name with the understanding that it will need to be changed before being released.

@zen4ever
Copy link
Owner

Yes, it is a bit ugly. :-)

@treyhunner
Copy link
Collaborator Author

I turned this into a pull request. Currently the single test for this is under a authorizenet.customters.customer_tests. I may move the tests to authorizenet.tests to avoid this extra app.

Currently there are two models with managers based on the sample code I'd written before as well as a new generic view for creating payment profiles.

I may end up importing requiring webtest for the tests to integration test the forms with the views.

@treyhunner
Copy link
Collaborator Author

I consolidated the tests and moved the tests app to the top level directory.

Maybe nesting an app within authorizenet isn't necessary. We could include the new models, managers, and views in the authorizenet app and use a SHOW_CUSTOMERS_IN_ADMIN setting to enable/disable registering the models with the admin site.

@zen4ever
Copy link
Owner

Yes, that might be a better option.

@treyhunner
Copy link
Collaborator Author

A settings object is now used in the authorizenet.conf module so all settings are self-documented there.

Currently the admin interface works correctly for:

  • Adding customer profile
  • Deleting customer profile
  • Editing customer profile (payment profiles can be edited, but customer profiles are read-only)
  • Adding payment profile for customer
  • Deleting payment profile
  • Editing payment profile

Expiration dates are currently not stored in payment profiles, but they are stored/displayed on authorize.net so I'm considering creating a database field to store them.

I'm also planning to create an admin page linked to from the customer profile admin page that allows charging one of a customer's payment profiles.

The error message passing also needs to be improved (e.g. "Error creating customer profile" is not a helpful message). Currently exceptions are raised with a very generic message and then never caught. I'm not sure how we should handle this yet.

@zen4ever
Copy link
Owner

Looks great.

Regarding the exceptions, if output includes sort of traceback, or reason for the error, we should include it in the exception message. We shouldn't catch them, because it is a job of the application that uses the library.

@treyhunner
Copy link
Collaborator Author

Summary of changes:

  • CustomerProfile and CustomerPaymentProfile models created for:
    • local storage of non-sensitive customer information
    • easier creation/update/deletion of customer profiles and customer payment profiles
  • Model form for CustomerPaymentProfile which automatically creates CustomerProfile if needed
  • Generic views for creating and updating CustomerPaymentProfile
  • Working admin pages for creating/updating/deleting customer profiles and payment profiles
  • Easy exception raising from CIMResponse based on success status

Changes that we might consider (pre or post merge):

  • Extend BillingError to include CIM status code and transaction status code information when applicable
  • Add shipping profile support
  • Add page allowing card to be charged from admin interface

The only bug in the current code that I'm aware of is that the save button on the customer profile admin page requires all payment profile credit card information to be re-entered (because it doesn't know not to save unchanged payment profiles). The solutions I've thought of to resolve this:

  • Remove the payment profiles from the customer profile admin page
  • Make payment profiles read-only on customer profile admin page and use django-relatives object_edit_link or similar to link to payment profiles
  • Use django-model-utils FieldTracker or something similar to track whether any fields have changed and only save if they have been

@zen4ever
Copy link
Owner

I like the idea with django-relatives for the payment profiles, and making them readonly. It is nice to see customer payment profiles, but they don't need to be editable from the customer profile page directly.

treyhunner added a commit that referenced this pull request Jul 28, 2013
Idea: include abstract models and helper functions for easier CIM
@treyhunner treyhunner merged commit 665829c into master Jul 28, 2013
@treyhunner treyhunner deleted the customer-models branch July 28, 2013 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants