bleroy / Nwazet.Commerce

Idiomatic commerce module for Orchard CMS.
BSD 3-Clause "New" or "Revised" License
26 stars 21 forks source link

Persistent shopping cart #117

Closed MatteoPiovanelli-Laser closed 6 years ago

MatteoPiovanelli-Laser commented 7 years ago

The cart is currently stored in the Http Session. That means it's 100% lost when the server gets updated and such. Also, I am not 100% sure how that would behave with clients that are not web-browsers (I am thinking specifically about native mobile apps).

I think it would make sense for many (most?) stores to be able to save the cart and keep it related to the user.

Some considerations that would go into designing this:

Any thoughts?

bleroy commented 7 years ago

That's not true. There is a session-based implementation of IShoppingCartStorage, but that is only a fallback for the main storage mechanism, which is local storage on the browser. Try it: restart the server and see if you lose your cart.

MatteoPiovanelli-Laser commented 7 years ago

I see. I did not check that. My fault, as I often test in incognito mode (so that I am logged in as admin on my main browser) I never noticed.

The other considerations I think would stand, however.

bleroy commented 7 years ago

The ability to save a cart for later, have wishlists, etc. is good. I'm wondering if it's possible to decouple those from cart storage. In other words, have named carts & wishlists that can work against any cart storage (that would probably require adding a name to carts in the storage interface) on the one hand, and on the other hand an additional cart storage implementation that persists carts against the user's account.

MatteoPiovanelli-Laser commented 7 years ago

I'll dig deeper into the current implementations and get back to you on this.

MatteoPiovanelli-Laser commented 7 years ago

I think it makes sense to decouple carts and wishlists from the cart storage, yes. Using a cart in db should be an IShoppingCartStorage implementation. In terms of data to store, a cart and a wishlist are pretty much the same: items, a name, the id of the owner, maybe a flag saying whether the record is a cart or a wishlist. The cart does not need a name, but I would still make the Name property required, and give it a default value for the cart. The Retrieve() method of the interface returns the cart because it is recognizable thanks to the flag. The storage implementation definitely needs to access the CurrentUser, but that is in the WorkContext anyway. A given user may only have 1 cart, but there is on principle no limit on the number of wishlists.

There is a need to handle a few corner cases. Off the top of my head, if I am anonymous, I put products in the cart (local storage), then login, my cart should carry over. However, there may b something already in my "stored" cart.

MatteoPiovanelli-Laser commented 7 years ago

Here's the way I am designing this (all the names of things are not necessarily set in stone, of course, and neither is the design itself):

I have a NamedProductsListRecord that is the base for PersistentShoppingCartPart and WishListPart. I also have the ContentTypes ShoppingCart and WishList. Those CTs are not Creatable, nor Lilstable, nor anything. The BO user can list and see Carts and WIsh Lists, but he may not edit/delete them, or really interact with them (unless there is a special permission). When the feature is active and the customer comes to the site, we create his single ShoppingCart (if he does not have one already), synchronize whatever is in the localstorage cart (still need to think on how to handle all the possible cases). Each authenticated customer will "have" one and only one cart. Authenticated users may "have" 0 or more WishList items. I still need to work a bit on the meaning of "have" in the previous paragraph. The authenticated user will cause those items to be created/updated from the store's frontend, by doing stuff like "Add to cart", "Remove from Cart", "Add to withlist", "Create wishlist". I'll figure out all this actions, corresponding in my head to Controller Actions, but any input is appreciated.

THe NamedProductsListRecord will (probably) have some properties that are "exposed" by the PersistentShoppingCartPart, some by WishListPart, some by both, some by neither. For example:

Having two different parts, even though they may really represent the same kind of stuff, makes it cleaner to also have drivers/handlers to define their specialized behaviours.

