Skip to content
This repository has been archived by the owner on Jul 24, 2020. It is now read-only.

[1499] Clean up #1337 #1545

Merged
merged 1 commit into from
May 3, 2016
Merged

[1499] Clean up #1337 #1545

merged 1 commit into from
May 3, 2016

Conversation

esoterik
Copy link
Collaborator

Resolves #1499

@orenyk
Copy link
Contributor

orenyk commented Apr 19, 2016

What about updating the links on the other equipment pages not to be hitting /categories.zip, but rather the relevant resource?

@esoterik
Copy link
Collaborator Author

It looks like I preemptively pushed! This is ready for review; I also added single category export.

@@ -48,7 +48,6 @@
<% else %>
<%= link_to "New Equipment Item", new_equipment_item_path, class: 'btn btn-primary' %>
<%= link_to "Import Equipment Item", equip_import_page_path, class: 'btn btn-default' %>
<%= link_to "Export Equipment Data", categories_path(format: 'zip'), :class => "btn btn-default" %>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we don't want to include the export link on the equipment items index? Any reason why not?

@orenyk
Copy link
Contributor

orenyk commented Apr 25, 2016

Let's verify the case where the description (or some other field) contains a comma and make sure it doesn't break everything.

@@ -70,4 +70,20 @@ def download_equipment_data
download_zip([categories, models, items],
"EquipmentData_#{Time.zone.now.to_s(:number)}")
end

# NOTE: this method depends on ActionController
def download_category_data(cat)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this basically does the same thing as download_equipment_data except it restricts the export to a single category? I feel like this is a lot of duplication in that case.

@orenyk
Copy link
Contributor

orenyk commented Apr 25, 2016

Another note: the export button does not appear on the scoped equipment model index view (i.e. /categories/:id/equipment_models), but it does appear on the general index view (i.e. /equipment_models). We should be consistent (EDIT and make sure we take this into account with the equipment items index as well).

@orenyk
Copy link
Contributor

orenyk commented Apr 25, 2016

Reviewed, a bunch of things to look into, let me know what you think!

@esoterik
Copy link
Collaborator Author

Somewhat surprisingly, it's okay to have fields with commas

@esoterik
Copy link
Collaborator Author

Okay, ready for rereview

<%= link_to "View All Equipment Models", equipment_models_path, :class => 'btn btn-default' %>
<%= link_to "View All Equipment Models", equipment_models_path, class: 'btn btn-default' %>
<%= link_to "Import Equipment Models", equip_import_page_path, class: "btn btn-default" %>
<%= link_to "Export Equipment Data", equipment_models_path(format: 'zip'), class: "btn btn-default" %>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make this the category_path(@category, format: 'zip') instead since it's a scoped view and you built scoped export?

@orenyk
Copy link
Contributor

orenyk commented Apr 26, 2016

A few small comments left, almost there!

@esoterik
Copy link
Collaborator Author

Okay this is probably ready!

@orenyk
Copy link
Contributor

orenyk commented Apr 26, 2016

Looks good! Can you squash / rebase onto the latest master so we can merge?

Resolves #1499
  - Include CsvExport in all relevant controllers
  - Add single category export
@esoterik
Copy link
Collaborator Author

This is ready to go!

@orenyk
Copy link
Contributor

orenyk commented May 3, 2016

Merging! @esoterik please add this to the CHANGELOG for v6.1.0 (#1548), thanks!

@orenyk orenyk merged commit 9556f2f into master May 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants