Pull requests

#3 Declined
ems/Nwazet.Commerce-Old Nwazet.Commerce-Old
bleroy/Nwazet.Commerce Nwazet.Commerce

PayPal, Promotions/Discounts, Product Recommendations, Minimum Quantity, Cart History

  1. Jeffrey Olmstead

Hi Bertrand,

Being new at the pull request field, I will just throw all of this out there for you to dialog on if you choose. I see you implemented a Promotions/Discounts module. There are some features you have that I don't and some that I have you don't, which means merging that facet might be a royal pain. Either way, before any additional modules are implemented I wanted to see if we could merge some of these solutions. If not, I am concerned mine will permanently be a separate fork, which wasn't my intent when building. I know now that it is better to do pull requests on smaller projects at a time, will do so going forward. Look forward to your thoughts.


  • Learn about pull requests

Comments (25)

  1. Jeffrey Olmstead author

    I am looking to get my extensions of Nwazet.Commerce back in line with your core module (I really want to tap into using Stripe - and contribute of course!).

    How do you recommend I go about this? Do you still intend to merge and pull the information I have below into your core or should I go ahead and maintain them as separate / dependent modules? If separate / dependent modules, I would need some extension points built into your core for it to work so would also need to know if you are open to that.

  2. Bertrand Le Roy repo owner

    The Strip changes are in now, and they are going pretty deep. We have order management now for example. Can you please try to take the latest changes in from the main repo, do the merge, and then make a new pull request?

    1. Jeffrey Olmstead author

      Sorry if comes through multiple times, has not been taking comment in browser

      I just got the merge to compile, which was a decent size feat. The biggest areas where I need to blend (and need feedback) are:


      • I have a Promotion module and you have a Promotions module
      • Your module seems to be a time based discount with the ability to target roles and apply the discount on a select quantity
      • My module allows a discount to be set by users who have a discount code, supports single use code and can be conditioned on an existing product id
      • In summary, some of the concepts and fields overlap which begs the question of whether to expand your module or to have a parallel module, what are your thoughts?

      Product Recommendations

      • I have a recommendations module which allows you to conditionally recommend products
      • For example, if you add Product A to your cart, then Product B will be recommended (it also has logic of making conditional)

      I mention this as this module needs an interface called from the Shopping Cart Controller (i.e. where you have CheckoutServices, ShipppingMethodProviders, and ExtracartInfoProviders I would also now add RecommendationService. This is a feature I need with clients, do you like the model of integration or do you see another way?

      Minimum Order Quantity

      • This is not a new module, this is simply a field I added on the ProductPart
      • It seems a very broad requirement for many users but wanted to let you know that would be a deviation from your current "core" and I want to get your feedback on it

      It looks like everything else will integrate with very minimal impact to the base. I look forward to hearing your thoughts on the above items.

  3. Bertrand Le Roy repo owner

    I think for promotions we should be able to reconcile both approaches. Mine relies on IPromotion that should apply well to yours as well. You may need to add a hook in the shopping cart so that the form to enter the promotion code gets rendered. This will be a very welcome addition. If you can look at how it's done for shipping methods and taxes, where a single interface allows several implementations to share the same admin UI, that would be great. Your promotion feature could be called coupons, or something along those lines. If you need to extend the interface, that's fine.

    Recommendations sound fine, and you are welcome to add the hooks you need to the shopping cart. I would like to ask you to add some tests however, as this is a very sensitive piece of code, but one that is so far extensively tested.

    Minimum order quantity sounds like a more than acceptable addition. I don't need it myself, but I see how it may be very useful to others.

    Thanks for your contributions, and for your understanding as I force you to do complicated merges...

  4. Jeffrey Olmstead author

    I have been studying your code base to ensure I am not duplicating efforts. My goal is to utilize all the extension points that are available. To this effect, I have started with a clean code pull of the Nwazet.Commerce module and am making my modules work as an external solution to ensure I am only touching what needs done (if you like what the modules are doing then it is real easy to make then part of yours and if not they can forever remain separate and others can use them that way). I feel I have all of the features integrated but one - hence the question.

    I have a feature called Nwazet.History and you have Nwazet.Orders. I fully appreciate the value of your module and want to do away with my "History" module. However, my "History" module has a feature I cannot do without, it records the userid of the purchaser (or allows it to be set in the Admin). Either way, this allows me to control digital product releases (which is point number 2 that I would need an option on ProductPart to require login). In short, do you feel these can be native features of Nwazet.Commerce:

    1. Nwazet.Orders feature would expand OrdarPart to capture "UserId" (would be -1 if no authenticated user)
    2. Nwazet.Products feature would expand ProductPart to capture "AuthenticationRequired"

    The logic for both of the above is pretty simple to implement and I feel they have some value across the board. It would allow my other modules to then work. To keep things moving, I will move forward with integrating them and they will show up in the pull request unless I hear differently.

    Side note, I think you want to add [OrchardFeature("Stripe")] at the top of the StripeController.

  5. Jeffrey Olmstead author

    Finalizing my data changeover from History to Orders. When I try to export "Order" the XML doesn't provide "Items" or "Activity" correctly:

    <Order Id="/Identifier=f170c2e6b0374233b3f12aaa2fdf3f2d" Status="Published">
          <OrderPart CustomerEmail="jeffre_1287967192_per@gmail.com" CustomerPhone="" IsTestOrder="true" Password="Xxx7Ql5k1Fw=" SpecialInstructions="" Status="Accepted" SubTotal="50" Total="50" TrackingUrl="">
            <Shipping Description="" ShippingCompany="" Price="0" />
            <Taxes Name="" Amount="0" />
            <ShippingAddress Honorific="" FirstName="" LastName="" Address1="" Address2="" City="" Company="" Country="" PostalCode="" Province="" />
            <BillingAddress Honorific="" FirstName="Test" LastName="User" Address1="1 Main St" Address2="" City="San Jose" Company="" Country="United States" PostalCode="95131" Province="CA" />
          <IdentityPart Identifier="f170c2e6b0374233b3f12aaa2fdf3f2d" />
          <CommonPart Owner="/User.UserName=manager" CreatedUtc="2013-12-23T15:56:11Z" PublishedUtc="2013-12-23T19:04:06Z" ModifiedUtc="2013-12-23T19:04:06Z" />

    Both of these are trying to add children XML to already children xml and use this similar code block:

    .AddEl(new XElement(ItemsName,
                        part.Items.Select(it =>
                            new XElement(ItemName).With(it)
                                .ToAttr(i => i.ProductId)
                                .ToAttr(i => i.Title)
                                .ToAttr(i => i.Quantity)
                                .ToAttr(i => i.Price))))

    Is this an issue with just my environment or something there also. I can look for a fix if it doesn't work there also. Thanks for letting me know.

  6. Jeffrey Olmstead author

    I have fixed the export, just needed a .Element tacked onto the end of it to force it to evaluate the expression. It will be in the pull request. Pretty close now - have a good one.

  7. sobasi

    I just realized this dialog on changing/merging History and Order. I'm working on a site that uses Paypal. I saw this pull request and started with ems/Nwazet.Commerce, hoping to replace it with the core when pull is complete and 1.8 is out.

    The thing is, the site will be selling a service, not goods. I want to update the service request (records) when PayPal trasaction is complete. I could not find a suitable extension point without replicating code, so I decided to create a filter on History item publish event, since it does create a relation btw. purchased product with the user.

    I would like to ask if this is a good way to do this, or is there some other way I missed. Even if it is, I figure it will change when the merge is done. So is there a way I can avoid rewriting that part when I upgrade to 1.8?

  8. sobasi

    I try not to touch the commerce module. I'm not a good tester, don't know almost nothing about paypal or e-commerce, and I'm affraid I will break it if I do so.

    History publish, being done in a IPaymentCompleteService, is the end of the paypal transaction workflow in ems module. It looked least harmful to attach something to the end, which I can implement in my own modules. I guess I can do something with SuppressDependency in my module as well, but it involves replicating code, which I don't like.

    But I must admit it's indirect. Do you advise to change the module code or suppress the service injection against it?

  9. Bertrand Le Roy repo owner

    I advise to change the module code. There should be an interface for code that gets called when a transaction has been completed, and that all payment modules should call into if they can. Something like ICommerceTransactionCompleted with a method that passes in the list of product quantities for the order, and other known information.

  10. Jeffrey Olmstead author

    I too will need an ICommerceTransactionCompleted interface also as I need to be able to record the coupon codes that were "consumed" by a user. So if you don't end up getting it done, I will end up building it out as a result of the integration I am doing. I am actually looking to take my changes live on some websites I have and will then post a pull request for the initial integration. After that I will be looking to more tightly integrate the modules (which will result in ICommerceTransactionCompleted interface being requested in a pull request and possibly some others).

  11. Jeffrey Olmstead author

    No, this should be removed (I will try to decline it) as I moved to the technique of only integrating the core needs. The remainder of these modules can be found here: https://bitbucket.org/ems/rework.commerceextensions.

    If you see any code in the Commerce Extensions you want in Nwazet.Commerce we can of course integrate it otherwise it will stand as it's own module. I would want to optimize the code further prior to integrating if you do want any of the features in the "core".

  12. Jeffrey Olmstead author

    Excellent. In all honesty, PayPal needs the most work to be up to par. Coupons works good with the only caveat being they are not consumed (so no single use working right now - may need to expand Nwazet.Commerce to support consumption). I am actively working on both of these modules to make them better so what I will do, when I feel they are ready, is put up a new fork on this one, bring the modules over, and then do a pull request. Just to recap on the modules that exist outside core:

    • Role Assignment
    • PayPal
    • Coupons
    • Product Recommendation

    Sad, but PayPal is the one I care least about as I am moving towards Stripe though I see the value of having it.