Now, there is something that I am thinking that may seem odd: I am planning to have NamedProductsListRecord be a ContentPartVersionRecord. That may seem weird, but I have been talking with the marketing people here (and they have been talking with the marketing people of our clients). They would love a "cart history" along the lines of "Customer X added product Y to his cart on FirstDate and removed it on SecondDate". Versioning the parts and the items pretty much does that for us already (as long as we also have a string Event in the record to save stuff like "Added item", "Purchased cart"...), and makes it easier to build marketing functionalities later on. On the other hand, if I did not have the version history of the cart, I would have to add a different record to track that information, and attach somehow to all events updating the cart. That seems needlessly complex, to be honest, even though it would have the advantage of having the definition of the record sit in the marketing feature. I feel quite strongly in favour of the VersionRecord, because that would save me a ton of work down the line, at the (cheap) cost of making sure now that stuff like "Add To Cart" always generates a new version of the cart (in the scope of this feature).

MatteoPiovanelli-Laser commented 7 years ago

Correct me on this if I am wrong, please:

The IShoppingCart implementation that is there, in the Add method, does

ItemsInternal.Insert(0, new ShoppingCartItem(productId, quantity, attributeIdsToValues));

Where:

private List<ShoppingCartItem> ItemsInternal {
    get {
        return _cartStorage.Retrieve();
    }
}

To keep the same IShoppingCartStorage, I will have to write a different IShoppingCart, because whenever there is a call that updates the cart, I should also update the underlying records, and that cannot (I think) be done simply with that Insert. Is that right? My work-in-progress PersistentShoppingCartPart exploits the [Serializable] attribute of ShoppingCartItem and has:

private List<ShoppingCartItem> _items
{
    get
    {
        return (List<ShoppingCartItem>)
            new BinaryFormatter()
                .Deserialize(
                    new MemoryStream(
                        Encoding.ASCII.GetBytes(Retrieve(r => r.SerializedItems))));
    }
    set
     {
         MemoryStream stream = new MemoryStream();
         new BinaryFormatter().Serialize(stream, value);
         stream.Position = 0;
         Store(r => r.SerializedItems,
             new StreamReader(stream).ReadToEnd()
         );
    }
}
bleroy commented 7 years ago

1st post above: I'm not sure you even need a record. The shopping cart should be just fine being stored in the Infoset. The reporting features you're being asked for sound like they should not influence the runtime shopping cart design. Don't make it versionable just for this. Instead, have workflow activities that can be handled to log those events, so separate reports that extract and compile the data can be generated.

2nd post: seems like composition should be used. Each implementation of a shopping cart should have a shopping cart storage injected, but should only depend on the interface.

MatteoPiovanelli-Laser commented 7 years ago

seems like composition should be used. Each implementation of a shopping cart should have a shopping cart storage injected, but should only depend on the interface

Yes, I think that's what I meant. In the "persistent" IShoppingCart I will inject the IShoppingCartStorage, but also something like an ISnoppingCartAdditionalServices to provide what is missing from the former.

MatteoPiovanelli-Laser commented 7 years ago

I'm not sure you even need a record. The shopping cart should be just fine being stored in the Infoset. The reporting features you're being asked for sound like they should not influence the runtime shopping cart design. Don't make it versionable just for this. Instead, have workflow activities that can be handled to log those events, so separate reports that extract and compile the data can be generated.

You lost me there. I can see having the carts/wishlists being in the infoset of a versioned ContentItem, with me forcing a DraftRequired when things get updated. The infoset could easily contain the "description" of the event that caused the update ("Item added to cart", "Item removed from cart", and so on...). I can also see that having activities being "triggered" by what happens to the cart could be useful to provide simple ways to expand on the behaviour. I don't get what you mean when you related those activities with the generation of reports and such. Wouldn't it be enough to go through the infosets of the different versions of the cart?

The main reason I was favoring a ContentPartVersionRecord over infosets is that I feel that would be faster to query and generate reports for stuff like "All the users who added such items to their carts in this period of time and never bought them". A User's cart history may easily become quite long and I don't know how that will affect querying over the infosets.

MatteoPiovanelli-Laser commented 7 years ago

Expanding on my ideas for the storage implementation:

In the "persistent" implementation of IShoppingCartStorage I am working on, I have a private GetCart() method, whose return value is to get the PersistentShoppingCartPart to use/update. If there is an authenticated user, we can find his cart easily as they are the owner of the ContentItem (the ContentType "ShoppingCart" also has a CommonPart to track when things change, and who changes them). If there is no authenticated user, I still wish to keep track of things, so that I can down the line merge the cart history the user generated before logging in (see how Amazon handles this). The way I would do this is to add a NumericField : ContentField to the "ShoppingCart" Type. Let's call that Field "AnonId". I would use it like that:

