🐞🔨 Marketplace: DeliveryAreas cannot be Removed when in use
#2015
Conversation
| # rubocop:disable Rails/SkipsModelValidations | ||
| delivery_area.carts.update_all(delivery_area_id: nil) | ||
| # rubocop:enable Rails/SkipsModelValidations | ||
| delivery_area.destroy |
There was a problem hiding this comment.
Might be worth wrapping these two lines in a transaction.
There was a problem hiding this comment.
Random thought: could we accomplish this with something like this on the Delivery Area model?
has_many :pre_checkout_carts, ...appropriate scopes go here..., dependent: :nullifyThere was a problem hiding this comment.
Maybe? I forgot about dependent: :nullify; but the test coverage should prove it out.
| @@ -13,7 +13,7 @@ def create | |||
|
|
|||
| when "checkout.session.completed" | |||
| payment_intent = Stripe::PaymentIntent.retrieve(event.data.object.payment_intent, {api_key: marketplace.stripe_api_key}) | |||
| order = marketplace.orders.find_by(id: payment_intent.transfer_group) | |||
| order = Order.unscoped.find_by(id: payment_intent.transfer_group, marketplace:) | |||
There was a problem hiding this comment.
I don't love how this reads. It immediately makes me want to go and check why does this need to be unscoped and what does that mean. Does that mean that really this is a "cart_or_order"? Or should this be a "cart"? Or something else?
There was a problem hiding this comment.
This is a consequence of that default scope that I added; I'm going to revert that.
app/furniture/marketplace/order.rb
Outdated
| @@ -1,6 +1,7 @@ | |||
| class Marketplace | |||
| class Order < Record | |||
| self.table_name = "marketplace_orders" | |||
| default_scope { where.not(status: :pre_checkout) } | |||
There was a problem hiding this comment.
Why do we need a default scope here? How certain are we that this won't introduce yet another bug somewhere where an edge case isn't tested? (I'm not certain at all, personally.)
I'd rather we explicitly applied named scopes as needed, instead of having a default scope.
There was a problem hiding this comment.
Yea, this is a side effect of not having split the marketplace_carts and marketplace_orders tables out. I like putting it in a named scope!
OK, this should be a bit nicer. There's some other cleanup I want to do in another PR to: - Rip out the Turbo Streams (they don't seem useful) - Make the button say "Archive" when there are Orders
zspencer
force-pushed
the
marketplace/delivery-areas/cannot-be-removed-when-in-use
branch
from
160f3ac to
d600a0c
Compare
| has_many :orders, inverse_of: :delivery_area | ||
| has_many :carts, inverse_of: :delivery_area | ||
| has_many :orders, -> { checked_out }, inverse_of: :delivery_area | ||
| has_many :carts, inverse_of: :delivery_area, dependent: :nullify |
There was a problem hiding this comment.
Ah, and this works because Marketplace::Carts has a default scope of :pre_checkout, right? Nice
There was a problem hiding this comment.
Yes, which is part of why I kinda-like the default scopes; but also I now have a ticket for the refactor: #2020
zspencer
deleted the
marketplace/delivery-areas/cannot-be-removed-when-in-use
branch
This:
destroyDeliveryAreaswith anOrderCart#delivery_areaonDeliveryAreas#destroy