✨ `Marketplace`: Collect `Shopper`s Delivery Address before `Checkout` #1173

zspencer · 2023-03-02T23:21:45Z

🗺️ Marketplace: DeliveryArea #1136

While collecting the address via Stripe was reasonable at the time, if we want to support dynamic pricing, we'll need the address ahead of time.

This will also (theoretically) allow us to let people know when they are outside of the Distributor delivery area.

After!

split-setting-address-from-cart.mp4

Before

before-address-collected.mp4

zspencer · 2023-03-02T23:31:32Z

app/furniture/marketplace/carts/_cart.html.erb

-    </tbody>
-    <%= render "marketplace/carts/footer", cart: cart %>
-  </table>
+<div id="<%= dom_id(cart) %>">


Sorry for the indentation change, but I needed to put in an id for the whole shebang so we could replace it all at once :X.

zspencer · 2023-03-02T23:32:23Z

app/furniture/marketplace/checkout.rb

@@ -16,9 +16,6 @@ def create_stripe_session(success_url:, cancel_url:)
        mode: "payment",
        success_url: success_url,
        cancel_url: cancel_url,
-        shipping_address_collection: {
-          allowed_countries: ["US"],
-        },


We don't want to get it from Stripe anymore, since we get it before hand.

I assume: "get it before hand." from the user via this code change that allows the user to input their delivery address in a field.

zspencer · 2023-03-02T23:33:37Z

app/furniture/marketplace/carts/_total.html.erb

@@ -1,10 +1,12 @@
-<span id="cart-total-<%= cart.id%>" class="w-full flex flex-col">
-  <span class="text-right">
+<span id="cart-total-<%= cart.id%>" class="w-full flex flex-col sm:text-right">


We were setting the text-right in every span, which seemed excessive.

KellyAH · 2023-03-03T07:34:14Z

The After! video shows the saved address not auto-populating the field when the change address button is clicked. Was that fixed to not show a blank field when changing a saved address? It would sux as a user being forced to retype the whole address if they only needed to fix a typo.

KellyAH · 2023-03-03T07:44:03Z

app/furniture/marketplace/carts/_cart.html.erb

-    </tbody>
-    <%= render "marketplace/carts/footer", cart: cart %>
-  </table>
+<div id="<%= dom_id(cart) %>">


what is dom_id? what does dom stand for?

nevermind. found it in https://apidock.com/rails/ActionView/RecordIdentifier/dom_id

Yea, it's part of the Rails Model/View/Controller coupling; where a unique id attribute is generated from the model element. It's pretty useful in turbo land!

KellyAH · 2023-03-03T07:51:15Z

app/furniture/marketplace/carts/_footer.html.erb

-      <%= button_to("Checkout", cart.location(child: :checkout), data: { turbo: false }) %>
+      <%- if cart.delivery_address.present? %>
+        <%= button_to("Checkout", cart.location(child: :checkout), data: { turbo: false }) %>
+      <%- end %>


this seems to make the checkout button hidden if the user never inputted a cart.delivery_address. Customers might worry when they don't see the checkout button.

Why not just have the Checkout disabled if cart.delivery_address does not exist or is empty string? Or always keep Checkout enabled, but if they submit the form without the cart.delivery_address, do not let the form submit and inform them that cart.delivery_address is required.

disabled is better than hidden for sure! I considered making it so if it wasn't set, the CheckoutsController#show action gave them an affordance for adding the Cart#delivery_address but I decided against it because it was more code.

I also considered putting the entire cart except for the delivery_address behind a if/else block; but that seemed excessive.

@KellyAH - Do you have a preference between:

Disabling the checkout button unless the delivery_address is set, or

Hiding the cart until it has a delivery_address

Part of me thinks option 2 is actually worth doing; since it serves as a forcing function and reduces the amount of elements on the page significantly...

wouldn't 2️⃣ hiding the cart, freak the customer out since they can't see their cart items. thus they don't know what is in their cart/what happened to their cart.

IMHO 1️⃣ makes it clear that some other required action is needed to be allowed to proceed to the checkout flow.

I think we can presume a lot about what would or would not freak a customer out, but I don't like to design out of an expectation of fear or anxiety.

It's not terribly unusual for clicking a link to navigate to a different page; so a customer freaking out because when they clicked a link they went somewhere else would surprise me.