Regarding _anonId: On principle this could be the SessionId for the current HttpContext. However, I am planning to put an ISomethingAnonymousSomethingProvider (with a more meaningful name), whose base abstract implementation would give that SessionId. This gives the chance to easily override this default, to make life easier for stateless clients, like Native apps could be.

MatteoPiovanelli-Laser commented 7 years ago

Actually I think "AnonId" would better be in a ContentPartRecord for the PersistentShoppingCartPart, so it's easier to query/search. I think that even keeping the versioning of the cart in the infosets, I would have a ContentPartRecord to mirror the Latest/Published, so everything can be nicely indexed.

bleroy commented 7 years ago

OK, let me explain. There's the cart, in its primary usage, which is to persist items the customer wants to buy until checkout and it's moved into an order. Then there's the reporting scenario, which extracts data from various sources (including cart history) and mashes that up to present useful insights to the site administrators. Both are distinct scenarios that concern different users, and have different constraints. This is similar to the reason why people have a database for day-to-day operation, and OLAP data cubes for reporting.

I'd like to avoid having the design of the cart influenced by what we want to do with reporting: the reporting doesn't need the actual cart, and shouldn't depend on its implementation details (such as what tables implement it). It should only need the data.

Once you start focusing on the cart in its day-to-day operation, storing it (by which I mean a list of quantities of products, not operations like you seem to suggest) in the Infoset seems a lot easier and will also perform better (less joins).

bleroy commented 7 years ago

Why would you implement this anonymous id using a numeric content field?

More generally, I'm getting worried that you're about to implement a lesser session feature. If I'm anonymous, I already have persistent carts through local storage. This works today. There are also out of the box session providers that resist server restarts. Now if you're authenticated, I think we could have some additional compelling features such as wish lists.

I'd first focus on the features that really add value. I'm still not convinced we need a better way to store carts.

MatteoPiovanelli-Laser commented 7 years ago

Re: storing/reporting/versioning I understand what you mean now, and can agree with it. Rather than versioning the "ShoppingCart" items, I will raise events and trigger activities. I would do this with something like an ICartEventHandler, to provide plug-in points on adding/removing items.

Re: Anonymous Id and storing carts My concern stems from the fact that we usually deal with clients that are not web-browsers. The anonymous Id IS, as you said, a lesser session feature. Now, if the call comes from a web browser, the Anonymous ID will be the Session ID. However, if the calls do not come from a web-browser, I usually do not have an Http Session that persists there. In that case I would use a different implementation of the provider giving me the Anonymous ID, that allows me to exchange a token of some sort back and forth through an API layer to identify an anonymous user. Storing the cart then for an authenticated user plays in a similar scenario, where I may add stuff to my cart using a client (e.g. a mobile app) and then use a different client to finalize things (e.g. a web browser). Storing for anonymous users allows me to merge the cart on login, by getting whatever was added anonymously and adding it to whatever already is in the cart. Of course, the anonymous thing does not carry over between clients (e.g.: I add to cart anonymously from a mobile app, of course I will not see that when I log in on my laptop). So far, the only thing of these carts I would actually use a record for, rather than the infoset, is the AnonymousId itself, so that when I am looking for a cart by that information I don't have to read the infoset. But I definitely could be wrong in my understanding of the relative performances there, because after all we would have to do string comparison anyway (even though the record may be indexed by the anonymousId)

bleroy commented 7 years ago

A mobile client can do local storage of its cart the same way the browser does. Furthermore, any client will communicate through Http with the server, and as such will support cookies, and as a consequence, sessions, as those are a server concept supported by client side cookies.

MatteoPiovanelli-Laser commented 7 years ago

Hi. I was just talking with our mobile group. I think we can do it as you are saying with session storage also for native apps, and a persistence level to store the cart for authenticated users to "access" their cart across clients.

MatteoPiovanelli-Laser commented 7 years ago

The thing we are worrying our heads around is the following scenarios:

First: Anonymous user has this cart: Local storage: Product1, Product2 Session: Product1, Product2 Persistent: Prdocut3, Product4, added when they were logged in As they log in, Persistent and Session get merged server side, and the client is served the cart with the 4 items. This is fine.

