store wishlist in database

[dfalk]

Wishlist with email notifications on stock updating.

[travisbot]

This pull request fails (merged a5b7b4f into 1586508).

[travisbot]

This pull request fails (merged a5b7b4f into 1586508).

[travisbot]

This pull request passes (merged 5e026f3 into 1586508).

[dfalk]

any thoughts?

[stephenmcd]

Yeah it's a great idea - once I have some time I'll go through it, hopefully soon.

Thanks Dmitry

[rterbush]

This is a must have feature. Anything we can do to help get this into a release?

[stephenmcd]

Sorry for letting this one fall by the wayside, I'd completely forgotten about it. I've gone over it now and there are a few issues to address.

The primary concern is that there are actually two distinct features implemented here. First is allowing registered users to create persistent wishlists tied to their account, not their session. I'm all for this feature and I think it'd be a great addition. The second thing it implements is bulk emailing. If a product was in 100s of users wishlists and they were to be notified, with the way this is implemented, the server would be tied up handling the admin's request to send all these out. I use an arbitrary value of "100s" here, but the number is irrelevant - the point is that it introduces a scaling issue which is unsafe. Interestingly as the number of potential emails being sent out grows, you can conclude that popularity of the site itself also grows, and so the need to not have requests tied up by something like this becomes more of a concern. The good news is that with the DB backed wishlists, a project developer should be able to hook into Django's signals to implement this feature themselves, using whatever mailing approach they'd like to. So ultimately the email feature needs to be removed from this code.

A couple of other minor points.

It'd be good if there was a BaseWishlist class that implements the methods for adding and retrieving items from the user's session. The model-based Wishlist could then subclass it, and override those methods appropriately. We could then have one point in the code that determines which class to use (based on whether the user is authenticated), and then all of the form/view code would not need all the branching it has in it within the approach implemented in this pull request.

Finally, unfortunately this pull request won't merge cleanly at this point in time. Again this is my fault for letting things lag for so long, which I apologise for, but given the above feedback I've made, isn't too big of an issue since it needs more work anyway.

[uranusjr]

Hi,

I've taken some time into the code and made some changes. I couldn't reach @dfalk so I figure I probably should post here first. Changes include adding a setting entry to disable email notification, and putting most of the wishlist adding/deleting/fetching logic in WishlistManager (Wishlist.objects)

I considered using inheritance and patching __new__ to abstract the wishlist updating mechanism, but was unsatisfied with both. Putting HTTP related codes into a model that is inherited by a model just doesn't feel right for me, and overriding __new__ just seems hacky. In the end I opt for the simple route:

1. Add a separate dummy class utils.CookieBackedWishlist that understands HttpRequest objects, and update request.wishlist when necessary.
2. Use a helper function utils.get_wishlist instead of calling Wishlist() to instantiate a new wishlist entry.
3. Always call set_cookie to update the cookie-backed wishlist when a view updates the wishlist — this doesn't really hurt anyway.

I'd like to hear about it if someone has a cleaner way to do this. You can view my branch is here, which rebased @dfalk's work and applied the changes mentioned .

[stephenmcd]

Hey @uranusjr - thanks a lot for this. Do you want to set up an initial pull request with your changes?

[uranusjr]

Sure, will do.

[stepmr]

+1