Marketplace: Differentiate between Checkout and Order #1057

zspencer · 2023-01-20T01:52:47Z

Where Checkout is useful for handling a Checkout flow for the Payment Processor, it doesn't make a ton of sense from a Buyer perspective once the Cart has been Checked out.

I've sprouted an Order model, with corresponding Controllers, and moved the checkouts/show and checkouts/_checkout views to orders/show and orders/_order.

zspencer · 2023-01-20T01:53:46Z

app/furniture/marketplace.rb

-      end
-      router.resources :checkouts, only: [:show, :create]
-    end
-  end


I moved these to furniture/marketplace/routes.rb which I think will make it easier to find routes because you can use a fuzzy-finder and type marketplace routes and get the routes; instead of having to remember that they live in furniture/marketplace.rb

zspencer · 2023-01-20T01:54:42Z

app/furniture/marketplace/checkout.rb

+    def complete(stripe_session_id:)
+      update!(status: :paid, stripe_session_id: stripe_session_id)
+      cart.update!(status: :checked_out)
+    end


I figured shoveling this in here is probably better than leaving it in the controller; but maybe I'm wrong.

I think it's okay to leave the complete method here for now. It's just updating the cart record right? It's not doing anything in request/response flows -- which is what the checkouts_controller is for right?

Yep, it's doing the data transformations; so the controller just happens to know that showing a checkout when stripe gives it an id it also completes the checkout.

I like to leave conversations open, rather than "resolving" them. Resolving feels like it dismisses the conversation! You had good questions! Other people may have them too!

zspencer · 2023-01-20T01:56:12Z

app/furniture/marketplace/checkouts_controller.rb

-      else
-        Shopper.find_or_create_by(person: current_person)
-      end
-    end


I pulled these into marketplace/controller.rb since it was being used in many places.

zspencer · 2023-01-20T04:45:22Z

app/furniture/marketplace/cart.rb

@@ -12,6 +12,7 @@ class Cart < ApplicationRecord
    has_many :cart_products, dependent: :destroy, inverse_of: :cart
    has_many :products, through: :cart_products, inverse_of: :carts
    has_one :checkout, inverse_of: :cart
+    has_one :order, inverse_of: :cart, class_name: "Order"


I don't think the class_name is actually useful here 🤦

what happens if you remove it? what breaks?

I don't think anything will, I just added it sillilly.

Remove it! 🪄 ✨ 🔥 it! 😄

KellyAH · 2023-01-20T05:49:50Z

spec/furniture/marketplace/checkouts_controller_request_spec.rb

        expect(cart.reload).to be_checked_out
        expect(checkout.reload).to be_paid
+        expect(response).to redirect_to(checkout.becomes(Marketplace::Order).location)


The changes in this PR seem like a large enough refactor that I assumed there'd be more unit tests added/modified to provide test coverage for the new Order model, corresponding Controllers, and orders/show, orders/_order. But maybe it's just the over tester in me.

Can you clarify where you're seeing risk here? At this point, we don't have significant deal-flow; so flexibility in mutating the design seems more valuable than enshrining the design via tests.

that's a good point. 👍

I think there are definitely risks here, I just don't know which ones are worth writing tests for yet! If you have ideas about which particular cases you'd like tested (either cross-request or individual request) I would be happy to implement them!

KellyAH · 2023-01-20T06:03:10Z

app/furniture/marketplace/orders_controller.rb

+      policy_scope(marketplace.orders).find(params[:id])
+    end
+  end
+end


I am trying to visualize the way all these data models interact as the user goes thru the shopping flow is. Is it like this?

A customer adds items to their cart == a Cart of pre_checkout status contains cart_products.

The customer goes to the checkout page and is filling out credit card info to complete their purchase == a Cart of pre_checkout status is going thru the stripe checkout flow thus creating a Checkout record.

if the checkout flow via stripe is successful, the Cart changes to check_out status, and an Order is created from the cart_products.

So, let's decompose the datastructure a bit here:

at the database layer, a Checkout and an Order are equivalent; they both store their attributes in the same table.

At the application layer, a Checkout and an Order have different routes, controllers, policies, and views.

At present, whether something is represented as a Checkout or an Order depends on what url is being used to retrieve the data. If the request path includes marketplace/*/checkouts/* then the data stored in the database will be treated as a Checkout. If it's marketplace/*/orders then the data stored in the database will be treated as an Order.

So, an Order is not 'created' in the sense that it's not actually creating any records in the database table; rather we leverage "polymorphism" (which can be loosely defined as 'the ability for one data structure to be treated as another data structure') so that we have more Domain objects for different use cases; while retaining a more normalized database.

It's entirely possible that we do want an Order to be a distinct database table from a Checkout, or we may not want a Checkout to not be a database-backed model at all!

So, to reframe:

Shopper adds Products to their Cart, creating a CartProduct record which keeps track of things like quantity. (note: I think a Cart is also a kind of Order or Checkout, which I think @anaulin may agree with)

Shopper creates a Checkout, which directs them to Stripe to collect payment information.

Shopper completes Checkout, which directs them back from Stripe to Convene, and updates the Checkout object to indicate it had been succesfully checked-out.

Shopper is redirected to show the Order, which represents the Cart to Shoppers in a read-only manner.

wow this is great! once Marketplace is finished/finalized/stable we could make a seperate .MD doc readme with the above info and maybe a diagram ❤️

I think it could be dropped directling into app/furniture/marketplace/README.md in it's own PR :D

KellyAH · 2023-01-20T06:06:46Z

app/furniture/marketplace/routes.rb

+      end
+    end
+  end
+end


why does Marketplace get it's own routes.rb file? Why not just put it in
https://github.com/zinc-collective/convene/blob/main/config/routes.rb

If we put in in config/routes.rb then every developer maintaining a piece of Furniture would need to modify routes.rb in order to adjust the routes their Furniture users. Instead, we hook in to the Furniture class in config/routes.rb and tell it to iterate through every piece of furniture, and append the routes defined within the Furniture itself.

This allows Furniture pieces to "own" the code related to their furniture within the app/furniture/furniture_name folder; which reduces the amount of surface area a Furniture maintainer needs to cover in order to make significant changes to their Furniture's behavior.

TL/DR: Encapsulation! Every piece of Furniture owns it's entire implementation, from Routes to Models to Controllers to Policies to Views; within the app/furniture/furniture_name directory.

Where `Checkout` is useful for handling a Checkout flow for the Payment Processor, it doesn't make a ton of sense from a Buyer perspective once the Cart has been Checked out. I've sprouted an `Order` model, with corresponding Controllers, and moved the `checkouts/show` and `checkouts/_checkout` views to `orders/show` and `orders/_order`.

#831 From: #1057 (comment)

anaulin

I don't see the need to have both a Checkout and an Order class. It seems a Checkout is simply an Order that is in the "initiated but not completed" state.

More broadly, personally I find that this style of code-reuse-via-inheritance makes codebases harder to understand and maintain. (And in general I am strongly pro composition-over-inheritance.)

All that said, I'm not going to stand in your way if you want to take things in this direction.

#831 From: #1057 (comment)

zspencer · 2023-01-20T21:27:11Z

I've found in Rails codebases in particular, they become easier to maintain in a highly-rigorous domain-driven approach; which includes not having extraneous domain objects. Right now, I don't see Checkout, Cart, and Order as extraneous from a domain perspective, (as a Cart and an Order are things people reason about differently, and Checkout is just a ... weird abstraction) but I do think that they are extraneous from a database perspective.

My hope is that:

I can merge the marketplace_carts and marketplace_checkouts table into a single marketplace_orders table; where the domain models do not inherit from each other, and
I can remove the Checkout class from being ActiveRecord backed; and leave it as a domain object whose only job is to handle the Stripey-bits.

