3437 supplier name on invoice #3444

mkllnk · 2019-02-06T04:11:27Z

What? Why?

Closes #3437.

Simply adding the supplier name to each line item on the invoice allows a food hub to use the invoice as packing sheet.

What should we test?

Create an order and look at the emailed template.
Change the invoice template as super-admin in Settings/ Invoice Settings.
View an order as admin and look at the invoice through the action "Print Invoice"

Release notes

Changed: Invoices now contain the supplier name for each item.

Changelog Category: Changed

The spec just tested if a job was enqueued, but not if the job can actually be executed. Unfortunately, this test is quite slow.

luisramos0 · 2019-02-06T10:50:10Z

build broken. looks related to the PR.

luisramos0

Looks good just a method name error that is breaking the build.
We mentioned java interfaces yesterday, today is time for compiled languages where this is validation is for the IDE.

app/controllers/spree/admin/orders_controller_decorator.rb

sauloperez · 2019-02-06T20:32:26Z

app/services/invoice_renderer.rb

@@ -0,0 +1,29 @@
+class InvoiceRenderer
+  def render_to_string(order)
+    renderer.render_to_string(args(order))


This class would clearly benefit from moving order to an ivar populated in the constructor IMO

I don't see it as clearly. I see great benefit in declaring all input variables for each method. It makes refactoring easier. It also allow to call the same renderer with different orders. The BulkInvoiceService should probably do that.

sauloperez · 2019-02-06T20:33:35Z

app/services/invoice_renderer.rb

+      "spree/admin/orders/invoice"
+    end
+  end
+end


nice abstraction 👏

sauloperez · 2019-02-06T20:35:06Z

app/views/spree/admin/orders/_invoice_table2.html.haml

@@ -17,6 +17,9 @@
      %tr
        %td
          = render 'spree/shared/line_item_name', line_item: item
+          %br
+          %small
+            %em= raw(item.variant.product.supplier.name)


any chance we could pass something like supplier_name to this view? It'd reduce coupling a great deal but I'm not familiar with this view at all though.

This is actually within a loop. The only variable for the template is @order and this template then goes through the items:

@order.line_items...each do |item| #... end

We could put the content of the loop in a separate template, but I don't see the benefit here.

I see... what triggered my brain's attention was the violation of the Law of Demeter that item.variant.product.supplier.name causes. Don't you think?

The only solution that comes to my mind is to encapsulate it in LineItem#supplier_name. This at least decouples line item from all this knowledge. So it'd look something like raw(item.supplier_name).

EDIT https://www.youtube.com/watch?v=l9JXH7JPjR4is a fun explanation of the Law of Demeter 😂 (starts at 2:47)

I see, a very important point. I always hesitate to modify Spree code though. I'm also wondering if it's the responsibility of a line item to know the supplier name. Two ideas:

Provide LineItem#supplier via the decorator. Less knowledge for the line item and the view can select what to display from the supplier: raw(item.supplier.name).

Or provide a helper that can select the supplier name of a line item, variant or product: supplier_name(item).

If LineItem was our class I would definitely go for the first option, but since it's Spree, I'm not sure. What do you think?

sauloperez · 2019-02-06T20:37:35Z

app/services/invoice_renderer.rb

+    renderer.render_to_string(args(order))
+  end
+
+  def args(order)


this method is not used outside the class, right? in that case, it's better if we move it to private to keep the public API to the minimum.

The OrdersController calls this:

def print render InvoiceRenderer.new.args(@order) end

I tried to move the render call into the InvoiceRenderer but that failed badly.