My hesitation on 2️⃣ is that I just couldn't visualize how it would look in the UI/UX.

If the flow looks fine with the 2️⃣ change and is something you as a customer would be comfy with. I say do 2️⃣ 👍 😄

As we say at work: "Do whatever is easiest/simplest. Because we can always iterate/refactor later"

Here is 2️⃣'s flow! I think it's pretty decent (but it needs better text! lol)

split-setting-address-from-cart.mp4

KellyAH · 2023-03-03T08:01:03Z

app/furniture/marketplace/cart.rb

@@ -26,7 +26,11 @@ def product_total
      end
    end

-    delegate :delivery_fee, to: :marketplace
+    def delivery_fee
+      return marketplace.delivery_fee if delivery_address.present?


I assume this logic is covering the fact that the cart.delivery_fee cannot be calculated without a delivery_address so it sets delivery_fee to zero.

app/furniture/marketplace/carts/_cart.html.erb

app/furniture/marketplace/carts/_total.html.erb

zspencer · 2023-03-03T20:27:09Z

The After! video shows the saved address not auto-populating the field when the change address button is clicked. Was that fixed to not show a blank field when changing a saved address? It would sux as a user being forced to retype the whole address if they only needed to fix a typo.

So if I were to make it that smart, I would likely need to promote the delivery_address from a String to a full-on Model, which I'm not opposed to but felt like it was too big of a step for this PR. I had started down that path, going so far as sprouting a new Record for Marketplace::DeliveryAddress, which belongs_to :shopper and has_many :carts and has_many :orders.

But that would have wound up adding a few hundred lines of code; and I didn't want to do that just yet.

KellyAH · 2023-03-03T20:37:28Z

spec/furniture/marketplace/carts_controller_request_spec.rb

+
+  describe "#update" do
+    it "changes the delivery address" do
+      cart = create(:marketplace_cart, marketplace: marketplace, shopper: shopper)


why can't the cart be a let outside the it block? Is it because the cart instance must be created fresh for every it block? And making cart a let would make it a shared object accessed between it blocks and that would make tests too coupled or possibly cause tests to be flaky if they use/modify the same cart object?

Nah, it's because I didn't have multiple it statements; but I could pull this into a let/before/subject structure if ya want.

leave it. it can be pulled out if more it statements are ever added.

Wound up finding a bug during my testing, which made me want to write a second test case! So let it is!

KellyAH · 2023-03-03T20:40:41Z

spec/furniture/marketplace/cart_spec.rb

+    context "when the #delivery_address is present" do
+      let(:cart) { create(:marketplace_cart, delivery_address: "123", marketplace: marketplace) }
+
+      it { is_expected.to eql(product_a.price + product_b.price + product_b.price + marketplace.delivery_fee) }


why is product_b.price listed twice? Is that a typo or is the test increasing product_b quantity in the cart just by adding a 2nd product_b.price?

Yep, the cart_product that references product_b has a quantity of 2; I could do product_b.price * 2 if that is more clear?

IMHO and nitpick: product_b.price * 2 looks more clear and intentional.

KellyAH · 2023-03-03T20:47:11Z

app/furniture/marketplace/carts_controller.rb

+          redirect_to cart.room.location
+        end
+        format.turbo_stream do
+          render turbo_stream: [turbo_stream.replace(dom_id(cart), cart)]


Is this logic using turbo stream to AJAX-like update/rerender only the cart item elements on the webpage instead of rerendering the whole page?

- #1136 While collecting the address via Stripe was reasonable at the time, if we want to support dynamic pricing, we'll need the address ahead of time. This will also (theoretically) allow us to let people know when they are outside of the `Distributor` delivery area.

Co-authored-by: KellyAH <[email protected]>

KellyAH

waiting for https://github.com/zinc-collective/convene/pull/1173/files#r1124122135 to be resolved, then I will approve this PR. The rest of the code looks great 💎 😄

It's probably worth pushing some of these expectations down to the `cart_policy_spec` level, but the bug manifested at the controller tier so I wanted to catch it there.

zspencer changed the title ~~✨ Marketplace: Collect Shoppers Delivery Address before checkout~~ ✨ Marketplace: Collect Shoppers Delivery Address before Checkout Mar 2, 2023

