caxlsx / caxlsx_rails

A Rails plugin to provide templates for the axlsx gem
MIT License
744 stars 84 forks source link

Fix broken templates with comments as their last line #163

Closed kiskoza closed 1 year ago

kiskoza commented 1 year ago

Close #162

I found it very hard to run the tests locally, so I skipped writing one for this fix, but I verified the code by modifying an existing one - let me know if we need to add a spec covering the changes and I'll try again codifying it

straydogstudio commented 1 year ago

You could add a view that has a comment line at the end with no new line. Then write a controller action (or reuse one - I think there is one that changes views based on a parameter). Then write a test that calls that action/view.

These tests got written a long time ago. There is probably a better way to do tests against several versions of rails. You have to call the test scripts (e.g. ./spec/test_6.0.sh), and they probably only work on Mac or linux, but might in Windows WSL.

Do you want to try to write that test?

straydogstudio commented 1 year ago

You would add the view and action code in the /spec/dummy* folders.

straydogstudio commented 1 year ago

I will get to this eventually if you don't. I am simply insanely busy.

kiskoza commented 1 year ago

Thanks, I'll try to take a look in the next few days

kiskoza commented 1 year ago

I refactored a the template handler tests and added a new one that is failing on master, but got fixed with my code change.

Unfortunately there's a flaky check on the CI:

Edit: the flakyness goes away if I add this change:

diff --git a/spec/axlsx_request_spec.rb b/spec/axlsx_request_spec.rb
index e3578d4..8f6334c 100644
--- a/spec/axlsx_request_spec.rb
+++ b/spec/axlsx_request_spec.rb
@@ -165,7 +165,7 @@ describe 'Caxlsx request', :type => :request do

     it "mime all with render :xlsx and then :html" do
       # puts_def_formats 'before'
-      ActionView::Base.default_formats.delete :xlsx # see notes
+      # ActionView::Base.default_formats.delete :xlsx # see notes
       # puts_def_formats 'in my project'
       Capybara.current_driver = :mime_all

       visit '/another'
@@ -173,7 +173,7 @@ describe 'Caxlsx request', :type => :request do
       expect{
         visit '/home/only_html'
       }.to_not raise_error
-      ActionView::Base.default_formats.push :xlsx # see notes
+      # ActionView::Base.default_formats.push :xlsx # see notes

       # Output:
       # default formats before                        : [:html, :text, :js, :css, :ics, :csv, :vcf, :png, :jpeg, :gif, :bmp, :tiff, :mpeg, :xml, :rs
s, :atom, :yaml, :multipart_form, :url_encoded_form, :json, :pdf, :zip, :xlsx]
straydogstudio commented 1 year ago

Interesting. That test was created for Rails 4.2, as they were changing the mime handling for urls. Before I had forced the MIME format even if the url didn't match. That stopped working. I expect we can leave this out if all the tests are working. Thoughts?

kiskoza commented 1 year ago

I moved that test to run only on Rails 4.2 - we can keep it while we're supporting rails 4.x

straydogstudio commented 1 year ago

Sounds good. Thanks again for the work on this. Much appreciated.