RuntimeError (ActionController::RackDelegation#content_type= delegated to @_response.content_type=, but @_response is nil: #<ApplicationController:0x005630be1fca30 @_routes=nil, @_action_has_layout=true, @_headers={"Content-Type"=>"text/html"}, @_status=200, @_request=nil, @_response=nil, @_config={}, @_prefixes=["application"], @_lookup_context=#<ActionView::LookupContext:0x005630be144340 @details_key=#<ActionView::LookupContext::DetailsKey:0x005630c488b0f8 @hash=-3205369017263900504>, @details=...

I think render tries to store variables like the content type in the request object of the controller. But in the service we just have a new ApplicationController without any request object stored in there. render is only to be called on a the active controller that actually received a request. This is another example why internal state is bad.

I know there is a way to move render outside the controller although I never used it but I don't think we need it here. I was just talking about the #args method itself but I see what you did now 👍

sauloperez · 2019-02-06T20:38:04Z

spec/services/bulk_invoice_service_spec.rb

+      Delayed::Job.last.invoke_job
+
+      expect(service.invoice_created?(service.id)).to be_truthy
+    end


Doesn't it make more sense to have such integration-like spec in the controller and stick to unit-level in this spec file?

I'm testing only the interface of this service here. What makes it integration-like? Do you mean the creation of an order in the database?

How would you unit-test the logic of this method?

IMO creating a job an depending on Delayed Job is what makes this an integration test, other than the order but you know... we're too coupled to AR.

To make it unit I would use a test double rather than an actual CombinePDF object and assert that we call #save on it passing the file name, since that's its public API. We can do that with an instance_double.

Then, I'd move to the controller test the responsibility to check that a background job is enqueued using DelayedJobHelper.

Let me know if you want to pair on it or if I should provide more details.

Good point @sauloperez. This test isn't about Delayed Job at all. Your comment made me realise that I can invoke the method without delayed job to test it's actual execution. Have a look at my new commit. That's much clearer.

Also big thanks for the Sandy Metz talk about unit testing. Very simple and clear. My conclusion is though that I don't want to introduce doubles in this spec for several reasons:

Doubles need to be kept in sync with the API of the thing they are imitating. Sandy mentions that you should have tools for that in order to make this stable and efficient. I don't think we have that in place. We don't have a double which verifies that the doubled method exists on the real object.

This spec currently uses the public API of the service. Mocking requires knowledge of the internal workings. And that is bad, because changing the implementation of the service then requires changing the spec as well. The current spec should not need adjustments as long as the API stays the same.

Sandy's main reason for doubles is to stay close to the object under test. We don't want to call create_pdf and then check for a file on the filesystem within the spec. The filesystem is far away. But my spec doesn't go far away at all. It uses only the API of the object under test.

The remaining valid reason for doubles is speed. In this case though, I think that all the stability gained is worth the added 3 seconds.

What do you think?

This is definitely better now without the explicit call to DelayedJob although due to its design we're still coupled to it (That's why I'm not a big fan of this handle_asynchronously magic it implements).

We don't have a double which verifies that the doubled method exists on the real object.

That talk is old and RSpec's instance_double and class_double didn't exist at that time. Fortunately, things are better now and we already use them in our tests 🎉 !

Mocking requires knowledge of the internal workings.

What you actually mock is the objects the class under test depends on. You never mock the subject of the test as this wouldn't reflect what happens in production. And that is what makes it unit. You put the class in an isolated test harness (like in a chemistry lab) to ensure the unit does what it says.

We don't want to call create_pdf and then check for a file on the filesystem within the spec.

Of course not, that would be integration. If you need to do so is because the class under test is too coupled to the filesystem (generally speaking here).

I always see consider a test as a symptom of the class' design. If it's hard to put it in a test harness is because the design I implemented needs to improve. If it is so while testing in isolation chances are it'll be much harder when I need to refactor or reuse in the codebase. In these cases, what is a real game changer is to inject the dependencies in the constructor. It gives the possibility to replace them and makes coupling stand out.

The remaining valid reason for doubles is speed. In this case, though, I think that all the stability gained is worth the added 3 seconds.

Sure, not a big deal but I see these 3 seconds in every spec. And this really adds up.

Great discussion. I really enjoy talking about these ideas but let's move on. I think this is better now but I'll raise the topic again in the future, be sure of that

@sauloperez

I'm not a big fan of this handle_asynchronously magic

I totally agree! We should avoid that in future code.

That talk is old and RSpec's instance_double and class_double didn't exist at that time.

Thank you! I didn't know that. I definitely want to use that more. (Just not here. ;-) )

The outcome feels good now. 👍

sauloperez · 2019-02-06T20:40:05Z

spec/services/invoice_renderer_spec.rb

+    alternative = service.render_to_string(order)
+    expect(alternative).to match /^%PDF/
+    expect(alternative).to_not eq result
+  end


IMO this deserves to be split in two tests, one for each invoice_template branch, which it'd make the class and the test more maintainable. Besides, as it is, looks more like a capybara test.

Haha, I had a comment there explaining why and then removed it. This is the second time that I removed a comment and then you ask me about it. I should never remove one of my comments again.

Here are some reasons:

You can't test if the two results are different without rendering both results in the same test.

Splitting the test means generating the pdf files several times which costs spec running time.

What do you think? Is it worth it splitting the spec into three and pay with 6 seconds more run time?

it renders an invoice pdf

it renders an alternative invoice pdf

it renders two different pdfs

This is the second time that I removed a comment and then you ask me about it. I should never remove one of my comments again.

Splitting the test means generating the pdf files several times which costs spec running time.

That is what mocking is for. If this is a unit test, it shouldn't depend on anything else. Checking the various related unit units work together is the responsibility of an integration test.

I strongly recommend you https://youtu.be/URSWYvyc42M. I'll never explain it as well as Sandy Metz.

Co-Authored-By: mkllnk <[email protected]>

mkllnk · 2019-02-06T23:13:45Z

@sauloperez Thank you for your thorough review. I explained my decisions a bit more and it would be great to see what you think about it or what you can suggest.

sauloperez · 2019-02-07T12:47:17Z

spec/services/bulk_invoice_service_spec.rb

+      Delayed::Job.last.invoke_job
+
+      expect(service.invoice_created?(service.id)).to be_truthy
+    end


IMO creating a job an depending on Delayed Job is what makes this an integration test, other than the order but you know... we're too coupled to AR.

To make it unit I would use a test double rather than an actual CombinePDF object and assert that we call #save on it passing the file name, since that's its public API. We can do that with an instance_double.

Then, I'd move to the controller test the responsibility to check that a background job is enqueued using DelayedJobHelper.

Let me know if you want to pair on it or if I should provide more details.

sauloperez · 2019-02-07T12:57:50Z

app/views/spree/admin/orders/_invoice_table2.html.haml

@@ -17,6 +17,9 @@
      %tr
        %td
          = render 'spree/shared/line_item_name', line_item: item
+          %br
+          %small
+            %em= raw(item.variant.product.supplier.name)


I see... what triggered my brain's attention was the violation of the Law of Demeter that item.variant.product.supplier.name causes. Don't you think?

The only solution that comes to my mind is to encapsulate it in LineItem#supplier_name. This at least decouples line item from all this knowledge. So it'd look something like raw(item.supplier_name).

EDIT https://www.youtube.com/watch?v=l9JXH7JPjR4is a fun explanation of the Law of Demeter 😂 (starts at 2:47)

sauloperez · 2019-02-07T13:54:08Z

I'm quite aware that my review might be tiresome but I'd gladly help you if need it.

sstead · 2019-02-15T05:09:19Z

Testing notes

Hooray, a printed invoice now includes the producer name :)
This is true regardless of the Invoice Settings (superadmin)

