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.

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: Transmits orders to Square #1733

Merged
merged 28 commits into from
Aug 28, 2023

Conversation

rosschapman
Copy link
Contributor

@rosschapman rosschapman commented Aug 5, 2023

Description

TDR (Technical Decision Records)

Testing

Local QA Trials

  • 8/24/23 - Created multi-product, multi-quantity order
    image

  • 8/23/23 - Successfully created a Square order and payment with the correct vendor share price total with the Stripe processing fee subtracted:
    image

  • 8/18/23 - Successfully created a Square order and payment by invoking the call sequence in with this Stripe CLI command: $ stripe trigger checkout.session.completed. Order shoes in my Sandbox Seller Dashboard:

    image

@rosschapman rosschapman changed the base branch from main to adds-encrypted-configuration-to-furniture August 5, 2023 01:31
@rosschapman rosschapman force-pushed the adds-placeholders-square-order-creation branch from a0d9535 to b6e9e60 Compare August 5, 2023 01:32
Base automatically changed from adds-encrypted-configuration-to-furniture to main August 8, 2023 15:34
Copy link
Member

@zspencer zspencer left a comment

Choose a reason for hiding this comment

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

This looks like it's on the right track! I also added Support for Bust a Gem so that you don't have to manually un-stage your TAGS file.

@rosschapman rosschapman changed the title 👷‍♀️ Adds placeholders for Square Order Notification pipeline 👷‍♀️ Adds logic for Square Order Notification pipeline Aug 18, 2023
@rosschapman rosschapman requested a review from zspencer August 18, 2023 21:08
.env.example Outdated
@@ -31,3 +31,6 @@ OPERATOR_API_KEY=a-secret-that-should-get-changed
STRIPE_API_KEY=get-this-from-your-stripe-account

NEIGHBORHOOD_TIME_ZONE="Pacific Time (US & Canada)"

# Configure Square environment
# SQUARE_ENV="production"|"sandbox"
Copy link
Member

Choose a reason for hiding this comment

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

This probably should be set in the Marketplace for now, rather than a global variable.

Copy link
Member

@zspencer zspencer left a comment

Choose a reason for hiding this comment

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

It's on a good path; I would be happy for us to merge this if:

  • We get rid of the global variable storing sandbox vs production
  • It did not include the TAGS or the change to launch.json
  • It had some amount of tests. I'm happy to pair on writing the tests; or do a branch-off-your-branch and submit a patch with some tests.

square_order_id,
square_location_id,
space_id,
# TODO: what should be canonical pricing data here: stripe_balance_transaction or order data?
Copy link
Member

Choose a reason for hiding this comment

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

I would expect this to be the sellers_share of the order; not the full balance or order amount.

Order::ReceivedMailer.notification(order).deliver_later
order.events.create(description: "Notifications to Vendor and Distributor Sent")
Order::PlacedMailer.notification(order).deliver_later
order.events.create(description: "Notification to Buyer Sent")

SplitJob.perform_later(order: order)
else
raise UnexpectedStripeEventTypeError, event.type
# raise UnexpectedStripeEventTypeError, event.type
Copy link
Member

Choose a reason for hiding this comment

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

This probably should still be there, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops yes.

"script": "${file}",
"args": [],
"askParameters": true
},
Copy link
Member

Choose a reason for hiding this comment

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

Is removing this part of doing Square stuff? Or is it incidental?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh not sure what happened here. Oops.

location_id = marketplace.settings["square_location_id"]
customer_id = shopper.id

stripe_fee_per_line_item = (payment_processor_fee_cents / ordered_products.sum(&:quantity))
Copy link
Contributor Author

@rosschapman rosschapman Aug 23, 2023

Choose a reason for hiding this comment

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

@zspencer @anaulin I can't tell if this is tacky or not, but it quickly felt like the best way to make sure line items are separated in the Square order and the vendor share total is accurately calculated. From the PoV of the seller in their Square dashboard, they don't see this line item level fee. It merely comes in as the price total.

Copy link
Member

Choose a reason for hiding this comment

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