Does this sound like something you can be on board with?

anaulin

My experience is that when folks insist on having a "highly-rigorous domain-driven approach" we end up with oddly convoluted structures, because of trying to shoe-horn the inevitable oddnesses of real-world needs and interactions into something that is "perfectly abstracted and modeled" instead of, say, using the occasional command pattern.

Not to talk about how highly debatable this approach can be: for example, in my opinion, "Checkout" makes no sense as a domain model for a marketplace, because it in fact represents a verb (the action of checking out) and not an noun, and thus has no business being a model.

All that said, I'm happy for you check in this or do whatever you feel is the best way to move this forward. I'm sure we'll shake out the kinks as we go along regardless.

zspencer · 2023-01-20T22:25:58Z

I agree that we'll find the kinks and shake them out. I also agree that a Checkout is an absurd model; and I want it to be very tiny or not exist at all if we can get there.

zspencer requested review from KellyAH, anaulin, sadiejay and a team January 20, 2023 01:52

zspencer commented Jan 20, 2023

View reviewed changes

KellyAH reviewed Jan 20, 2023

View reviewed changes

zspencer added 2 commits January 19, 2023 22:47

Marketplace: I don't think we *need* the class_name here.

6c4e3f1

zspencer force-pushed the marketplace/refactor/shift-checkout-to-order branch from d6b9eb0 to 6c4e3f1 Compare January 20, 2023 06:47

Lint!

452c877

zspencer added a commit that referenced this pull request Jan 20, 2023

Marketplace: Lay out the HCI for Shopper Purchasing Products

9877300

#831 From: #1057 (comment)

zspencer mentioned this pull request Jan 20, 2023

Marketplace: Lay out the HCI for Shopper Purchasing Products #1061

Merged

anaulin reviewed Jan 20, 2023

View reviewed changes

zspencer added a commit that referenced this pull request Jan 20, 2023

Marketplace: Lay out the HCI for Shopper Purchasing Products (#1061)

a320067

#831 From: #1057 (comment)

anaulin approved these changes Jan 20, 2023

View reviewed changes

zspencer merged commit 65f1614 into main Jan 20, 2023

zspencer deleted the marketplace/refactor/shift-checkout-to-order branch January 20, 2023 22:27

zspencer added this to the 1.0 - Andromeda milestone May 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Marketplace: Differentiate between Checkout and Order #1057

Marketplace: Differentiate between Checkout and Order #1057

zspencer commented Jan 20, 2023

zspencer Jan 20, 2023

zspencer Jan 20, 2023

KellyAH Jan 20, 2023

zspencer Jan 20, 2023

zspencer Jan 20, 2023

zspencer Jan 20, 2023

zspencer Jan 20, 2023

KellyAH Jan 20, 2023

zspencer Jan 20, 2023

KellyAH Jan 20, 2023

zspencer Jan 20, 2023

KellyAH Jan 20, 2023

zspencer Jan 20, 2023

KellyAH Jan 20, 2023

zspencer Jan 20, 2023

KellyAH Jan 20, 2023

zspencer Jan 20, 2023

KellyAH Jan 20, 2023

zspencer Jan 20, 2023

KellyAH Jan 20, 2023

zspencer Jan 20, 2023 •

edited

Loading

anaulin left a comment

zspencer commented Jan 20, 2023

anaulin left a comment

zspencer commented Jan 20, 2023

Marketplace: Differentiate between Checkout and Order #1057

Marketplace: Differentiate between Checkout and Order #1057

Conversation

zspencer commented Jan 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zspencer Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

anaulin left a comment

Choose a reason for hiding this comment

zspencer commented Jan 20, 2023

anaulin left a comment

Choose a reason for hiding this comment

zspencer commented Jan 20, 2023

zspencer Jan 20, 2023 •

edited

Loading