https://docs.google.com/document/d/1TwqaHrk6q6Ad49luLkoGC6LpCenTiXaijBsa4v8L8N8/edit?usp=sharing

mkllnk added 4 commits February 6, 2019 11:26

Test execution of BulkInvoiceService#start_pdf_job

7c5b430

The spec just tested if a job was enqueued, but not if the job can actually be executed. Unfortunately, this test is quite slow.

Remove code duplication and test PDF creation

d97fa60

Cover alternative invoice template with specs

3aea16e

Print supplier name on invoice

4a3e5f1

mkllnk self-assigned this Feb 6, 2019

luisramos0 approved these changes Feb 6, 2019

View reviewed changes

app/controllers/spree/admin/orders_controller_decorator.rb Outdated Show resolved Hide resolved

sauloperez suggested changes Feb 6, 2019

View reviewed changes

luisramos0 and others added 2 commits February 7, 2019 09:19

Update app/controllers/spree/admin/orders_controller_decorator.rb

2d0df7f

Co-Authored-By: mkllnk <[email protected]>

Re-use renderer object

5ee3dbf

sauloperez suggested changes Feb 7, 2019

View reviewed changes

Simplify spec of invoice creation

612ea4c

sauloperez approved these changes Feb 14, 2019

View reviewed changes

RachL added the pr-staged-au staging.openfoodnetwork.org.au label Feb 14, 2019

mkllnk merged commit d4589cc into openfoodfoundation:master Feb 15, 2019

mkllnk deleted the 3437-supplier-name-on-invoice branch February 15, 2019 05:32

luisramos0 removed the pr-staged-au staging.openfoodnetwork.org.au label Sep 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3437 supplier name on invoice #3444

3437 supplier name on invoice #3444

mkllnk commented Feb 6, 2019

luisramos0 commented Feb 6, 2019

luisramos0 left a comment

sauloperez Feb 6, 2019

mkllnk Feb 6, 2019

sauloperez Feb 6, 2019

sauloperez Feb 6, 2019

mkllnk Feb 6, 2019

sauloperez Feb 7, 2019

mkllnk Feb 14, 2019

sauloperez Feb 6, 2019

mkllnk Feb 6, 2019

sauloperez Feb 13, 2019

sauloperez Feb 6, 2019

mkllnk Feb 6, 2019

sauloperez Feb 7, 2019

mkllnk Feb 14, 2019

sauloperez Feb 14, 2019 •

edited

Loading

mkllnk Feb 15, 2019

sauloperez Feb 6, 2019

mkllnk Feb 6, 2019

sauloperez Feb 7, 2019 •

edited

Loading

mkllnk commented Feb 6, 2019

sauloperez Feb 7, 2019

sauloperez Feb 7, 2019

sauloperez commented Feb 7, 2019

sstead commented Feb 15, 2019

3437 supplier name on invoice #3444

3437 supplier name on invoice #3444

Conversation

mkllnk commented Feb 6, 2019

What? Why?

What should we test?

Release notes

luisramos0 commented Feb 6, 2019

luisramos0 left a 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

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

sauloperez Feb 14, 2019 • 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

sauloperez Feb 7, 2019 • edited Loading

Choose a reason for hiding this comment

mkllnk commented Feb 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sauloperez commented Feb 7, 2019

sstead commented Feb 15, 2019

sauloperez Feb 14, 2019 •

edited

Loading

sauloperez Feb 7, 2019 •

edited

Loading