-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
👷 Refactors existing Square logic to a new SquareOrder
service class
#1909
Conversation
StripeEventsController
to a new SquareOrder
service class
StripeEventsController
to a new SquareOrder
service classStripeEventsController
to a new SquareOrder
service class
app/furniture/marketplace/order.rb
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relocates all Square related behavior to the new Marketplace::SquareOrder
class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sketches the use of live sandbox credentials for e2e testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sketches of unit tests for complex internal methods of the new SquareOrder
service class. I'm thinking about this class in a way similar to an external library, therefore it feels justified to test complex private methods that are critical to the correct transmission of data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like where this is headed! Just not approving because it's still in draft.
Can't wait to see the tests!
private def create_square_order_payment | ||
square_create_payment_body = prepare_square_create_order_payment_body | ||
|
||
marketplace.square_client.payments.create_payment(body: square_create_payment_body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, just for shits and giggles.
StripeEventsController
to a new SquareOrder
service classSquareOrder
service class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the credentials for a Zinc Collective LLC Square Account and read through the code.
I think this is a good enough step forward; and left some stylistic feedback around:
- Using
delegate
andattr_accessor
to reduce the amount of setup on class instantiation - Including more context when sending information to the Square POS
- Storing significant events (Square Order, etc) in the database so that they are visible to the Marketplace Vendor and Distributor; rather than just developers.
# receiving account which are requirements for the order to show up in the | ||
# Seller Dashboard. | ||
def send_order | ||
Rails.logger.info("Start creating Square order") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loggers are great! But what I really want are events attached to the Order so that we can surface the information to the folks at Piikup:
order.events.create(description: "Payment Received") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok. A blend of misinterpreting your guidance and forgetting we had an event system implemented. Let's do it. Although I'm interested in keeping the logs around for our own benefit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the thinking around what strings go into the public.marketplace_events.description
column since it's a "free" text
type? Understandably with a small org code review serves as a guardrail to maintain consistent event descriptions/actions. But would the list of accepted descriptions strings be something we would consider declaring in some kind of enum somewhere so developers could easily consult an existing set?
I already notice we use a typical pattern of Title Case, past tense, and Noun + Verb. What about adding a small style guide comment to the Event
model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a style guide is probably better than an enum; but if you want to gather the usages I wouldn't be opposed.
end | ||
end | ||
|
||
context "when Square notifications are enabled" do | ||
let(:order) { order_for_square } | ||
let(:marketplace) { create_marketplace_with_square } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than a unique way to create data, it may be better to use FactoryBot
's trait
system and create(:marketplace, :with_square)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will follow up in #1936
@@ -65,6 +65,14 @@ | |||
with_delivery_areas | |||
with_notification_methods | |||
end | |||
|
|||
trait :with_square do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there supposed to be an Application ID? I'm adding secrets to Github and I've added:
MARKETPLACE_VENDOR_SQUARE_ACCESS_TOKEN
MARKETPLACE_VENDOR_SQUARE_LOCATION
SQUARE_ENV
MARKETPLACE_DISTRIBUTOR_APPLICATION_ID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the application id is only important if we make it an OAUTH App; which I would love for us to do at some point (so that someone can just add "Delivery by Piikup" to their Square account and magic-pony complete!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not necessary for instantiating a Square Client class.
if marketplace.square_order_notifications_enabled? | ||
order.send_to_square_seller_dashboard | ||
end | ||
marketplace.square_connection&.send_order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very tiny!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is. Although I'd still prefer a more object-oriented, explicit exposure and/or handling of nil
cases. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once you get the tests passing let's shipit!
Issue
Marketplace
:Seller
receivesOrder
in theirSquare
Point of Sale #1500Goals
Add additional test coverage for a Guest checkout
The new class should expose a single public method for executing the workflow to create an order in square. For example, from inside
stripe_events_controller.rb
or elsewhereNotes
SquareOrder
instance need to do it's work?order
should suffice to query all necessary data