🧹🔨🥗`Marketplace`: Show `Order#delivery_window` in `Order::EmailReceiptComponent` #1319

zspencer · 2023-04-06T15:44:47Z

🥔✨ Marketplace: DeliveryConstraints #1185

So, the delivery_window field which can either be a plain string or a particular date or even a date range is a much more sophisticated object.

I took some time to explore what it could look like to use ActiveModel::Types to create a value object which can take responsibility for representing the data more appropriately.

I think the next step would be to back the Delivery class with a marketplace_deliveries table and move Order#contact_email, etc. into it.

In the meantime, I also added some tests to where the delivery_window is being used, so that when / if we make that change we have some guard rails in place.

No UI changes, but this does add capybara and configure our view_component library for testing.

zspencer

This is a weird PR. Apologies in advance.

I noticed that the Order#delivery_window was not showing up in the EmailReceiptComponent in production, but it was in the mailer_preview. That was because I was using a FactoryBot.build(:marketplace_order) to populate the values, so the delivery window had never needed to be retrieved from the database! And I had done the same in the Marketplace::OrderPlacedMailer specs... So at no time was Order#delivery_window being called on an Order that had actually been pulled out of the database.

Womp. Womp.

And when I was testing it via the UI, I was using the Cart object... Which did have a Cart#delivery_window.

I started by just copy and pasting the delivery_window method into Order, but that offended my oh-so-delicate sensibilities and I revolted. And since I ... reallllllly want.... a Delivery class..... I thought "Well, what if I sprout that Delivery model?! It sounds like the time!"

And I wanted to relearn the ActiveRecord::Type bits since I haven't had much time to do hands-on-keys coding in a bit; and I want to have a better feel for how the pieces work together...

So anyway, that's all the justification I have for why I didn't create a marketplace_deliveries table and store the data that I've been copy-pasting between tables.

I could see wanting to go down that path sooner rather than later tho. I was really hoping the Type stuff would feel like a solid win...

I'm going to add some more precise tests at the Type and Model level.

app/furniture/marketplace/policy.rb

zspencer · 2023-04-07T05:48:33Z

spec/furniture/marketplace/cart/deliveries_controller_request_spec.rb

@@ -35,7 +35,7 @@
        expect { perform_request }
          .to change(delivery, :contact_email).to(delivery_attributes[:contact_email])
          .and change(delivery, :contact_phone_number).to(delivery_attributes[:contact_phone_number])
-          .and change(delivery, :delivery_window).to(DateTime.parse(delivery_attributes[:delivery_window]))
+          .and change { delivery.reload.delivery_window.value }.to(delivery_attributes[:delivery_window])


This feels clunky as heck

spec/furniture/marketplace/delivery/window_spec.rb

spec/furniture/marketplace/delivery/window_type_spec.rb

app/furniture/marketplace/order/email_receipt_component.html.erb

zspencer · 2023-04-07T06:00:21Z

app/furniture/marketplace/order.rb

@@ -12,6 +12,7 @@ class Order < Record
    has_many :products, through: :ordered_products, inverse_of: :orders

    has_encrypted :delivery_address
+    attribute :delivery_window, ::Marketplace::Delivery::WindowType.new


I feel like this should work with delegate :window, to: :delivery, prefix: true

Note! It does not becausebecomes does not re-initialize the data from the database, so the attribute needs to be selected and cast(ed?)

This will work when we make a marketplace_deliveries table instead of using the becomes hack.

zspencer · 2023-04-07T06:08:11Z

app/furniture/marketplace/cart.rb

-      DateTime.parse(super) || 48.hours.from_now
-    rescue Date::Error => _e
-      48.hours.from_now
-    end


But hey at least I didn't copy and paste this code everywhere 🤣

app/furniture/marketplace/cart.rb

zspencer · 2023-04-07T06:27:52Z

app/views/application/_datetime_field.html.erb

 <div>
  <%= form.label attribute %>
-  <%= form.datetime_field attribute %>
+  <%= form.datetime_field attribute, disabled: disabled %>


I feel like this could be merged independently, and maybe wombo-comboed with a FieldComponent so more of our :x_field partials have standardized inputs.

Wombo-comboed! That's a new one for me.

- #1185 So, the `delivery_window` field which can either be a plain string or a particular date or even a date range is a much more sophisticated object. I took some time last night to explore what it could look like to use `ActiveModel::Type`s to create a value object which can take responsibility for representing the data more appropriately. That said, I think `store_model` will be better ™️, but I didn't want to try to dig into that just yet.

We can't use `alias_method` with `attribute`, which is weird but ok.

…nd `Delivery::WindowType`

OK, that makes sense... We keep the type castin gin the type... and that makes everything else lighter.

Keeps the refactor just a tiny bit more cleaner and full

anaulin · 2023-04-08T02:51:03Z

app/furniture/marketplace/delivery/window.rb

+      alias_method :==, :eql?
+
+      def time_like?
+        value.respond_to?(:strftime)