Second: Anonymous user has this cart: Local storage: Product1, Product2 Session: EMPTY, because the session terminated in the mean time Persistent: Prdocut3, Product4, added when they were logged in As they log in, server side I would merge persistent and session, and serve to the client only Product3 and Product4. The carts in this case are not aligned. This should work by changing a bit the update logic, along the lines of:

At login the cart that comes to the client from the server is the empty session cart. In the local javascript (correct me if I am wrong because I am bad at js), that triggers a call to update. There, the server, receives the cart from the local storage, sees the session cart is empty, merges the cart from the client into the persistent cart. Then it copies the persistent cart into session and serves it to the client.

Does this make sense?

MatteoPiovanelli-Laser commented 7 years ago

sorry, I did not make my self explicit, we would get rid of that AnonymousID in this scenario, and have a Storage and Cart implementation that basically does Session storage for anonymous users, and Session+Persistent for authenticated users.

The persistent would be in the infoset of the "ShoppingCart" Item owned by the user, in the PersistentCartPart Similarly, Wishlists would be in the infoset of the "WishList" Items owned by the user, each item with its WishListPart, and TitlePart (Unlike the cart, that does not really need one).

PersistentCartPart and WIshListPart really are identical, but I would keep them as two distinct classes to avoid having to add checks in all the handlers/drivers to verify whther the part belongs to a cart or a wishlist.

MatteoPiovanelli-Laser commented 7 years ago

THe idea re: WishListPart vs SHoppingCartPart is:

public abstract class ProductListPartBase : ContentPart {
    //virtual implementation of setters and getters for the shared properties:
    protected virtual List<ShoppingCartItem> _items {
        //this serializes/deserializes to/from the infoset
    }

    public virtual List<ShoppingCartItem> Items {
        get { return _items == null ? new List<ShoppingCartItem>() : _items; }
    }

    //and so on
}

THen I would have

public class ShoppingCartPart : ProductListPartBase {
    //implement whatever is specific to carts
}

and

public class WishListPart : ProductListPartBase {
    //implement whatever is specific to wishlists
}

I am not 100% sure the abstract class is fine with Orchard like that. I am going to try.

bleroy commented 7 years ago

On merging carts: it looks like a safe proposition is to always merge when you find a discrepancy between any two persistence places (local, session, or user-bound). Merging with an empty cart is just a special case.

Inheritance in Orchard usually is a smell. Everything is designed for composition over inheritance. In this particular case, I don't see the benefits of the class hierarchy. The product list part is the same in both cases, so have only one part and use it for both content types. You would have two content types, using the same part.

MatteoPiovanelli-Laser commented 7 years ago

Inheritance in Orchard usually is a smell. Everything is designed for composition over inheritance. In this particular case, I don't see the benefits of the class hierarchy. The product list part is the same in both cases, so have only one part and use it for both content types. You would have two content types, using the same part.

As I was saying, it's a matter of being able to specialize drivers and handlers without having to check a condition on the ContentType, or on a IsCart flag. I can 100% do without the "base" abstract class, but I feel it would result in more readable code. Simple example would be the case where I do not want to see/use the quantity of products in a WishList.

Honestly, I don't know if in terms of "information contained" a WishList and a Cart are ever really different, or every difference is just in the way they are used.

Up to you, really.

bleroy commented 7 years ago

I agree that drivers and handlers should not check on a content type. A flag, maybe, but I'd like to know what specifically you have in mind for what you've described so far as "whatever is specific to wishlists". Let's have them be the same for now, and figure out how to deal with those differences when we know what they actually are. Sounds good?

MatteoPiovanelli-Laser commented 7 years ago

A flag should be fine. I honestly have no idea what could be specific to WishLists in terms of information. Behaviours, sure, that is why I'll have separate drivers/handlers/controllers. Information, I don't know. I actually asked around the office and we could not come up with anything. Does not mean it does not exist, but we could postpone worrying about that.

