zinc-collective / convene

An Operating System for the Solidarity Economy
https://convene.zinc.coop
Other
56 stars 19 forks source link

🥗✨🧹 `Marketplace`: Every `Marketplace` in a `Space` shares `TaxRate`s #1466

Closed zspencer closed 1 year ago

zspencer commented 1 year ago

OK this is a bit larger than I wanted it to be, but it should be a purely structural change.

Once this is merged, we'll want to merge a follow-up PR (This one!) to:

Things I'm not certain on:

  1. I was torn on creating a Bazaar table instead of inheriting from Space... I didn't want to attach tax_rates to the Neighborhood level Space; since that would have "leaked" the Marketplace implementation into the Neighborhood; but adding a new table felt like Too Much.
  2. Bazaar may not be the right word; it's likely that it could work just as well as Marketplace::Space, but I know we've had challenges with namespaced domain objects; so I figured getting rid of the collisions is maybe OK? Or maybe not?
zspencer commented 1 year ago

I suppose a tighter way to split the "structure/behavior" change would have been to do the following PRs:

  1. Sprout the TaxRate#bazaar, populate it, and write to it on TaxRate#{create,update}
  2. Add the TaxRate#bazaar foreign key
  3. Switch to read from bazaar.tax_rates rather than marketplace.tax_rates
  4. Drop the TaxRate#marketplace field
anaulin commented 1 year ago

Why not just use the space_id to aggregate all the TaxRates, Shoppers, etc, for all the Marketplaces in a Space?

zspencer commented 1 year ago

I started out trying to do something like this on the product/form view:

TaxRate.where(marketplace: Marketplace.where(room: space.rooms))

And that kinda half-worked; but after setting the product.tax_rates to include a tax rate from another marketplace, exceptions started popping up.

So I figured the "safest" thing to do was "re-anchor" the tax-rate to the Space in the object graph rather than trying to walk up from the Marketplace to the Space and back down across all spaces.

Another option (rather than a bazaar class) I left as "Option B" was adding in an affordance for a Marketplacae::Route to append_space_routes that would allow a particular Furniture/Gizmo/whatever but that would have required rejiggering the UI more immediately.

anaulin commented 1 year ago

Fair enough. Computers.

zspencer commented 1 year ago

I'm still not convinced that Bazaar is the right thing; but eff it we can always rename the field and class later 🙃

anaulin commented 1 year ago

Seems like this broke bin/setup:

[...]
rake aborted!
ActiveRecord::RecordInvalid: Validation failed: Bazaar must exist
/Users/anaulin/src/github.com/zinc-collective/convene/lib/tasks/release.rake:13:in `block (3 levels) in <main>'
/Users/anaulin/src/github.com/zinc-collective/convene/lib/tasks/release.rake:7:in `block (2 levels) in <main>'
Tasks: TOP => release:after_build
(See full trace by running task with --trace)
Shutting down background worker
Killing session flusher

== Command ["bin/rake release:after_build"] failed ==
anaulin commented 1 year ago

FWIW, I just did a rails db:reset and that fixed this issue. Possibly something to do with janky local development data.

zspencer commented 1 year ago

I've noticed that my dev enviroment also gets sadder over time; mostly due to foreign keys not being set (my bad)... but I'm glad reset worked.