-
Notifications
You must be signed in to change notification settings - Fork 4.2k
CSV Reporting of Shopping Cart Purchases, with tests #1685
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
Conversation
|
@ormsbee also, if you please. |
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.
Does this mean it's doing a separate DB query for every row to get the annotation? If so, can we grab this information at a higher level?
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.
Maybe it just makes more sense for Annotation / Comments to be a new TextField on OrderItem, defaulting to u""? Then this extra lookup can happen cart insertion time, but the reporting will just select from one table.
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, I think I would prefer that. @dianakhuang?
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 agree with @ormsbee about having a separate column for new comments instead of a new PaidRegistration-specific model.
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.
Well, so we'd still need the model, so that we have a place to lookup what to put into this comments field. (the mapping from course_id to PTA number needs to live somewhere). The only other "convenient" place would be settings, but I think that's wrong.
But the good thing is its use limited to PaidCourseRegistrations and is named accordingly.
|
OK. I added |
squashing to one commit to make cherry-picking by feature possible
|
@dianakhuang ping. i understand you're busy, though. |
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 there any reason why you don't use a View object here instead to make the difference between the GET and the POST behavior clearer?
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.
care to elaborate? what did you have in mind?
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 was thinking something like ChooseModeView where you have a view object and separate out different functions for post and get requests.
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 see. Simple answer: I don't believe creating another encapsulating layer for this functionality is necessary. There isn't any state to be kept / generated in that additional capsule, and 1/2 the functionality is in the models anyway, so this kept the view as simple as possible (and somewhat less magical, though I can see that course_modes.urls isn't too bad either). Also, some of django.contrib's views (like auth) structure the code this way.
FWIW, I don't think the if clauses on request.method are too confusing since the clause bodies weren't that long.
I can see a case for a generic model-based view that generates CSVs from model(s) being DRY, but I didn't want to sign up to do that with this feature request.
|
I think those were my only comments. We might want to build on top of this for our own reporting, but we're still working that out. |
|
ok to merge, then? @ormsbee @dianakhuang And I'd be happy to make future changes to they facilitate your use case. |
CSV Reporting of Shopping Cart Purchases, with tests
@dianakhuang
At Stanford, we're running the credit card purchased courses for our medical school. Meaning that the purchased income actually goes into their account from CyberSource. This also means that med school accountants need to be able to access the itemized purchased data from our platform (since CyberSource HOP can't itemize).
We agreed that the easiest way is for us to provide a page in the platfrom from which they can download CSV reports that itemize purchases made over a time range. There's no link to this page anywhere in the platform. They'll just have to bookmark it. The access control for this link is done by a magic
django.contrib.auth.models.Group, whose name is set in settings.They also had one "special" requirement, that each itemized line be annotated with a "PTA" number, which is a direct 1-to-1 mapping to the
course_idof the purchasedPaidCourseRegistration. (Yes, I suggested that they could simply match up the description with a PTA number. No, somehow they weren't comfortable with that). This leads to the need for the newPaidCourseRegistrationAnnotationmodel.