Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Marketplace: Collect Shoppers Delivery Address before Checkout #1173

Merged
merged 4 commits into from
Mar 3, 2023
Merged

✨ Marketplace: Collect Shoppers Delivery Address before Checkout #1173

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fe15761
✨ `Marketplace`: Collect `Shopper`s Delivery Address before checkout
zspencer Mar 2, 2023
bc7f76b
✍️ `Marketplace`: Wordsmith!
zspencer Mar 3, 2023
b3b7280
✨ `Marketplace`: Don't show `Cart` until there's a `delivery_address`
zspencer Mar 3, 2023
314d4d0
🐞 `Marketplace`: Ensure `Guest`s can update `Cart`
zspencer Mar 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion app/furniture/marketplace/cart.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,11 @@ def product_total
end
end

delegate :delivery_fee, to: :marketplace
def delivery_fee
return marketplace.delivery_fee if delivery_address.present?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly.


0
end

def price_total
product_total + delivery_fee
Expand Down
24 changes: 24 additions & 0 deletions app/furniture/marketplace/cart_policy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# frozen_string_literal: true

class Marketplace
class CartPolicy < ApplicationPolicy
alias_method :cart, :object
def permitted_attributes(_params = nil)
%i[delivery_address]
end

def create?
true
end

def update?
cart.shopper.person == current_person
end

class Scope < ApplicationScope
def resolve
scope.all
end
end
end
end
66 changes: 43 additions & 23 deletions app/furniture/marketplace/carts/_cart.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,44 @@
<div class="-mx-4 mt-8 overflow-hidden shadow ring-1 ring-black ring-opacity-5 sm:-mx-6 md:mx-0 md:rounded-lg">
<table class="min-w-full divide-y divide-gray-300 table-fixed">
<thead class="bg-gray-50">
<tr>
<th scope="col" class="py-3.5 pl-4 pr-3 text-left text-sm font-semibold text-gray-900 sm:pl-6">
<%= Marketplace::Product.human_attribute_name(:name) %>
</th>
<th scope="col" class="hidden px-3 py-3.5 text-left text-sm font-semibold text-gray-900 lg:table-cell">
<%= Marketplace::Product.human_attribute_name(:description) %>
</th>
<th scope="col" class="hidden px-3 py-3.5 text-left text-sm font-semibold text-gray-900 sm:table-cell">
<%= Marketplace::Product.human_attribute_name(:price) %>
</th>
<th scope="col" class="w-48">&nbsp;</th>
</tr>
</thead>
<tbody class="divide-y divide-gray-200 bg-white">
<%- cart.marketplace.products.each do |product| %>
<%= render cart.cart_products.find_or_initialize_by(product: product) %>
<%- end %>
</tbody>
<%= render "marketplace/carts/footer", cart: cart %>
</table>
<div id="<%= dom_id(cart) %>">
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is dom_id? what does dom stand for?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


<%- if cart.delivery_address.blank? %>
<%= form_with model: cart.location do |cart_form| %>
<%= render "text_field", attribute: :delivery_address, form: cart_form %>

<%= cart_form.submit %>
<%- end %>
<%- else %>
<div class="flex flex-wrap">
<p class="p-3">
Delivering to <%= cart.delivery_address %>

</p>
<%= button_to "Update Address", cart.location, params: { cart: { delivery_address: nil } }%>
</div>

<%- end %>

<div class="-mx-4 mt-8 overflow-hidden shadow ring-1 ring-black ring-opacity-5 sm:-mx-6 md:mx-0 md:rounded-lg">
<table class="min-w-full divide-y divide-gray-300 table-fixed">
<thead class="bg-gray-50">
<tr>
<th scope="col" class="py-3.5 pl-4 pr-3 text-left text-sm font-semibold text-gray-900 sm:pl-6">
<%= Marketplace::Product.human_attribute_name(:name) %>
</th>
<th scope="col" class="hidden px-3 py-3.5 text-left text-sm font-semibold text-gray-900 lg:table-cell">
<%= Marketplace::Product.human_attribute_name(:description) %>
</th>
<th scope="col" class="hidden px-3 py-3.5 text-left text-sm font-semibold text-gray-900 sm:table-cell">
<%= Marketplace::Product.human_attribute_name(:price) %>
</th>
<th scope="col" class="w-48">&nbsp;</th>
</tr>
</thead>
<tbody class="divide-y divide-gray-200 bg-white">
<%- cart.marketplace.products.each do |product| %>
<%= render cart.cart_products.find_or_initialize_by(product: product) %>
<%- end %>
</tbody>
<%= render "marketplace/carts/footer", cart: cart %>
</table>
</div>
</div>
5 changes: 3 additions & 2 deletions app/furniture/marketplace/carts/_footer.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@
<tr>
<td class="text-left px-3 py-3.5 text-right" colspan="8">
<%= render "marketplace/carts/total", cart: cart %>
<%= 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 %>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KellyAH - Do you have a preference between:

  1. Disabling the checkout button unless the delivery_address is set, or
  2. 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...

Copy link
Contributor

@KellyAH KellyAH Mar 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@zspencer zspencer Mar 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@KellyAH KellyAH Mar 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

split-setting-address-from-cart.mp4

</td>

</tr>
</tfoot>
12 changes: 7 additions & 5 deletions app/furniture/marketplace/carts/_total.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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">
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<span>
Products:
<%= humanized_money_with_symbol(cart.product_total) %></span>
<span class="text-right">Delivery:
<%= humanized_money_with_symbol(cart.delivery_fee) %></span>
<span class="text-right font-bold">
<%- if cart.delivery_address.present? %>
<span>Delivery Fee:
<%= humanized_money_with_symbol(cart.delivery_fee) %></span>
<%- end %>
<span class="font-bold">
Total: <%= humanized_money_with_symbol(cart.price_total) %>
</span>
</span>
24 changes: 24 additions & 0 deletions app/furniture/marketplace/carts_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
class Marketplace
class CartsController < Controller
def update
authorize(cart)
cart.update(cart_params)
respond_to do |format|
format.html do
redirect_to cart.room.location
end
format.turbo_stream do
render turbo_stream: [turbo_stream.replace(dom_id(cart), cart)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep!

end
end
end

def cart
@cart ||= policy_scope(marketplace.carts).find(params[:id])
end

def cart_params
policy(Cart).permit(params.require(:cart))
end
end
end
3 changes: 0 additions & 3 deletions app/furniture/marketplace/checkout.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

payment_intent_data: {
transfer_group: cart.id
}
Expand Down
8 changes: 7 additions & 1 deletion spec/furniture/marketplace/cart_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,13 @@
cart.cart_products.create!(product: product_b, quantity: 2)
end

it { is_expected.to eql(product_a.price + product_b.price + product_b.price + marketplace.delivery_fee) }
it { is_expected.to eql(product_a.price + product_b.price * 2) }

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) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

end
end

describe "#product_total" do
Expand Down
21 changes: 21 additions & 0 deletions spec/furniture/marketplace/carts_controller_request_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
require "rails_helper"

RSpec.describe Marketplace::CartsController, type: :request do
let(:marketplace) { create(:marketplace) }
let(:space) { marketplace.space }
let(:room) { marketplace.room }
let(:person) { create(:person) }

let(:shopper) { create(:marketplace_shopper, person: person) }

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

sign_in(space, person)

put polymorphic_path(cart.location), params: {cart: {delivery_address: "123 N West St"}}

expect(response).to redirect_to(room.location)
end
end
end