Extends bulk print invoice spec to assert on file contents #11836
Conversation
filipefurtad0
added
the
technical changes only
spec/system/admin/orders_spec.rb
Outdated
| def extract_pdf_content | ||
| pdf_href = page.all('a').pluck('href') | ||
| page.find(class: "button", text: "VIEW FILE").click | ||
| invoice_file_number = pdf_href[0][45..59] | ||
| invoice_path = "tmp/invoices/#{invoice_file_number}.pdf" | ||
| reader = PDF::Reader.new(invoice_path) | ||
| @invoice_content = reader.pages.map(&:text) | ||
| end |
There was a problem hiding this comment.
This block should be within the test description instead of being a global method. Otherwise it will leak into other specs.
There was a problem hiding this comment.
This block should be within the test description
Ok, I've created a context for the test and moved the method it in there - I'm not 100% sure this is what you meant though?
spec/system/admin/orders_spec.rb
Outdated
| within ".modal-content" do | ||
| expect(page).to have_link(class: "button", text: "VIEW FILE", href: /invoices/) | ||
|
|
||
| extract_pdf_content # extracts content do variable invoice_content |
There was a problem hiding this comment.
It would be better to return the content from the method:
| extract_pdf_content # extracts content do variable invoice_content | |
| invoice_content = extract_pdf_content |
Then the test can store it however it wants and you don't have to know the name of the variable.
This way, we don't need to use it as a global method Clarifies context naming
filipefurtad0
force-pushed
the
check_pdf_file_bulk_invoice_print
branch
from
ae700b5
to
674974f
Compare
spec/system/admin/orders_spec.rb
Outdated
| invoice_file_number = pdf_href[0][45..59] | ||
| invoice_path = "tmp/invoices/#{invoice_file_number}.pdf" | ||
| reader = PDF::Reader.new(invoice_path) | ||
| invoice_content = reader.pages.map(&:text) |
There was a problem hiding this comment.
| invoice_content = reader.pages.map(&:text) | |
| reader.pages.map(&:text) |
spec/system/admin/orders_spec.rb
Outdated
|
|
||
| expect(page).to have_content "Bulk Invoice created" | ||
| expect(page).to have_link(class: "button", text: "VIEW FILE", href: /invoices/) | ||
| invoice_content = extract_pdf_content # extracts content do variable invoice_content |
There was a problem hiding this comment.
Now you don't need the comment any more.
| invoice_content = extract_pdf_content # extracts content do variable invoice_content | |
| invoice_content = extract_pdf_content |
| page.find("#listing_orders tbody tr:nth-child(1) input[name='bulk_ids[]']").click | ||
| page.find("#listing_orders tbody tr:nth-child(2) input[name='bulk_ids[]']").click | ||
| context "can bulk print invoices" do | ||
| def extract_pdf_content |
There was a problem hiding this comment.
I think with Maikel's change, this method can safely live at the bottom of the file again. But it's a single-use method anyway so it seems ok to be here.
| pdf_href = page.all('a').pluck('href') | ||
| page.find(class: "button", text: "VIEW FILE").click | ||
| invoice_file_number = pdf_href[0][45..59] |
There was a problem hiding this comment.
It looks like these lines are referring to the same link, but in a different way. In fact, I think the second line (page.find..click) isn't necessary: we're viewing the temporary file directly so don't need to trigger a download.
What? Why?
Checks for the contents of bulk generated pdf files. This came up here. Thank you for bringing this up @abdellani 🙏
Rather than converting the what the browser displays to text (as invoice_print_spec.rb does) it:
What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates