[1337] Add user and equipment CSV export #1340

esoterik · 2015-11-03T23:18:50Z

Resolves #1337

adds the module CsvExport to handle exporting CSV files
also supports exporting multiple CSVs at once in a zip file

orenyk · 2015-11-08T05:47:27Z

app/views/equipment_models/index.html.erb

@@ -9,7 +9,8 @@
    <%= link_to "View All Equipment Models", equipment_models_path, :class => 'btn btn-default' %>
  <% else %>
    <%= link_to "New Equipment Model", new_equipment_model_path, :class => "btn btn-primary" %>
-  <%= link_to "Import Equipment Models", equip_import_page_path, :class => "btn btn-default" %>
+    <%= link_to "Import Equipment Models", equip_import_page_path, :class => "btn btn-default" %>
+    <%= link_to "Export Equipment Data", categories_path(format: 'zip'), :class => "btn btn-default" %>


Should we add this link to the equipment items index as well?

orenyk · 2015-11-08T06:24:18Z

This looks really good! I want to take another look at the module when it's not this late at night but overall this seems to really solve the issue. I was able to export equipment and (after tweaking the names to avoid duplicates) import new data based on the exported files. The same was true for user export/import.

A few notes:

While the idea of exporting a .zip archive with all three models is great behavior, we should allow users to select which equipment types they want to export. We could either make this a JS popup on the pages where they can hit the export button, or have all the export buttons take them to a page where they can select which data to export (the columns for each model being predetermined) and then we generate a .zip archive with whatever data they wanted (or simply return a single csv if they only want one model). Basically, I'm suggesting a data export portal vs having a route for each resource.
The naming of the files is different - equipment files are named with camel case (e.g. EquipmentModel... whereas the user model is not capitalized and I'm guessing would be snake case if necessary. We should just be consistent (I prefer snake case, personally, but either is fine).
Finally, I think we should use a simple numeric timestamp including the time to avoid file duplication issues (as unlikely as they will be). It would just be a matter of calling Time.zone.now.to_s(:number) instead.

Again, great work! Why don't you start working on the above and I'll re-review the module tomorrow to see if anything other than what I already commented on jumps out at me. Thanks!

orenyk · 2015-11-08T22:07:31Z

lib/csv_export.rb

+    columns = data.size == 1 ? objects.first.attributes.keys : data[1]
+
+    # we never need to export ids
+    columns.delete('id')


This is good, any thoughts on also removing any password / authentication stuff if it's there? We could set up an array of columns to delete and simply call something like PROTECTED_COLS.each { |col| columns.delete(col) }. I don't love it since that array is coupled to the schema of our users table, but there's no specific harm in calling Hash#delete with a key that doesn't exist. Thoughts?

This sounds good

orenyk · 2015-11-08T22:10:08Z

Ok, second read-through complete, ready for updates!

orenyk · 2016-01-31T05:45:44Z

Replied to some of your comments, not sure if you're done (rubocop error / any other changes) so we can do a complete review once you give the go-ahead.

orenyk · 2016-02-02T03:37:40Z

Looks like we have some merge conflicts following tonight's PR mergings - can you rebase onto master?

esoterik · 2016-02-05T21:16:09Z

done!

orenyk · 2016-02-07T06:07:35Z

Gemfile

@@ -28,6 +28,9 @@ gem 'net-ldap', '~> 0.13.0'
 # attachments
 gem 'paperclip', '~> 4.3.2'

+# for exporting multiple files
+gem 'rubyzip'


Can you specify a version here?

Also, make sure to use the latest version, thanks 😄

orenyk · 2016-02-07T06:08:44Z

Don't have time for full review, will get to it tomorrow.

orenyk · 2016-02-09T19:37:22Z

lib/csv_export.rb

+  end
+
+  # downloads a csv of the given model table
+  # expects table to be an array with the following format:


I believe that these comments are outdated / unnecessary.

orenyk · 2016-02-09T20:08:44Z

A few small comments but this is really close. Let me know what you think about the ActionController dependency thing and once we've wrapped up the other stuff we can merge this in. Nice work!

Resolves #1337 - adds the module CsvExport to handle exporting CSV files - also supports exporting multiple CSVs at once in a zip file

[1337] Add user and equipment CSV export

esoterik added the 4 - Needs Review label Nov 3, 2015

orenyk changed the title ~~Add user and equipment CSV export~~ [1337] Add user and equipment CSV export Nov 4, 2015

orenyk reviewed Nov 8, 2015
View reviewed changes

orenyk added 1 - working and removed 4 - Needs Review labels Nov 8, 2015

orenyk reviewed Nov 8, 2015
View reviewed changes

esoterik force-pushed the 1337_csv_export branch 2 times, most recently from 95a5c5f to 95d6c09 Compare January 31, 2016 03:10

esoterik mentioned this pull request Feb 2, 2016

Refactor import functionality #899

Open

esoterik force-pushed the 1337_csv_export branch from bed420a to c67b782 Compare February 5, 2016 20:33

esoterik added 4 - Needs Review and removed 1 - working labels Feb 5, 2016

orenyk reviewed Feb 7, 2016
View reviewed changes

orenyk reviewed Feb 9, 2016
View reviewed changes

orenyk added 1 - working and removed 4 - Needs Review labels Feb 9, 2016

esoterik force-pushed the 1337_csv_export branch from c67b782 to 2978340 Compare February 11, 2016 23:31

Add user and equipment CSV export

38b0948

Resolves #1337 - adds the module CsvExport to handle exporting CSV files - also supports exporting multiple CSVs at once in a zip file

esoterik force-pushed the 1337_csv_export branch from 2978340 to 38b0948 Compare February 11, 2016 23:35

orenyk added a commit that referenced this pull request Feb 11, 2016

Merge pull request #1340 from YaleSTC/1337_csv_export

ff807f5

[1337] Add user and equipment CSV export

orenyk merged commit ff807f5 into master Feb 11, 2016

orenyk removed the 1 - working label Feb 11, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1337] Add user and equipment CSV export #1340

[1337] Add user and equipment CSV export #1340

esoterik commented Nov 3, 2015

orenyk Nov 8, 2015

esoterik Jan 31, 2016

orenyk commented Nov 8, 2015

orenyk Nov 8, 2015

esoterik Jan 31, 2016

orenyk commented Nov 8, 2015

orenyk commented Jan 31, 2016

orenyk commented Feb 2, 2016

esoterik commented Feb 5, 2016

orenyk Feb 7, 2016

orenyk Feb 9, 2016

orenyk commented Feb 7, 2016

orenyk Feb 9, 2016

orenyk commented Feb 9, 2016

[1337] Add user and equipment CSV export #1340

[1337] Add user and equipment CSV export #1340

Conversation

esoterik commented Nov 3, 2015

orenyk Nov 8, 2015

Choose a reason for hiding this comment

esoterik Jan 31, 2016

Choose a reason for hiding this comment

orenyk commented Nov 8, 2015

orenyk Nov 8, 2015

Choose a reason for hiding this comment

esoterik Jan 31, 2016

Choose a reason for hiding this comment

orenyk commented Nov 8, 2015

orenyk commented Jan 31, 2016

orenyk commented Feb 2, 2016

esoterik commented Feb 5, 2016

orenyk Feb 7, 2016

Choose a reason for hiding this comment

orenyk Feb 9, 2016

Choose a reason for hiding this comment

orenyk commented Feb 7, 2016

orenyk Feb 9, 2016

Choose a reason for hiding this comment

orenyk commented Feb 9, 2016