This calculation feels like a dangerous idea from a book-keeping perspective. They are selling the product for $10, not $10 - a_fraction_of_the_stripe_fees.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I figured as much. There's gotta be a better way around this. Square seems to compute and validate the line item total generated during order creation against the payment total during payment creation. If they don't match, the payment fails.

The trick is to have the vendor/seller line items appear with the correct amounts. Need to think on this more. It could be we add a negative service charge with the order creation data for the Stripe fee. But I'm not sure sellers are supposed to be aware of the Stripe fee?

Copy link
Member

Choose a reason for hiding this comment

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

Can we tell Square that the Payment is for the total?

quantity: ordered_product.quantity.to_s,
item_type: "ITEM", # ITEM|CUSTOM_AMOUNT|CGI
base_price_money: {
amount: (ordered_product.price_cents - stripe_fee_per_line_item),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rosschapman rosschapman requested a review from zspencer August 23, 2023 01:05
Copy link
Member

@zspencer zspencer left a comment

Choose a reason for hiding this comment

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

This is looking pretty good!

A few things:

  1. Why is TAGS checked in here?
  2. My gut says we should be passing the full amounts directly through; and not doing any kind of fee-deductions. The PoS doesn't need to know about payment-processor fees; just how much money we collected on their behalf. It's not like a sandwhich that costs $10 is registered as sold at "$9.08" by the PoS because someone used MasterCard and $9.15 because someone else used Visa...

That said, I am down to approve this once the TAGS are deleted because I could be 100% wrong and I would rather we move towards prod and learn.

def square_environment
if Rails.env.production?
"production"
end
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if this was a value stored in the Marketplace; since we do need to be able to use the sandbox environment in a Rails production context.

Even if we don't make it to the UI; it does not seem appropriate to flag based upon deployment environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may have misinterpreted your earlier feedback as put on marketplace model for some reason. Oof, tough getting used to jumping in and out of a codebase on limited time!

Copy link
Member

Choose a reason for hiding this comment

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

Well, this is definitely the right place for it in our current domain model; it's just not quite how I would decide whether it's prod or not.

And yea, the "put work down and pick it back up in narrow time windows" is a hard skill!

location_id = marketplace.settings["square_location_id"]
customer_id = shopper.id

stripe_fee_per_line_item = (payment_processor_fee_cents / ordered_products.sum(&:quantity))
Copy link
Member

Choose a reason for hiding this comment

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

This calculation feels like a dangerous idea from a book-keeping perspective. They are selling the product for $10, not $10 - a_fraction_of_the_stripe_fees.

@zspencer zspencer changed the title 👷‍♀️ Adds logic for Square Order Notification pipeline 🥔✨ Marketplace: Transmits orders to Square Aug 23, 2023
@zspencer zspencer changed the title 🥔✨ Marketplace: Transmits orders to Square 🥔✨👷 Marketplace: Transmits orders to Square Aug 23, 2023
@rosschapman rosschapman force-pushed the adds-placeholders-square-order-creation branch from cd3bb2a to 052453b Compare August 24, 2023 19:38
@rosschapman rosschapman marked this pull request as ready for review August 24, 2023 19:45

# TODO: Add `square_environment` attribute to database/model
def square_client
@square_client ||= Square::Client.new(access_token: marketplace.square_access_token, environment: marketplace.square_environment)
Copy link
Member

Choose a reason for hiding this comment

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

Since we're reading data from the marketplace to create the square_client, that implies to me that it maybe wants to live on the marketplace, and the Order class can delegate :square_client, to: :marketplace?

@@ -24,6 +23,10 @@ def create
order.update!(status: :paid, placed_at: DateTime.now, payment_processor_fee_cents: balance_transaction.fee)
order.events.create(description: "Payment Received")

if marketplace.square_order_notifications_enabled?
order.send_to_square_seller_dashboard
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on encapsulating this conditional within the method? Basically, Order#send_to_square_seller_dashboard is a no-op if marketplace.square_order_notifications_enabled? is false.

Copy link
Member

@zspencer zspencer left a comment

Choose a reason for hiding this comment

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

Ship it!

@rosschapman rosschapman merged commit 8505a83 into main Aug 28, 2023
@rosschapman rosschapman deleted the adds-placeholders-square-order-creation branch August 28, 2023 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants