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

3437 supplier name on invoice #3444

Merged
merged 7 commits into from
Feb 15, 2019
Merged

3437 supplier name on invoice #3444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7c5b430
Test execution of BulkInvoiceService#start_pdf_job
mkllnk Feb 6, 2019
d97fa60
Remove code duplication and test PDF creation
mkllnk Feb 6, 2019
3aea16e
Cover alternative invoice template with specs
mkllnk Feb 6, 2019
4a3e5f1
Print supplier name on invoice
mkllnk Feb 6, 2019
2d0df7f
Update app/controllers/spree/admin/orders_controller_decorator.rb
luisramos0 Feb 6, 2019
5ee3dbf
Re-use renderer object
mkllnk Feb 6, 2019
612ea4c
Simplify spec of invoice creation
mkllnk Feb 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions app/controllers/spree/admin/orders_controller_decorator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,7 @@ def resend
end

def invoice
pdf = render_to_string pdf: "invoice-#{@order.number}.pdf",
template: invoice_template,
formats: [:html], encoding: "UTF-8"
pdf = InvoiceRenderer.new.render_to_string(@order)

Spree::OrderMailer.invoice_email(@order.id, pdf).deliver
flash[:success] = t('admin.orders.invoice_email_sent')
Expand All @@ -48,7 +46,7 @@ def invoice
end

def print
render pdf: "invoice-#{@order.number}", template: invoice_template, encoding: "UTF-8"
render InvoiceRenderer.new.args(@order)
end

def print_ticket
Expand All @@ -61,10 +59,6 @@ def update_distribution_charge

private

def invoice_template
Spree::Config.invoice_style2? ? "spree/admin/orders/invoice2" : "spree/admin/orders/invoice"
end

def require_distributor_abn
unless @order.distributor.abn.present?
flash[:error] = t(:must_have_valid_business_number, enterprise_name: @order.distributor.name)
Expand Down
12 changes: 2 additions & 10 deletions app/services/bulk_invoice_service.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
class BulkInvoiceService
include WickedPdf::PdfHelper
attr_reader :id

def initialize
Expand All @@ -11,10 +10,7 @@ def start_pdf_job(order_ids)
orders = Spree::Order.where(id: order_ids)

orders.each do |order|
invoice = renderer.render_to_string pdf: "invoice-#{order.number}.pdf",
template: invoice_template,
formats: [:html], encoding: "UTF-8",
locals: { :@order => order }
invoice = renderer.render_to_string(order)

pdf << CombinePDF.parse(invoice)
end
Expand Down Expand Up @@ -42,11 +38,7 @@ def directory
end

def renderer
ApplicationController.new
end

def invoice_template
Spree::Config.invoice_style2? ? "spree/admin/orders/invoice2" : "spree/admin/orders/invoice"
@renderer ||= InvoiceRenderer.new
end

def file_directory
Expand Down
29 changes: 29 additions & 0 deletions app/services/invoice_renderer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
class InvoiceRenderer
def render_to_string(order)
renderer.render_to_string(args(order))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end

def args(order)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

{
pdf: "invoice-#{order.number}.pdf",
template: invoice_template,
formats: [:html],
encoding: "UTF-8",
locals: { :@order => order }
}
end

private

def renderer
ApplicationController.new
end

def invoice_template
if Spree::Config.invoice_style2?
"spree/admin/orders/invoice2"
else
"spree/admin/orders/invoice"
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice abstraction 👏

3 changes: 3 additions & 0 deletions app/views/spree/admin/orders/_invoice_table.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
%tr
%td
= render 'spree/shared/line_item_name', line_item: item
%br
%small
%em= raw(item.variant.product.supplier.name)
%td{:align => "right"}
= item.quantity
%td{:align => "right"}
Expand Down
3 changes: 3 additions & 0 deletions app/views/spree/admin/orders/_invoice_table2.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
%tr
%td
= render 'spree/shared/line_item_name', line_item: item
%br
%small
%em= raw(item.variant.product.supplier.name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

%td{:align => "right"}
= item.quantity
%td{:align => "right"}
Expand Down
10 changes: 10 additions & 0 deletions spec/services/bulk_invoice_service_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,16 @@

expect(Delayed::Job.last.payload_object.method_name).to eq :start_pdf_job_without_delay
end

it "creates a PDF invoice" do
order = create(:completed_order_with_fees)
order.bill_address = order.ship_address
order.save!

service.start_pdf_job_without_delay([order.id])

expect(service.invoice_created?(service.id)).to be_truthy
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@sauloperez sauloperez Feb 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :trollface:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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. 👍

end

describe "#invoice_created?" do
Expand Down
20 changes: 20 additions & 0 deletions spec/services/invoice_renderer_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
require 'spec_helper'

describe InvoiceRenderer do
let(:service) { described_class.new }

it "creates a PDF invoice with two different templates" do
order = create(:completed_order_with_fees)
order.bill_address = order.ship_address
order.save!

result = service.render_to_string(order)
expect(result).to match /^%PDF/

allow(Spree::Config).to receive(:invoice_style2?).and_return true

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

  1. it renders an invoice pdf
  2. it renders an alternative invoice pdf
  3. it renders two different pdfs

Copy link
Contributor

@sauloperez sauloperez Feb 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

:trollface:

  • 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.

end