-
-
Notifications
You must be signed in to change notification settings - Fork 719
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.
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
Extends bulk print invoice spec to assert on file contents #11836
Extends bulk print invoice spec to assert on file contents #11836
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting idea. This seems a lot more efficient than downloading the file. The downside is that it's very specific to invoices and how we store them. But that's okay for now.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
ae700b5
to
674974f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. That's what I meant.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting approach, well done !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I reviewed this and had a draft improvement to add, but forgot to indicate I was working on it!
I'll add in a new PR.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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