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

Jump to bottom

🐞 Marketplace: Shopper and Vendor received duplicated Order Notification #1656

Closed
Tracked by #1331
, #1672
...
anaulin opened this issue Jul 13, 2023 · 3 comments
Closed
Tracked by #1331
, #1672
...

🐞 Marketplace: Shopper and Vendor received duplicated Order Notification #1656

anaulin opened this issue Jul 13, 2023 · 3 comments
Assignees
@zspencer
Labels
🐞 bug Something isn't working

Comments

@anaulin
Copy link
Member

anaulin commented Jul 13, 2023 •
edited

When a new order was placed on one of the Piikup marketplaces, both the shopper and the convene-support@ email address received the order confirmation notification twice. Only one notification should be sent to each address, however.
@anaulin anaulin added the 🐞 bug Something isn't working label Jul 13, 2023
@anaulin anaulin added this to the 1.0 - Andromeda milestone Jul 13, 2023
@anaulin anaulin changed the title Marketplace ordering: shopper (and Convene support) received duplicated email notifications 🐞 Marketplace ordering: shopper (and Convene support) received duplicated email notifications Jul 13, 2023
@anaulin anaulin removed this from the 1.0 - Andromeda milestone Jul 13, 2023
@zspencer zspencer mentioned this issue Jul 13, 2023
Open
8 tasks
@zspencer zspencer changed the title 🐞 Marketplace ordering: shopper (and Convene support) received duplicated email notifications 🐞 Marketplace: Shopper and Vendor received duplicated Order Notification Jul 13, 2023
@KellyAH KellyAH mentioned this issue Jul 14, 2023
Closed
7 tasks
@zspencer
Copy link
Member

Gonna tilt at this one next, now that #1657 has two possible fixes in open PRs

@zspencer zspencer self-assigned this Jul 21, 2023
zspencer added a commit that referenced this issue Jul 23, 2023
@zspencer
🥗 Marketplace: Confirm Order::Notifications are sent to Shopper…
46716c4 
… and `Vendor`

- #1656

So, this is a bit of a mound-o-changes that may want to be split out; in
particular:

- `StripeCLI#with_stripe` now emits the stripe-cli's stderr and stdout to the
  Rails logger; rather than swallowing them whole

- It tells `Capybara` not to use `rack-test`
- *Successfully* forward stripe events which was apparently not
  actually happening before
- Confirm a single email is sent to the `Shopper` and `Vendor`
@zspencer zspencer mentioned this issue Jul 23, 2023
Merged
@zspencer
Copy link
Member

zspencer commented Jul 23, 2023 •
edited

Good news: I wrote a test to prove only a single OrderPlaced and OrderReceived email is sent on our end! Yay! Testing!

Bad news: The test passes

So I started digging through the logs, and what appears to be happening is:

This indicates to me a few things:

  • The transfer can't be made immediately since payments don't clear into the balance for ~3 days
  • We probably should just... capture and store the stripe event rather than attempt to perform all this work at once.

Another thing I noticed is that stripe webhooks fires events for every webhook registered on the account; which makes me think that the place we really want StripeEvents coming in to is the StripeUtility, not the Marketplace; so that we only get one event per transaction rather than 1 events per-transaction-per-marketplace-that-has-been-connected-to-stripe.

zspencer added a commit that referenced this issue Jul 25, 2023
@zspencer
🐞🔨 Marketplace: Prevent duplicate Order::Notification
fbb68f2 
- #1656

Stripe sends every event to every `Stripe::Webhook` listening for it's
type; which makes a lot of sense but does not play well with our current
design where every Marketplace creates their own `Stripe::Webhook`.

In the short-term, discarding events for orders that do not belong to
the given `Marketplace` gets us away from accidental double-mailing (or
worse, double-transfering!)
@zspencer zspencer mentioned this issue Jul 25, 2023
Merged
@zspencer
Copy link
Member

OK, once #1699 gets merged, we should be able to re-test on a Space with multiple Marketplaces and confirm only one email gets sent.

zspencer added a commit that referenced this issue Jul 27, 2023
@zspencer
🥗🐞🔨 Marketplace: Prevent duplicate Order::Notification (#1699)
8966cf8 
- #1656

Stripe sends every event to every `Stripe::Webhook` listening for it's
type; which makes a lot of sense but does not play well with our current
design where every Marketplace creates their own `Stripe::Webhook`.

In the short-term, discarding events for orders that do not belong to
the given `Marketplace` gets us away from accidental double-mailing (or
worse, double-transfering!)
zspencer added a commit that referenced this issue Jul 27, 2023
@zspencer
🥗 Marketplace: Test Order::Notification sent Shopper and `Vendo…
57a898b 
…r` (#1694)

- #1656

So, this is a bit of a mound-o-changes that may want to be split out; in
particular:

- `StripeCLI#with_stripe` now emits the stripe-cli's stderr and stdout to the
  Rails logger; rather than swallowing them whole

- It tells `Capybara` not to use `rack-test`
- *Successfully* forward stripe events which was apparently not
  actually happening before
- Confirm a single email is sent to the `Shopper` and `Vendor`
@anaulin anaulin closed this as completed Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zspencer zspencer

Labels
🐞 bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@zspencer @anaulin