zspencer requested review from anaulin, KellyAH, a team and daltonrpruitt March 2, 2023 23:29

zspencer force-pushed the marketplace/collect-delivery-address branch from 1f9dc06 to d764872 Compare March 2, 2023 23:34

zspencer commented Mar 2, 2023

View reviewed changes

zspencer added the ✨ feature Reduces Client's Burden or Grants them Benefits label Mar 2, 2023

KellyAH reviewed Mar 3, 2023

View reviewed changes

app/furniture/marketplace/carts/_cart.html.erb Outdated Show resolved Hide resolved

KellyAH reviewed Mar 3, 2023

View reviewed changes

app/furniture/marketplace/carts/_total.html.erb Outdated Show resolved Hide resolved

KellyAH reviewed Mar 3, 2023

View reviewed changes

zspencer and others added 2 commits March 3, 2023 12:50

✍️ Marketplace: Wordsmith!

bc7f76b

Co-authored-by: KellyAH <[email protected]>

zspencer force-pushed the marketplace/collect-delivery-address branch from 383d57f to bc7f76b Compare March 3, 2023 20:50

KellyAH suggested changes Mar 3, 2023

View reviewed changes

zspencer added 2 commits March 3, 2023 13:11

✨ Marketplace: Don't show Cart until there's a delivery_address

b3b7280

🐞 Marketplace: Ensure Guests can update Cart

314d4d0

It's probably worth pushing some of these expectations down to the `cart_policy_spec` level, but the bug manifested at the controller tier so I wanted to catch it there.

zspencer requested a review from KellyAH March 3, 2023 21:23

KellyAH approved these changes Mar 3, 2023

View reviewed changes

zspencer merged commit e501597 into main Mar 3, 2023

zspencer deleted the marketplace/collect-delivery-address branch March 3, 2023 21:25

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`: Collect `Shopper`s Delivery Address before `Checkout` #1173

✨ `Marketplace`: Collect `Shopper`s Delivery Address before `Checkout` #1173

zspencer commented Mar 2, 2023 •

edited

Loading

zspencer Mar 2, 2023

zspencer Mar 2, 2023

KellyAH Mar 3, 2023

zspencer Mar 3, 2023

zspencer Mar 2, 2023

KellyAH commented Mar 3, 2023 •

edited

Loading

KellyAH Mar 3, 2023

KellyAH Mar 3, 2023

zspencer Mar 3, 2023

KellyAH Mar 3, 2023

zspencer Mar 3, 2023

zspencer Mar 3, 2023

KellyAH Mar 3, 2023 •

edited

Loading

zspencer Mar 3, 2023 •

edited

Loading

KellyAH Mar 3, 2023 •

edited

Loading

zspencer Mar 3, 2023

KellyAH Mar 3, 2023

zspencer Mar 3, 2023

zspencer commented Mar 3, 2023

KellyAH Mar 3, 2023

zspencer Mar 3, 2023

KellyAH Mar 3, 2023

zspencer Mar 3, 2023

KellyAH Mar 3, 2023

zspencer Mar 3, 2023

KellyAH Mar 3, 2023

zspencer Mar 3, 2023

KellyAH Mar 3, 2023

zspencer Mar 3, 2023

KellyAH left a comment

✨ Marketplace: Collect Shoppers Delivery Address before Checkout #1173

✨ Marketplace: Collect Shoppers Delivery Address before Checkout #1173

Conversation

zspencer commented Mar 2, 2023 • edited Loading

After!

Before

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

KellyAH commented Mar 3, 2023 • edited Loading

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

KellyAH Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

zspencer Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

KellyAH Mar 3, 2023 • edited Loading

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 commented Mar 3, 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

KellyAH left a comment

Choose a reason for hiding this comment

✨ `Marketplace`: Collect `Shopper`s Delivery Address before `Checkout` #1173

✨ `Marketplace`: Collect `Shopper`s Delivery Address before `Checkout` #1173

zspencer commented Mar 2, 2023 •

edited

Loading

KellyAH commented Mar 3, 2023 •

edited

Loading

KellyAH Mar 3, 2023 •

edited

Loading

zspencer Mar 3, 2023 •

edited

Loading

KellyAH Mar 3, 2023 •

edited

Loading