A thing I will probably implement today is: Anonymous user: uses local storage and session cart. Authenticated user: session cart is always empty. Whenever we add stuff, I put it into the persistent cart directly. This way, we only have stuff in session if they were added to the cart when not logged in, and it's really easy to safely merge carts then, without going crazy trying to figure out whether we are duplicating stuff. This way, I also don't have to handle the "login" event explicitly, as at the next operation/update/refresh the carts will be merged anyway. The idea is that I will be always merging the session cart into the persistent one, since the session cart will only possibly have something right after logging in.

MatteoPiovanelli-Laser commented 7 years ago

Right, I made some progress:

The current implementation in our fork (I only did the persistent cart, so far):

There is still an issue I am looking to fix. Upon logging out, the session's cart is empty, and the user cannot access the cart from the ContentItem. However, the scripts will load the cart from local storage and post it. This will put it in session storage, causing all kinds of security concerns and trouble in general

MatteoPiovanelli-Laser commented 7 years ago

uhm... I cannot seem to be able to replicate that problematic behaviour, some how.

MatteoPiovanelli-Laser commented 7 years ago

no, ok. It only seems to happen in the cart view, and not in the minicart.

bleroy commented 7 years ago

That's interesting. Yes, the cart view and minicart should always behave the same way.

So are you saying that the cart (including local storage) should be emptied when logging out?

MatteoPiovanelli-Laser commented 7 years ago

The js, upon receiving an empty cart, is triggering an update of the cart, passing the info from local storage.
The way persistent storage is implemented right now, the session cart is alway empty for authenticated users. Upon logging out then, the page receives an empty cart.

I think the local storage should be emptied then.

I have an hard time imagining a server-side check to discriminate between:

MatteoPiovanelli-Laser commented 7 years ago

One more issue with the way things are:

Emptying the storage at logout: Yes, but how can I? I mean, how do I know "logout" happened. I can logout by hitting a link, but I can also be logged out because I haven't accessed the site in ages, I guess. That is a thing that happens. In the second case, how would I intercept the event to clear local storage?

Again, checking CurrentUser == null and SessionCart is Empty defeats the point of even having a local storage, as it is not enough to tell us logout happened.

MatteoPiovanelli-Laser commented 7 years ago

What I am looking at, in terms of fixing that issue is to disable/clear all the cart's local storage for authenticated users, as we have the cart anyway. ANonymous users would use session and local storage.

On login, the session's cart would get merged into the persistent cart

On logout, session and local storage would be empty, hence we would be able to use them normally from there.

I was playing around Amazon.com earlier to see how they do things, and in terms of UX that's what happens there. Also, they don't seem tp be storing the cart in local storage: I only see a bunch of keys there, that seem unaffected by what I do to the cart.

MatteoPiovanelli-Laser commented 7 years ago

and in terms of UX that's what happens there.

I mean, as a user there:

By preventing local storage when the persistent storage for authenticated user is active, we can get at the same apparent behaviour.

MatteoPiovanelli-Laser commented 7 years ago

I should be able to do a PR on Monday. It will have the carts' work, and not yet the wishlists. Those should be easier because there's no retrocompatibility issues, as they are a new feature. Also, we already figured out most of the things that relate to them as we analyzed carts.

MatteoPiovanelli-Laser commented 7 years ago

Started doing WishLists. Some thoughts:

Wishlists only apply to authenticated users

Functionalities:

IN the "detail" view of a WL:

The list of a user's WLs is sorted by name, except that the default WL is always first.

From those considerations, I see some data differences compared to carts (I mean the part we are planning to use for both):

To make it amazon-like, the wishlist should also have an AutoroutePart, that is populated to something like wishlist/{IdString}. This sets things up to have public/shareable wishlists. As an alternative, we should design routes so they look somewhat like that, again keeping in mind the fact that we may wish to share the links, so instead of going to a WL's autoroute, we would go to a controller's action whose route looks like that. We then woul need a specific hash or string that can be shown in the frotend for sharing purposes and that has a 1-to-1 relation with our WL. Again, this would be an additional property of a WL that is not in commonn with the carts.

There are further complexities related to attributes. Where does a user get to pick them for a product? upon adding to the list? when moving to the cart?

I'll get back here, once I analyzed the efforts the different steps take, to propose a first implementation that allows expanding and implementing everything we may want or need.

MatteoPiovanelli-Laser commented 7 years ago

