patricklindsay / wice_grid

A Rails grid plugin to create grids with sorting, pagination, and (automatically generated) filters
http://wicegrid.herokuapp.com/
MIT License
33 stars 29 forks source link

Use rails built in send_file method #16

Closed the-undefined closed 5 years ago

the-undefined commented 6 years ago

In Rails 5.1.3 the csv export was not outputting the table contents, it was trying to render the layout file but in a CSV format:

screen shot 2018-05-23 at 14 09 58

The proposed fix is to use the Rails send_file method over send_file_rails2 which is defined in wice grid itself (and called by export_grid_if_requested).

I'm not too sure what it would take to get the send_file_rails2 method updated to work alongside the rails one: https://github.com/rails/rails/blob/375a4143cf5caeb6159b338be824903edfd62836/actionpack/lib/action_controller/metal/data_streaming.rb#L67 or what specifically was breaking / missing from the send_file_rails2 if we do need to keep it around.

This change resulted in the CSV export function working as expected. It will be interesting to see if the tests are still passing with the send_file method once the testbed has been ported over. If we can adopt the rails method then a good chunk of this controller code (lib/wice/wice_grid_controller.rb) could be deleted.

patricklindsay commented 6 years ago

Thanks for the contribution @the-undefined

The test suite has now been ported over so should be able to check that out. I don't want to merge things until we have a passing build.

the-undefined commented 6 years ago

I have found that the CSV spec needs fleshing out a bit:

# spec/features/csv_export_request_spec.rb

# encoding: utf-8
require 'acceptance_helper'

describe 'CSV export WiceGrid', type: :request, js: true do
  before :each do
    visit '/csv_export'
  end

  it 'should export csv' do
    # no idea how to test these

    # find(:css, 'button.wg-external-csv-export-button').click

    select 'Urgent', from: 'g1_f_priority_id'

    # find(:css, 'button.div.clickable.export-to-csv-button').click
  end
end

So I'll update the specs for CSV export as part of this PR.

JasonBarnabe commented 6 years ago

send_file_rails2 is fixed (with tests added) in #4, but removing as suggested here may be the better solution.