Nit: maybe inlining this method would make the strftime method clearer -- you are calling strftime if it responds to it -- saving the extra indirection of going "is time_like? a built-in thing? oh, no, it is defined down here, let's see... oh, i see".

Yea, there's an indirection bump but time_like? is used in a few other places (in particular figuring out which field to render for the form and how to present the window on the receipt)

Ah, fair enough.

anaulin · 2023-04-08T02:52:38Z

app/furniture/marketplace/delivery/window.rb

+
+      def strftime(*args, **kwargs)
+        return value.strftime(*args, **kwargs) if time_like?
+        value


Callers of strftime will probably expect a string coming back from this call. Is value a string? Or can it be something else that should be cast to a string? (ETA: Something else that isn't time-like)

Oo that's a good point... I could just call to_s? For now, value is either a string or a DateTime; but if we start wanting it to hold a date range or other less primitive data structures it will be weird to return them.

anaulin

It's not that weird. But perhaps I'm not the best judge of that.

Thanks for all the extra testing goodness!

zspencer force-pushed the marketplace/delivery-schedule-is-shown-on-order-confirmation branch from b60fd1c to 38ff75f Compare April 7, 2023 05:34

zspencer commented Apr 7, 2023

View reviewed changes

app/furniture/marketplace/cart.rb Outdated Show resolved Hide resolved

zspencer commented Apr 7, 2023

View reviewed changes

zspencer requested review from anaulin and a team April 8, 2023 01:20

zspencer changed the title ~~Marketplace: Sprout Delivery::Window object~~ 🧹🔨🥗Marketplace: Sprout Delivery::Window object Apr 8, 2023

zspencer marked this pull request as ready for review April 8, 2023 01:21

zspencer added ✨ feature Reduces Client's Burden or Grants them Benefits 🛠️ infrastructure ci, build, deploy, networking, etc. 🥗 test automation Adds some automated tests. V nutritious. labels Apr 8, 2023

zspencer added 8 commits April 7, 2023 18:58

Ahahah! It did it! It did the thing

e68a63a

Marketplace: Spec for the Delivery model!

9932864

Marketplace: spec the Delivery#window alias

8a1a487

We can't use `alias_method` with `attribute`, which is weird but ok.

Marketplace: Make sure the delivery window shows up in the email!

390ab87

Marketplace: Undo the weird name resolution

3edf378

Marketplace: A bit more low-level tests on the Delivery::Window a…

f27d517

…nd `Delivery::WindowType`

Marketplace: Move all type casting to the Type

cd0c585

OK, that makes sense... We keep the type castin gin the type... and that makes everything else lighter.

zspencer force-pushed the marketplace/delivery-schedule-is-shown-on-order-confirmation branch from d2414a0 to cd0c585 Compare April 8, 2023 01:58

Marketplace: Oooooh yea! Factories!

980092d

Keeps the refactor just a tiny bit more cleaner and full

zspencer changed the title ~~🧹🔨🥗Marketplace: Sprout Delivery::Window object~~ 🧹🔨🥗Marketplace: Show Order#delivery_window in Order::EmailReceiptComponent Apr 8, 2023

anaulin reviewed Apr 8, 2023

View reviewed changes

anaulin approved these changes Apr 8, 2023

View reviewed changes

zspencer merged commit 3d6825d into main Apr 8, 2023

zspencer deleted the marketplace/delivery-schedule-is-shown-on-order-confirmation branch April 8, 2023 03:08

zspencer added this to the 1.0 - Andromeda milestone May 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🧹🔨🥗`Marketplace`: Show `Order#delivery_window` in `Order::EmailReceiptComponent` #1319

🧹🔨🥗`Marketplace`: Show `Order#delivery_window` in `Order::EmailReceiptComponent` #1319

zspencer commented Apr 6, 2023 •

edited

Loading

zspencer left a comment

zspencer Apr 7, 2023

zspencer Apr 7, 2023

zspencer Apr 7, 2023 •

edited

Loading

zspencer Apr 8, 2023

zspencer Apr 7, 2023

zspencer Apr 7, 2023

anaulin Apr 8, 2023

anaulin Apr 8, 2023

zspencer Apr 8, 2023

anaulin Apr 8, 2023

anaulin Apr 8, 2023 •

edited

Loading

zspencer Apr 8, 2023

anaulin left a comment

🧹🔨🥗Marketplace: Show Order#delivery_window in Order::EmailReceiptComponent #1319

🧹🔨🥗Marketplace: Show Order#delivery_window in Order::EmailReceiptComponent #1319

Conversation

zspencer commented Apr 6, 2023 • edited Loading

zspencer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zspencer Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anaulin Apr 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anaulin left a comment

Choose a reason for hiding this comment

🧹🔨🥗`Marketplace`: Show `Order#delivery_window` in `Order::EmailReceiptComponent` #1319

🧹🔨🥗`Marketplace`: Show `Order#delivery_window` in `Order::EmailReceiptComponent` #1319

zspencer commented Apr 6, 2023 •

edited

Loading

zspencer Apr 7, 2023 •

edited

Loading

anaulin Apr 8, 2023 •

edited

Loading