After more considerations and talking with @HermesSbicego-Laser, I am posting what my initial plan looks like. I used Amazon as a reference for the features, and tried to think how to make them possible down the line as expansions on what would be implemented to begin with.

Two new ContentType: WishList and WishListItem. The ContentPart used for the WishList type would not be the same as the one used in the cart, as it would contain a list of ids that let us get to the WishListItems it contains. The reason for using a WIshListItem type is that it allow customizing what is in it: in the simplest case, it'll just have a "link" to a product. However, we may expand it by adding fields like "Quantity I want", "Comments", "Date added" (easy to do with a CommonPart), and so on and so forth. So, in "migration language", I am thinking something like this:

//At the bare minimum, a wishlist is a named list of products belonging to a user.
ContentDefinitionManager.AlterTypeDefinition("WishList", cfg => cfg
                .WithPart("CommonPart") //for owner and such
                .WithPart("WishListPart") //Probably needs a different part name,or a different type name
                .WithPart("TitlePart") //wish list has a name
            );
ContentDefinitionManager.AlterTypeDefinition("WishListItem", cfg => cfg
                .WithPart("WishListElementPart")
            ); //other parts added in Back Office when configuring the ecommerce

The WishListElementPart would correspond to a WishListElementPartRecord that has:

The WishListPart would have (I think all in infoset):

I would not be implementing privacy stuff yet, if I can avoid it, but would rather leave that for a later extension feature, adding something like a WishListPrivacyPart and corresponding behaviour. I still wish to think more on this, because I need to make sure that privacy stuff can be "inserted" in the behaviours I would be implementing to begin with, which would by default be "wishlists are all private". It may turn out that it's easier to provide the privacy configuration already rather than allow for it to plug in.

The way amazon displays a wishlist is with a menu on the left side, showing all your wishlists, and on the right/main area there's the details of the current wishlist. The way I would achieve this is by providing a WishListListWidgetPart that gives the list of your wishlists. That would "cover" the functionality of the left part, while giving the flexibility of putting it somewhere else. The content's list would then be coming from a controller, and displayed such that every WishListElement has its own shape, showing for example the "summary" of the item, alongside the buttons for "Add to cart", "move to other wishlist", "remove from wishlist". The view for the controller action would then also give the "access" to the CRUD for wishlists, so that a user may create/edit/delete their wishlists.

WishListListWidgetPart would also need an alternate to have something like the "Your Lists" menu, with links to your lists, "New List..." (routes into the WishListController) Routing to wishlists would go through the WishListController using unique strings to identify each WishList (no string to be directed to the default wishlist of the authenticated user). The easiest way to do this would be to use a GUID, stored in a property of the WishListPart. I can see there may be a way to uniquely identify things by using the User's Id and the WIshlists Id to generate a string in some reversible way, that I may decode in the controller to get the required info.

I would add a driver for ProductPart to add the "Add to wishlist" button to a product's view.

The considerations about the Default WishList stand as I was thinking earlier. It is created at a user's first interaction, with a default name, and the user can then interact with it as they do with any other wishlist. The way I see it, a user can never have 0 wishlists, so if they delete their last one, a new default one is created behind their back. Initially, the default WishList would be created with title T("Your WishList"), or something like that. Down the line we may want to add a site's setting to define that default title (for each language).

More features may also be added later on. For example, the wishlist may have an associated address. There are a few ways this can be done. I could have an additional property in WishListPart that leaves room for putting in information from IWishListExtensionProvider implementations (kind of like it's done for IProductAttributeExtensionProvider). THis way, each provider will also give us the shape to be displayed, e.g. in a WIshList's settings. Alternatively, I may have additional parts in the "WishList" ContentType, and figure out a way for those to alow configuration by a front-end user. Not having analyzed all possible cases yet, I would say that I like the providers more, but I still haven't thought about the actual interfaces yet. The providers may actually give room to implement the Privacy considerations here, as an extension.

Similarly, we may think to have IWishListElementExtensionProvider. I am considering this for the case where we want to add some information and have filters/searches in the UI based on that. Probably a lot of that can be done using projections/queries on different ContentPart, and composing a menu accordingly.

Off my head, this is what I would start implementing (tomorrow). What do you think?

MatteoPiovanelli-Laser commented 6 years ago

CLosing this as the PRs have been merged