From f3ae5267d8b38f6c1ba3ab4e4ba2a0b4f0313e48 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Wed, 7 Jun 2017 12:13:10 +0200 Subject: Add some comments to help me understand `#table_builder` These are personal comments, just notes to myself as I was reading the code and trying to understand it. Will be removing these comments at a later time, once I've created my alternate `table_builder`. Refs #3479 --- app/helpers/newapplication_helper.rb | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/app/helpers/newapplication_helper.rb b/app/helpers/newapplication_helper.rb index 3d43e9fc7..8198e0efb 100644 --- a/app/helpers/newapplication_helper.rb +++ b/app/helpers/newapplication_helper.rb @@ -1,6 +1,7 @@ module NewapplicationHelper # Table Builder + # selectable means actions-for-selection def table_builder collection, columns, actions, selectable = [], cls = nil return unless collection.present? @@ -8,6 +9,7 @@ module NewapplicationHelper content_tag :tr do hcont = [] + # Adds checkbox to table header unless selectable.empty? cbx = content_tag :div, '', class: 'checkbox' do check_box_tag('0', 'all').concat(content_tag(:label, '', for: '0')) @@ -16,12 +18,14 @@ module NewapplicationHelper end columns.map do |k, v| + # These columns are hard-coded to not be sortable if ["ID Codif", "Oid", "OiD", "ID Reflex", "Arrêt de départ", "Arrêt d'arrivée", "Période de validité englobante", "Période englobante", "Nombre de courses associées", "Journées d'application", "Arrêts de l'itinéraire", "Arrêts inclus dans l'ITL"].include? k hcont << content_tag(:th, k) else hcont << content_tag(:th, sortable_columns(collection, k)) end end + # Inserts a blank column for the gear menu hcont << content_tag(:th, '') if actions.any? hcont.join.html_safe @@ -34,6 +38,8 @@ module NewapplicationHelper content_tag :tr do bcont = [] + # Adds item checkboxes whose value = the row object's id + # Apparently the object id is also used in the HTML id attribute without any prefix unless selectable.empty? cbx = content_tag :div, '', class: 'checkbox' do check_box_tag(item.try(:id), item.try(:id)).concat(content_tag(:label, '', for: item.try(:id))) @@ -48,9 +54,11 @@ module NewapplicationHelper else item.try(attribute) end + # if so this column's contents get transformed into a link to the object if attribute == 'name' or attribute == 'comment' lnk = [] + # #is_a? ? ; or ? unless item.class == Calendar or item.class == Referential if current_referential lnk << current_referential @@ -172,6 +180,7 @@ module NewapplicationHelper pic2 = content_tag :span, '', class: "fa fa-sort-desc #{(direction == 'asc') ? 'active' : ''}" pics = content_tag :span, pic1 + pic2, class: 'orderers' + # This snake cases and downcases the class name. Should use the ActiveSupport method to do this obj = collection.model.to_s.gsub('Chouette::', '').scan(/[A-Z][a-z]+/).join('_').downcase (I18n.t("activerecord.attributes.#{obj}.#{key}") + pics).html_safe -- cgit v1.2.3 From 073ac0eb3330787532f551d20abe4600a0615446 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Wed, 7 Jun 2017 13:12:56 +0200 Subject: factories/workbenches: Add a new `:with_referential` trait This new trait allows us to create a referential association along with the workbench. Works like this: create(:workbench, :with_referential) We do this to allow us to reuse the `organisation`, `line_referential`, and `stop_area_referential` associations to ensure that the values of those fields are consistent in both the resulting workbench and referential objects. If we need to create multiple referentials, we could add an `amount` later. Left in some commented code describing my earlier attempts at this. Fortunately, found this Stack Overflow post that pointed me in the right direction: https://stackoverflow.com/questions/20685322/how-to-create-an-association-in-factorygirl-with-a-value-lazy-evaluated-from-ano I need this for a new `spec/helpers/table_builder_helper_spec.rb` spec, wherein I'm trying to test building a table of workspaces, but having trouble with code I copied from the app that tries to get `#validity_period` from workbenches. Thought at first that adding a referential (which does have this field) could help get past that, but it didn't quite work. Needs more investigation. Refs #3479 --- spec/factories/workbenches.rb | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/spec/factories/workbenches.rb b/spec/factories/workbenches.rb index f51e7d94c..154482213 100644 --- a/spec/factories/workbenches.rb +++ b/spec/factories/workbenches.rb @@ -5,5 +5,24 @@ FactoryGirl.define do association :organisation, :factory => :organisation association :line_referential association :stop_area_referential + + trait :with_referential do + # TODO: change all => to : + # association :referential, + # organisation: { organisation } + + # after(:stub) do |workbench, evaluator| + # + # end + + referentials do |workbench| + [association( + :referential, + organisation: workbench.organisation, + line_referential: workbench.line_referential, + stop_area_referential: workbench.stop_area_referential + )] + end + end end end -- cgit v1.2.3 From 212d1f740b17ae89b69603a3f1964351a38b86f6 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 8 Jun 2017 11:03:20 +0200 Subject: Add a few TODOs related to the table builder work Just a few notes to myself for things to work on in this story. Refs #3479 --- app/helpers/newapplication_helper.rb | 3 +++ app/views/referential_lines/show.html.slim | 1 + spec/factories/chouette_2_factories.rb | 1 + 3 files changed, 5 insertions(+) diff --git a/app/helpers/newapplication_helper.rb b/app/helpers/newapplication_helper.rb index 8198e0efb..27b9cb0a4 100644 --- a/app/helpers/newapplication_helper.rb +++ b/app/helpers/newapplication_helper.rb @@ -59,6 +59,9 @@ module NewapplicationHelper lnk = [] # #is_a? ? ; or ? + # TODO: Ask Jean-Paul: on which pages do we create multiple links? + # Do we actually create multiple links with this code? + # Answer: this is a polymorphic URL unless item.class == Calendar or item.class == Referential if current_referential lnk << current_referential diff --git a/app/views/referential_lines/show.html.slim b/app/views/referential_lines/show.html.slim index db99381d3..5fb1bb125 100644 --- a/app/views/referential_lines/show.html.slim +++ b/app/views/referential_lines/show.html.slim @@ -7,6 +7,7 @@ / Below is secundary actions & optional contents .row .col-lg-12.text-right.mb-sm + / TODO: Make a list of Link objects that can be rendered with link_tos here and in the gear menu = link_to @line.human_attribute_name(:footnotes), referential_line_footnotes_path(@referential, @line), class: 'btn btn-primary' = link_to t('routing_constraint_zones.index.title'), referential_line_routing_constraint_zones_path(@referential, @line), class: 'btn btn-primary' diff --git a/spec/factories/chouette_2_factories.rb b/spec/factories/chouette_2_factories.rb index e8eba13e6..a8e80702d 100644 --- a/spec/factories/chouette_2_factories.rb +++ b/spec/factories/chouette_2_factories.rb @@ -1,3 +1,4 @@ +# TODO: Move these factories into their own files so all factory definitions are consistent FactoryGirl.define do factory :organisation do -- cgit v1.2.3 From c3d752af1ed040f64cd7cbfb964c2c5d5d8f7cde Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 8 Jun 2017 11:36:03 +0200 Subject: Add in-progress `TableBuilderHelper` (WIP) Porting `table_builder` code over from `NewapplicationHelper`. The code is very much in progress and incomplete. Only decided to commit it now because I keep making changes and want to have those tracked so I don't lose anything. A lot of the original is commented out and the initial test I wrote doesn't pass. The test also doesn't use the proper data, I just copied it from the website's output. Anyway, this is sort of the starting point. Refs #3479 --- app/helpers/table_builder_helper.rb | 107 ++++++++++++++++++++++++++ spec/helpers/table_builder_helper_spec.rb | 120 ++++++++++++++++++++++++++++++ 2 files changed, 227 insertions(+) create mode 100644 app/helpers/table_builder_helper.rb create mode 100644 spec/helpers/table_builder_helper_spec.rb diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb new file mode 100644 index 000000000..91eef5a47 --- /dev/null +++ b/app/helpers/table_builder_helper.rb @@ -0,0 +1,107 @@ +module TableBuilderHelper + # TODO: rename this after migration from `table_builder` + def table_builder_2( + collection, + columns, + sortable: true, + selectable: false, # TODO: is this necessary? + # selection_actions: [] ## this has been gotten rid of. The element based on this should be created elsewhere + links: [], # links: or actions: ? I think 'links' is better since 'actions' evokes Rails controller actions and we want to put `link_to`s here + sort_by: {}, # { column: 'name', direction: 'desc' } + cls: '' # can we rename this to "class"? +# sort column +# sort direction + +# TODO: add `linked_column` or some such attribute that defines which column should be linked and what method to call to get it + ) + # TODO: Maybe move this to a private method + head = content_tag :thead do + content_tag :tr do + hcont = [] + + # Adds checkbox to table header + if !selectable + cbx = content_tag :div, '', class: 'checkbox' do + check_box_tag('0', 'all').concat(content_tag(:label, '', for: '0')) + end + hcont << content_tag(:th, cbx) + end + + columns.map do |k, v| + # These columns are hard-coded to not be sortable + if ["ID Codif", "Oid", "OiD", "ID Reflex", "Arrêt de départ", "Arrêt d'arrivée", "Période de validité englobante", "Période englobante", "Nombre de courses associées", "Journées d'application"].include? k + hcont << content_tag(:th, k) + else + # FIXME: error undefined `current_referential` + # hcont << content_tag(:th, sortable_columns(collection, k)) + # temporarily replaced with: + hcont << content_tag(:th, k) + end + end + # Inserts a blank column for the gear menu + hcont << content_tag(:th, '') if links.any? + + hcont.join.html_safe + end + end + + # TODO: refactor + body = content_tag :tbody do + collection.collect do |item| + + content_tag :tr do + bcont = [] + + # Adds item checkboxes whose value = the row object's id + # Apparently the object id is also used in the HTML id attribute without any prefix + if !selectable + # TODO: Extract method `build_checkbox(attribute)` + cbx = content_tag :div, '', class: 'checkbox' do + check_box_tag(item.try(:id), item.try(:id)).concat(content_tag(:label, '', for: item.try(:id))) + end + bcont << content_tag(:td, cbx) + end + + columns.map do |k, attribute| + value = + if Proc === attribute + attribute.call(item) + else + item.try(attribute) + end + # if so this column's contents get transformed into a link to the object + if attribute == 'name' or attribute == 'comment' + lnk = [] + + # #is_a? ? ; or ? + # unless item.class == Calendar or item.class == Referential + # if current_referential + # lnk << current_referential + # lnk << item.line if item.respond_to? :line + # lnk << item.route.line if item.class == Chouette::RoutingConstraintZone + # lnk << item if item.respond_to? :line_referential + # lnk << item.stop_area if item.respond_to? :stop_area + # lnk << item if item.respond_to? :stop_points or item.class.to_s == 'Chouette::TimeTable' + # elsif item.respond_to? :referential + # lnk << item.referential + # end + # else + # lnk << item + # end + # + # bcont << content_tag(:td, link_to(value, lnk), title: 'Voir') + else + bcont << content_tag(:td, value) + end + end + # TODO: error undefined `current_referential` + # bcont << content_tag(:td, links_builder(item, links), class: 'actions') if links.any? + + bcont.join.html_safe + end + end.join.html_safe + end + + content_tag :table, head + body, class: cls + end +end diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb new file mode 100644 index 000000000..e9d1afed3 --- /dev/null +++ b/spec/helpers/table_builder_helper_spec.rb @@ -0,0 +1,120 @@ +require 'spec_helper' + +describe TableBuilderHelper, type: :helper do + describe "#table_builder_2" do + it "builds a table" do + workbenches = [ + build_stubbed(:referential) + ] + + expected = <<-HTML + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + HTML +#
+# 0 élément(s) sélectionné(s) +#
+ + minified_expected = expected.gsub(/^\s+/, '').gsub("\n", '') + + expect(helper.table_builder_2( + workbenches, + { :name => 'name', + :status => Proc.new {|w| w.archived? ? ("
Conservé
").html_safe : ("
En préparation
").html_safe}, + :status => Proc.new {|w| ("
En préparation
").html_safe}, + :organisation => Proc.new {|w| w.organisation.name}, + :validity_period => Proc.new {|w| w.validity_period.nil? ? '-' : t('validity_range', debut: l(w.try(:validity_period).try(:begin), format: :short), end: l(w.try(:validity_period).try(:end), format: :short))}, + :lines => Proc.new {|w| w.lines.count}, + :created_at => Proc.new {|w| l(w.created_at, format: :short)}, + :updated_at => Proc.new {|w| l(w.updated_at, format: :short)}, + :published_at => ''}, + links: [:show, :edit, :archive, :unarchive, :delete], + cls: 'table has-filter has-search' + )).to eq(minified_expected) + end + end +end + + +# Replace table builder on workspaces#show page +# Make the builder work without a `current_referential` so we can actually test it +# Make a way to define a column as non-sortable. By default, columns will be sortable. Unless sortable==false and no columns should be sortable. +# +# +# TODO: +# - Finish writing workbench test +# - Port some code over to the new table builder +# - Ask Jean-Paul if there's anything he wishes could be changed or improved about the existing table builder +# - Thing that Jean-Paul didn't like was the link generation -- cgit v1.2.3 From 6acb866c9e1d60147d8869eb151178eb0c11bda6 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 8 Jun 2017 14:26:26 +0200 Subject: TableBuilder: Re-enable code dependent on `current_referential` Working on getting this to work and be testable. Haven't figured out how I want to deal with `current_referential` yet to make it easy to test. Refs #3479 --- app/helpers/table_builder_helper.rb | 40 +++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 91eef5a47..7b8ada9a2 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -32,10 +32,7 @@ module TableBuilderHelper if ["ID Codif", "Oid", "OiD", "ID Reflex", "Arrêt de départ", "Arrêt d'arrivée", "Période de validité englobante", "Période englobante", "Nombre de courses associées", "Journées d'application"].include? k hcont << content_tag(:th, k) else - # FIXME: error undefined `current_referential` - # hcont << content_tag(:th, sortable_columns(collection, k)) - # temporarily replaced with: - hcont << content_tag(:th, k) + hcont << content_tag(:th, sortable_columns(collection, k)) end end # Inserts a blank column for the gear menu @@ -74,28 +71,27 @@ module TableBuilderHelper lnk = [] # #is_a? ? ; or ? - # unless item.class == Calendar or item.class == Referential - # if current_referential - # lnk << current_referential - # lnk << item.line if item.respond_to? :line - # lnk << item.route.line if item.class == Chouette::RoutingConstraintZone - # lnk << item if item.respond_to? :line_referential - # lnk << item.stop_area if item.respond_to? :stop_area - # lnk << item if item.respond_to? :stop_points or item.class.to_s == 'Chouette::TimeTable' - # elsif item.respond_to? :referential - # lnk << item.referential - # end - # else - # lnk << item - # end - # - # bcont << content_tag(:td, link_to(value, lnk), title: 'Voir') + unless item.class == Calendar or item.class == Referential + if current_referential + lnk << current_referential + lnk << item.line if item.respond_to? :line + lnk << item.route.line if item.class == Chouette::RoutingConstraintZone + lnk << item if item.respond_to? :line_referential + lnk << item.stop_area if item.respond_to? :stop_area + lnk << item if item.respond_to? :stop_points or item.class.to_s == 'Chouette::TimeTable' + elsif item.respond_to? :referential + lnk << item.referential + end + else + lnk << item + end + + bcont << content_tag(:td, link_to(value, lnk), title: 'Voir') else bcont << content_tag(:td, value) end end - # TODO: error undefined `current_referential` - # bcont << content_tag(:td, links_builder(item, links), class: 'actions') if links.any? + bcont << content_tag(:td, links_builder(item, links), class: 'actions') if links.any? bcont.join.html_safe end -- cgit v1.2.3 From 71611d2d4ffd5a1d6569acd0cae703ed22c55ed5 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 8 Jun 2017 14:29:11 +0200 Subject: TableBuilder: Port `links_builder` and `sortable_columns` from old code Bring these two methods from `NewapplicationHelper` into `TableBuilderHelper` so we can mess with them in the context of our helper. We'll want to try to change & improve them a bit. Also need to make sure we rename them, otherwise they'll conflict with the existing helper methods. Added a few notes to myself in the comments. Refs #3479 --- app/helpers/table_builder_helper.rb | 100 ++++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 7b8ada9a2..54f6833e7 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -100,4 +100,104 @@ module TableBuilderHelper content_tag :table, head + body, class: cls end + + private + + # TODO: `def build_link[s]` + def links_builder(item, actions) + trigger = content_tag :div, class: 'btn dropdown-toggle', data: { toggle: 'dropdown' } do + content_tag :span, '', class: 'fa fa-cog' + end + + menu = content_tag :ul, class: 'dropdown-menu' do + actions.collect do |action| + polymorph_url = [] + + unless [:show, :delete].include? action + polymorph_url << action + end + + unless item.class == Calendar or item.class == Referential + if current_referential + polymorph_url << current_referential + polymorph_url << item.line if item.respond_to? :line + polymorph_url << item.route.line if item.class == Chouette::RoutingConstraintZone + polymorph_url << item if item.respond_to? :line_referential + polymorph_url << item.stop_area if item.respond_to? :stop_area + polymorph_url << item if item.respond_to? :stop_points or item.class.to_s == 'Chouette::TimeTable' + elsif item.respond_to? :referential + polymorph_url << item.referential + end + else + polymorph_url << item + end + + if action == :delete + if policy(item).present? + if policy(item).destroy? + content_tag :li, '', class: 'delete-action' do + link_to(polymorph_url, method: :delete, data: { confirm: 'Etes-vous sûr(e) de vouloir effectuer cette action ?' }) do + txt = t("actions.#{action}") + pic = content_tag :span, '', class: 'fa fa-trash' + pic + txt + end + end + end + else + content_tag :li, '', class: 'delete-action' do + link_to(polymorph_url, method: :delete, data: { confirm: 'Etes-vous sûr(e) de vouloir effectuer cette action ?' }) do + txt = t("actions.#{action}") + pic = content_tag :span, '', class: 'fa fa-trash' + pic + txt + end + end + end + + elsif action == :edit + if policy(item).present? + if policy(item).update? + content_tag :li, link_to(t("actions.#{action}"), polymorph_url) + end + else + content_tag :li, link_to(t("actions.#{action}"), polymorph_url) + end + elsif action == :archive + unless item.archived? + content_tag :li, link_to(t("actions.#{action}"), polymorph_url, method: :put) + end + elsif action == :unarchive + if item.archived? + content_tag :li, link_to(t("actions.#{action}"), polymorph_url, method: :put) + end + else + content_tag :li, link_to(t("actions.#{action}"), polymorph_url) + end + end.join.html_safe + end + + content_tag :div, trigger + menu, class: 'btn-group' + + end + + # TODO: clean up? + def sortable_columns collection, key + # #, #]> + # (byebug) collection.model + # Referential(id: integer, name: string, slug: string, created_at: datetime, updated_at: datetime, prefix: string, projection_type: string, time_zone: string, bounds: string, organisation_id: integer, geographical_bounds: text, user_id: integer, user_name: string, data_format: string, line_referential_id: integer, stop_area_referential_id: integer, workbench_id: integer, archived_at: datetime, created_from_id: integer, ready: boolean) + # params = {"controller"=>"workbenches", "action"=>"show", "id"=>"1", "q"=>{"archived_at_not_null"=>"1", "archived_at_null"=>"1"}} + direction = (key.to_s == params[:sort] && params[:direction] == 'desc') ? 'asc' : 'desc' + + link_to(params.merge({direction: direction, sort: key})) do + pic1 = content_tag :span, '', class: "fa fa-sort-asc #{(direction == 'desc') ? 'active' : ''}" + pic2 = content_tag :span, '', class: "fa fa-sort-desc #{(direction == 'asc') ? 'active' : ''}" + + pics = content_tag :span, pic1 + pic2, class: 'orderers' + # This snake cases and downcases the class name. Should use the ActiveSupport method to do this + # TODO: Maybe give this whole thing a name so it's clearer what's going on. Also, figure out a way to maybe explicitise the dynamicness of getting the model type from the `collection`. + # TODO: move these two lines to a new method called `column_header_label` and rename `pics` to something like `icons` or arrow icons or some such + obj = collection.model.to_s.gsub('Chouette::', '').scan(/[A-Z][a-z]+/).join('_').downcase + + (I18n.t("activerecord.attributes.#{obj}.#{key}") + pics).html_safe + end + end end -- cgit v1.2.3 From 1ea3187de06cb159faf0f773c50263dd69eafaa8 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 8 Jun 2017 14:31:51 +0200 Subject: TableBuilder: Ideas for method arguments A couple of ideas for new method arguments. Didn't like `cls:`, but not a fan of avoiding the name just for looks if the alternative is messy. Don't like the fact that `current_referential` comes in as a global to a couple parts of the builder. Ideally want to be explicit about that it is and where it comes from. Unfortunately, it's rather convoluted and dynamic, designed to abstract away the difference between different models' referentials. And it's connected to `parent` (very dynamic) via the `inherited_resources` gem, which means that refactoring it out would create a huge mess of changes. Not anxious to board that ship just yet. Do we want to pass `current_referential` in directly and just make the tests easier to write? Would it be a pain to pass it in every time from the caller? I don't know yet. Still trying to figure out the best course of action there. Refs #3479 --- app/helpers/table_builder_helper.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 54f6833e7..161ffb17c 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -3,12 +3,14 @@ module TableBuilderHelper def table_builder_2( collection, columns, + current_referential: nil, sortable: true, selectable: false, # TODO: is this necessary? # selection_actions: [] ## this has been gotten rid of. The element based on this should be created elsewhere links: [], # links: or actions: ? I think 'links' is better since 'actions' evokes Rails controller actions and we want to put `link_to`s here sort_by: {}, # { column: 'name', direction: 'desc' } cls: '' # can we rename this to "class"? + # ^^ rename to html_options = {} at the end of the non-keyword arguments? Hrm, not a fan of combining hash args and keyword args # sort column # sort direction -- cgit v1.2.3 From ebd01ff27d3a4b2a57566632ce37873de4acf810 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 8 Jun 2017 14:37:44 +0200 Subject: TableBuilder spec: Stub a `params` global Since the `TableBuilder` depends on the controller's `params` hash (the link builder needs the controller, action, etc. in order to build a link), we need to stub `params` in order for the helper call to work in our test. Still not a fan of this implicit dependency, but we'll see. I might keep it in because maybe it makes more sense and would require less change to keep it the way it is. The link builder is highly dynamic and seems like it would be tricky to make non-dynamic. Needs more thought. Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index e9d1afed3..085ed5a81 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -7,6 +7,12 @@ describe TableBuilderHelper, type: :helper do build_stubbed(:referential) ] + allow(helper).to receive(:params).and_return({ + :controller => 'workbenches', + :action => 'show', + :id => workbenches[0].workbench.id + }) + expected = <<-HTML -- cgit v1.2.3 From fa04c7ecc9dff0a06ad5a9c793a7b17defcf4300 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 8 Jun 2017 14:40:26 +0200 Subject: TableBuilder spec: Add a couple TODOs It's not actually a collection of `workbenches` as we originally thought, it's a collection of referentials. The name of the variable should be changed to reflect that. Also, in the real code, this value is not an array, but an `ActiveRecord::Relation`, thus the `sortable_columns` method is able to call `.model` on it. Since the test uses an array, that breaks. Need to figure out what we want to do there. I don't like the dynamic-ness of the way this is done now, but we need to figure out if it's possible to factor that out in a pleasant way. Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index 085ed5a81..f936e9303 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe TableBuilderHelper, type: :helper do describe "#table_builder_2" do it "builds a table" do + # TODO: Rename to `referentials` + # TODO: `sortable_columns` calls `#model` on this collection workbenches = [ build_stubbed(:referential) ] -- cgit v1.2.3 From c4a5af98e48a51ebbca4e1eaad96da44887e8757 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 8 Jun 2017 15:01:27 +0200 Subject: TableBuilder spec: Rename `workbenches` to `referentials` Because the collection contains referentials, not workbenches. Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index f936e9303..5ca23ad09 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -3,16 +3,15 @@ require 'spec_helper' describe TableBuilderHelper, type: :helper do describe "#table_builder_2" do it "builds a table" do - # TODO: Rename to `referentials` # TODO: `sortable_columns` calls `#model` on this collection - workbenches = [ + referentials = [ build_stubbed(:referential) ] allow(helper).to receive(:params).and_return({ :controller => 'workbenches', :action => 'show', - :id => workbenches[0].workbench.id + :id => referentials[0].workbench.id }) expected = <<-HTML @@ -98,7 +97,7 @@ describe TableBuilderHelper, type: :helper do minified_expected = expected.gsub(/^\s+/, '').gsub("\n", '') expect(helper.table_builder_2( - workbenches, + referentials, { :name => 'name', :status => Proc.new {|w| w.archived? ? ("
Conservé
").html_safe : ("
En préparation
").html_safe}, :status => Proc.new {|w| ("
En préparation
").html_safe}, -- cgit v1.2.3 From 5166f0b6030b79d47b02743656eff835b523cae1 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 8 Jun 2017 15:05:59 +0200 Subject: TableBuilder spec: Stub `#model` on collection `referentials` (known as `collection` in the `TableBuilder`) is supposed to be an `ActiveRecord::Relation` instance. The `#sortable_columns` method calls `collection.model` in order to dynamically build a table column header. In order to get the test to the next stage, stub the `#model` method. Not a huge fan. Ideally, the code wouldn't be so dynamic, but maybe I'm wrong about that. Still need to think about this for the refactoring. Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index 5ca23ad09..f3db493ce 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -3,11 +3,12 @@ require 'spec_helper' describe TableBuilderHelper, type: :helper do describe "#table_builder_2" do it "builds a table" do - # TODO: `sortable_columns` calls `#model` on this collection referentials = [ build_stubbed(:referential) ] + allow(referentials).to receive(:model).and_return(Referential) + allow(helper).to receive(:params).and_return({ :controller => 'workbenches', :action => 'show', -- cgit v1.2.3 From 01417ebaeec1150be5ce976c70322ef4f91e22af Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 8 Jun 2017 15:37:49 +0200 Subject: workbenches/show.html.slim: Temporarily change to `table_builder_2` Use this page as a visual testing ground for the new table builder. Currently just verifying that nothing obvious changes visually. Eventually we'll convert this page for real, but the API for the updated table builder isn't finalised yet. Refs #3479 --- app/views/workbenches/show.html.slim | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/app/views/workbenches/show.html.slim b/app/views/workbenches/show.html.slim index 77e670923..65fb4652b 100644 --- a/app/views/workbenches/show.html.slim +++ b/app/views/workbenches/show.html.slim @@ -22,7 +22,7 @@ - if @wbench_refs.any? .row .col-lg-12 - = table_builder @wbench_refs, + /= table_builder @wbench_refs, { :name => 'name', :status => Proc.new {|w| w.archived? ? ("
Conservé
").html_safe : ("
En préparation
").html_safe}, :organisation => Proc.new {|w| w.organisation.name}, @@ -34,6 +34,19 @@ [:show, :edit, :archive, :unarchive, :delete], [:delete], 'table has-filter has-search' + / TODO: change => to : + = table_builder_2 @wbench_refs, + { :name => 'name', + :status => Proc.new {|w| w.archived? ? ("
Conservé
").html_safe : ("
En préparation
").html_safe}, + :organisation => Proc.new {|w| w.organisation.name}, + :validity_period => Proc.new {|w| w.validity_period.nil? ? '-' : t('validity_range', debut: l(w.try(:validity_period).try(:begin), format: :short), end: l(w.try(:validity_period).try(:end), format: :short))}, + :lines => Proc.new {|w| w.lines.count}, + :created_at => Proc.new {|w| l(w.created_at, format: :short)}, + :updated_at => Proc.new {|w| l(w.updated_at, format: :short)}, + :published_at => ''}, + links: [:show, :edit, :archive, :unarchive, :delete], + cls: 'table has-filter has-search' + / [:delete], = new_pagination @wbench_refs, 'pull-right' -- cgit v1.2.3 From fcaf1c7e79c1f2c9e5f95fd032e363ffc84b35e1 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 8 Jun 2017 15:41:49 +0200 Subject: TableBuilder: Extract column header code to a new method Give this bit of code a better name. Update relevant TODOs. Refs #3479 --- app/helpers/table_builder_helper.rb | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 161ffb17c..ee79fb4d1 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -195,11 +195,15 @@ module TableBuilderHelper pics = content_tag :span, pic1 + pic2, class: 'orderers' # This snake cases and downcases the class name. Should use the ActiveSupport method to do this - # TODO: Maybe give this whole thing a name so it's clearer what's going on. Also, figure out a way to maybe explicitise the dynamicness of getting the model type from the `collection`. - # TODO: move these two lines to a new method called `column_header_label` and rename `pics` to something like `icons` or arrow icons or some such - obj = collection.model.to_s.gsub('Chouette::', '').scan(/[A-Z][a-z]+/).join('_').downcase + # TODO: figure out a way to maybe explicitise the dynamicness of getting the model type from the `collection`. + # TODO: rename `pics` to something like `icons` or arrow icons or some such - (I18n.t("activerecord.attributes.#{obj}.#{key}") + pics).html_safe + (column_header_label(collection.model, key) + pics).html_safe end end + + def column_header_label(model, field) + model_underscored = model.to_s.gsub('Chouette::', '').scan(/[A-Z][a-z]+/).join('_').downcase + I18n.t("activerecord.attributes.#{model_underscored}.#{field}") + end end -- cgit v1.2.3 From a2fc8b0253819fa5585a04cf9c8232a689184b29 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 8 Jun 2017 15:52:02 +0200 Subject: TableBuilder: Simplify #column_header_label Get rid of the old code that got an I18n key from the collection's model name. Instead of the longer text transformations from before, use `ActiveSupport::Inflector` methods to achieve the same result. Makes for a shorter line of code. Refs #3479 --- app/helpers/table_builder_helper.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index ee79fb4d1..2296af8c7 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -194,7 +194,6 @@ module TableBuilderHelper pic2 = content_tag :span, '', class: "fa fa-sort-desc #{(direction == 'asc') ? 'active' : ''}" pics = content_tag :span, pic1 + pic2, class: 'orderers' - # This snake cases and downcases the class name. Should use the ActiveSupport method to do this # TODO: figure out a way to maybe explicitise the dynamicness of getting the model type from the `collection`. # TODO: rename `pics` to something like `icons` or arrow icons or some such @@ -203,7 +202,9 @@ module TableBuilderHelper end def column_header_label(model, field) - model_underscored = model.to_s.gsub('Chouette::', '').scan(/[A-Z][a-z]+/).join('_').downcase - I18n.t("activerecord.attributes.#{model_underscored}.#{field}") + # Transform `Chouette::Line` into "line" + model_key = model.to_s.demodulize.underscore + + I18n.t("activerecord.attributes.#{model_key}.#{field}") end end -- cgit v1.2.3 From 5d9ed9d90ada31f9ee895111613d970473981096 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 8 Jun 2017 18:00:01 +0200 Subject: TableBuilder spec: Get past Pundit errors Add a bunch of mocks etc. to get past errors related to using Pundit inside the helper. These errors included: * No method `policy` (included `Pundit` in `TableBuilderHelper`) * No method `authenticate` (mocked a `current_user` method on the helper) * Need to return a `user_context` from `current_user` to make policy's `#new` happy (no method `user` on `User`) At first tried to use the `create_user_context` method from spec/support/pundit, but it wasn't in scope so I just did it directly to get it working. Don't like all the mocks at all. A lot of pre-work to do just to get the helper to execute without exceptions. Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index f3db493ce..b1c2b9a3f 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -1,11 +1,25 @@ require 'spec_helper' +module TableBuilderHelper + include Pundit +end + describe TableBuilderHelper, type: :helper do describe "#table_builder_2" do it "builds a table" do - referentials = [ - build_stubbed(:referential) - ] + referential = build_stubbed(:referential) + + # user_context = create_user_context( + # user: build_stubbed(:user), + # referential: referential + # ) + user_context = OpenStruct.new( + user: build_stubbed(:user), + context: { referential: referential } + ) + allow(helper).to receive(:current_user).and_return(user_context) + + referentials = [referential] allow(referentials).to receive(:model).and_return(Referential) -- cgit v1.2.3 From 7319d9d1324db3da7da4ed441594aa5d05c1feec Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 8 Jun 2017 18:13:12 +0200 Subject: TableBuilder spec: Beautify actual HTML result In order to allow us to easily compare the HTML result with the expected value, beautify the HTML that comes out of the helper call. Otherwise, all the HTML is on a single line, and it can't be compared with RSpec's diff output (even if RSpec outputted the entire contents of the single line, we wouldn't really be able to compare it). Thanks to this post for describing an easy way to get HTML beautification: https://stackoverflow.com/questions/5622269/how-to-beautify-xml-code-in-rails-application We need to run the expected HTML string through the beautifier so we can get the formatting right, but otherwise this gives us a better base for test diffing. Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index b1c2b9a3f..5945b0f82 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -109,9 +109,7 @@ describe TableBuilderHelper, type: :helper do # 0 élément(s) sélectionné(s) # - minified_expected = expected.gsub(/^\s+/, '').gsub("\n", '') - - expect(helper.table_builder_2( + html_str = helper.table_builder_2( referentials, { :name => 'name', :status => Proc.new {|w| w.archived? ? ("
Conservé
").html_safe : ("
En préparation
").html_safe}, @@ -124,7 +122,11 @@ describe TableBuilderHelper, type: :helper do :published_at => ''}, links: [:show, :edit, :archive, :unarchive, :delete], cls: 'table has-filter has-search' - )).to eq(minified_expected) + ) + beautified_html = '' + REXML::Document.new(html_str).write(beautified_html, 4) + + expect(beautified_html).to eq(expected) end end end -- cgit v1.2.3 From 3b745cf43b877a4bb26532d16b602717779cf15c Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 8 Jun 2017 18:29:38 +0200 Subject: Gemfile: Add 'htmlbeautifier' in :test group Want a way to beautify HTML to get better diffs in the `TableBuilderHelper` spec. Beautifying the HTML allows us to do line comparisons of the HTML that results from the builder. Refs #3479 --- Gemfile | 1 + Gemfile.lock | 2 ++ 2 files changed, 3 insertions(+) diff --git a/Gemfile b/Gemfile index 55e88f78a..6520f73de 100644 --- a/Gemfile +++ b/Gemfile @@ -159,6 +159,7 @@ group :test do gem 'cucumber-rails', require: false gem 'simplecov', :require => false gem 'simplecov-rcov', :require => false + gem 'htmlbeautifier' end group :test, :development, :dev do diff --git a/Gemfile.lock b/Gemfile.lock index 992e44af0..f0b7630c6 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -261,6 +261,7 @@ GEM hashdiff (0.3.2) highline (1.7.8) hike (1.2.3) + htmlbeautifier (1.3.1) httparty (0.14.0) multi_xml (>= 0.5.2) i18n (0.8.1) @@ -603,6 +604,7 @@ DEPENDENCIES georuby-ext (= 0.0.5) google-analytics-rails has_array_of! + htmlbeautifier i18n-tasks inherited_resources jbuilder (~> 2.0) -- cgit v1.2.3 From 89a81c459b60391603f38cb1a477e883d3bb2403 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 8 Jun 2017 18:33:03 +0200 Subject: TableBuilder spec: Use 'htmlbeautifier' gem for beautifying Instead of `REXML::Document`, use `htmlbeautifier` to beautify our actual HTML result. The trouble with `REXML` was that it forced single quotes around attribute values. With 'htmlbeautifier', we get a good result that we can realistically use for line-by-line diffs between expected and actual. Much easier see what went wrong this way. And, in my current case, see what I need to update update in the expected string based on the factory-generated objects that I'm using. Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index 5945b0f82..c94df36a5 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -1,4 +1,5 @@ require 'spec_helper' +require 'htmlbeautifier' module TableBuilderHelper include Pundit @@ -123,8 +124,8 @@ describe TableBuilderHelper, type: :helper do links: [:show, :edit, :archive, :unarchive, :delete], cls: 'table has-filter has-search' ) - beautified_html = '' - REXML::Document.new(html_str).write(beautified_html, 4) + + beautified_html = HtmlBeautifier.beautify(html_str, indent: ' ') expect(beautified_html).to eq(expected) end -- cgit v1.2.3 From 9dab28fef1ac66b02a1ef62fdfb86cfc5ff13b96 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Fri, 9 Jun 2017 11:26:51 +0200 Subject: TableBuilder spec: Change params hash to new Ruby hash syntax I had copied this from a debug session outputting a controller's `params` hash. The original was: {"controller"=>"workbenches", "action"=>"show", "id"=>"1", "q"=>{"archived_at_not_null"=>"1", "archived_at_null"=>"1"}} which I just copy-pasted into my test. Since new code should be using the new syntax, change this to use colons instead of arrows. Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index c94df36a5..1049bf996 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -25,9 +25,9 @@ describe TableBuilderHelper, type: :helper do allow(referentials).to receive(:model).and_return(Referential) allow(helper).to receive(:params).and_return({ - :controller => 'workbenches', - :action => 'show', - :id => referentials[0].workbench.id + controller: 'workbenches', + action: 'show', + id: referentials[0].workbench.id }) expected = <<-HTML -- cgit v1.2.3 From 5d3cab81d61e5fb809a07f0e5fdaf00aa2f3210a Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Fri, 9 Jun 2017 11:31:35 +0200 Subject: TableBuilder spec: Correct expected column header hrefs Use the correct workbench ID in the expected links for column sorting. Previously I had copied this from rendered HTML on my local dev server. We should actually use the ID of the factory-generated workbench object. Now this part of our tests passes, correctly matching the URLs. Needed to copy over the `q` params that are created by the `WorkbenchesController` in order for this to actually work with the right query string parameters tacked on to the hrefs. Makes the test a bit more complicated, but more closely matches the real page's behaviour. Hrm, I wonder if we should get rid of that param entirely and modify the expected hrefs to removed them since that doesn't really appear to be related to what we're actually testing. I'll consider it. Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index 1049bf996..6f3ca7ee3 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -9,6 +9,7 @@ describe TableBuilderHelper, type: :helper do describe "#table_builder_2" do it "builds a table" do referential = build_stubbed(:referential) + workbench = referential.workbench # user_context = create_user_context( # user: build_stubbed(:user), @@ -27,7 +28,13 @@ describe TableBuilderHelper, type: :helper do allow(helper).to receive(:params).and_return({ controller: 'workbenches', action: 'show', - id: referentials[0].workbench.id + id: referentials[0].workbench.id, + + # These are added by WorkbenchesController#query_params + q: { + archived_at_not_null: 1, + archived_at_null: 1 + } }) expected = <<-HTML @@ -37,14 +44,14 @@ describe TableBuilderHelper, type: :helper do - - - - - - - - + + + + + + + + -- cgit v1.2.3 From bba80d09a1b429092114dff81a229e24a6720d87 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Fri, 9 Jun 2017 11:55:21 +0200 Subject: TableBuilder spec: Remove second table row from expected Since we only have one referential in our collection, remove the second row because it shouldn't be in the actual output. Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index 6f3ca7ee3..4f4cffe61 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -82,32 +82,6 @@ describe TableBuilderHelper, type: :helper do - - - - - - - - - - - - HTML -- cgit v1.2.3 From 4ce4f81ed51ec5215ecef431e6ed2cbbbef0952a Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Fri, 9 Jun 2017 11:57:26 +0200 Subject: TableBuilder spec: Get values dynamically from test referential Replace the hard-coded `Referential` fields with dynamic ones from the factory-generated referential we're using to test. This gets our expected output nearly matching the actual output. Pretty cool! Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index 4f4cffe61..a58905244 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -58,26 +58,26 @@ describe TableBuilderHelper, type: :helper do -
+
- Referential Yanis Gaillard + #{referential.name}
En préparation
- STIF - 01/05/2017 > 31/08/2017 - 1 - 02/05/2017 - 02/05/2017 + #{referential.organisation.name} + - + #{referential.lines.count} + #{I18n.localize(referential.created_at, format: :short)} + #{I18n.localize(referential.updated_at, format: :short)} -- cgit v1.2.3 From 652c63e3d5e9f842d88ea9bc3ae027712922f69a Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Fri, 9 Jun 2017 12:24:03 +0200 Subject: TableBuilder spec: Give test user all referential permissions Our expected output renders edit and destroy links in the cog menu: @@ -35,9 +35,7 @@ It does this because I copied it from HTML rendered from my local dev server, where my normal login user has admin permissions. The actual HTML output doesn't render those links because the factory-generated user for the test doesn't have permissions to those links. To get the actual to match the expected string, give our test user the proper permissions and ensure their organisation is the same as the test referential's organisation (as defined by the permissions in `ReferentialPolicy`). Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index a58905244..f3a6001d0 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -16,7 +16,15 @@ describe TableBuilderHelper, type: :helper do # referential: referential # ) user_context = OpenStruct.new( - user: build_stubbed(:user), + user: build_stubbed( + :user, + organisation: referential.organisation, + permissions: [ + 'referentials.create', + 'referentials.edit', + 'referentials.destroy' + ] + ), context: { referential: referential } ) allow(helper).to receive(:current_user).and_return(user_context) -- cgit v1.2.3 From 913cdad28b20ade05cccf262b096b02db2e0ab93 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Fri, 9 Jun 2017 12:33:32 +0200 Subject: TableBuilder spec: Remove newline from expected HTMl output The actual HTML output doesn't have a newline at the end. This isn't important for the test, so just remove the extra newline from the expected output that's caused by the heredoc terminator. Our test is finally green! Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index f3a6001d0..165e9c3f4 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -116,7 +116,7 @@ describe TableBuilderHelper, type: :helper do beautified_html = HtmlBeautifier.beautify(html_str, indent: ' ') - expect(beautified_html).to eq(expected) + expect(beautified_html).to eq(expected.chomp) end end end -- cgit v1.2.3 From 74e02b7666efda81344728c3f7a27039db96f830 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Fri, 9 Jun 2017 14:45:36 +0200 Subject: TableBuilderHelper: Move thead & tbody sections to their own methods Make new private methods for the and element builders. This just splits out the work in `table_builder_2` into smaller pieces, to begin to make them more manageable. Refs #3479 --- app/helpers/table_builder_helper.rb | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 2296af8c7..0a32e528c 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -16,8 +16,17 @@ module TableBuilderHelper # TODO: add `linked_column` or some such attribute that defines which column should be linked and what method to call to get it ) - # TODO: Maybe move this to a private method - head = content_tag :thead do + + content_tag :table, + thead(collection, columns, selectable, links) + + tbody(collection, columns, selectable, links), + class: cls + end + + private + + def thead(collection, columns, selectable, links) + content_tag :thead do content_tag :tr do hcont = [] @@ -43,9 +52,11 @@ module TableBuilderHelper hcont.join.html_safe end end + end + def tbody(collection, columns, selectable, links) # TODO: refactor - body = content_tag :tbody do + content_tag :tbody do collection.collect do |item| content_tag :tr do @@ -99,12 +110,8 @@ module TableBuilderHelper end end.join.html_safe end - - content_tag :table, head + body, class: cls end - private - # TODO: `def build_link[s]` def links_builder(item, actions) trigger = content_tag :div, class: 'btn dropdown-toggle', data: { toggle: 'dropdown' } do -- cgit v1.2.3 From ca10698f263dad9298b001e9194c4e042c1a9cff Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Fri, 9 Jun 2017 14:55:30 +0200 Subject: TableBuilderHelper: Change `or` to `||` For better style and logical operator precedence, use `||` instead of the `or` operator. Refs #3479 --- app/helpers/table_builder_helper.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 0a32e528c..01f356217 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -80,18 +80,18 @@ module TableBuilderHelper item.try(attribute) end # if so this column's contents get transformed into a link to the object - if attribute == 'name' or attribute == 'comment' + if attribute == 'name' || attribute == 'comment' lnk = [] # #is_a? ? ; or ? - unless item.class == Calendar or item.class == Referential + unless item.class == Calendar || item.class == Referential if current_referential lnk << current_referential lnk << item.line if item.respond_to? :line lnk << item.route.line if item.class == Chouette::RoutingConstraintZone lnk << item if item.respond_to? :line_referential lnk << item.stop_area if item.respond_to? :stop_area - lnk << item if item.respond_to? :stop_points or item.class.to_s == 'Chouette::TimeTable' + lnk << item if item.respond_to? :stop_points || item.class.to_s == 'Chouette::TimeTable' elsif item.respond_to? :referential lnk << item.referential end @@ -126,14 +126,14 @@ module TableBuilderHelper polymorph_url << action end - unless item.class == Calendar or item.class == Referential + unless item.class == Calendar || item.class == Referential if current_referential polymorph_url << current_referential polymorph_url << item.line if item.respond_to? :line polymorph_url << item.route.line if item.class == Chouette::RoutingConstraintZone polymorph_url << item if item.respond_to? :line_referential polymorph_url << item.stop_area if item.respond_to? :stop_area - polymorph_url << item if item.respond_to? :stop_points or item.class.to_s == 'Chouette::TimeTable' + polymorph_url << item if item.respond_to? :stop_points || item.class.to_s == 'Chouette::TimeTable' elsif item.respond_to? :referential polymorph_url << item.referential end -- cgit v1.2.3 From 0fb0bd13e4e09c910bc4159985b714f43f6d0fe0 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Fri, 9 Jun 2017 15:13:44 +0200 Subject: TableBuilderHelper: Correct `selectable` behaviour I had coded this the opposite of what it should have been. Instead, if `selectable` is true, we should show the selection checkboxes. Invert the conditions and add `selectable: true` to the `table_builder_2` calls that expect a checkbox column. Also remove the TODO because this argument is necessary. Refs #3479 --- app/helpers/table_builder_helper.rb | 6 +++--- app/views/workbenches/show.html.slim | 1 + spec/helpers/table_builder_helper_spec.rb | 1 + 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 01f356217..3e8602dac 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -5,7 +5,7 @@ module TableBuilderHelper columns, current_referential: nil, sortable: true, - selectable: false, # TODO: is this necessary? + selectable: false, # selection_actions: [] ## this has been gotten rid of. The element based on this should be created elsewhere links: [], # links: or actions: ? I think 'links' is better since 'actions' evokes Rails controller actions and we want to put `link_to`s here sort_by: {}, # { column: 'name', direction: 'desc' } @@ -31,7 +31,7 @@ module TableBuilderHelper hcont = [] # Adds checkbox to table header - if !selectable + if selectable cbx = content_tag :div, '', class: 'checkbox' do check_box_tag('0', 'all').concat(content_tag(:label, '', for: '0')) end @@ -64,7 +64,7 @@ module TableBuilderHelper # Adds item checkboxes whose value = the row object's id # Apparently the object id is also used in the HTML id attribute without any prefix - if !selectable + if selectable # TODO: Extract method `build_checkbox(attribute)` cbx = content_tag :div, '', class: 'checkbox' do check_box_tag(item.try(:id), item.try(:id)).concat(content_tag(:label, '', for: item.try(:id))) diff --git a/app/views/workbenches/show.html.slim b/app/views/workbenches/show.html.slim index 65fb4652b..61b9395fa 100644 --- a/app/views/workbenches/show.html.slim +++ b/app/views/workbenches/show.html.slim @@ -44,6 +44,7 @@ :created_at => Proc.new {|w| l(w.created_at, format: :short)}, :updated_at => Proc.new {|w| l(w.updated_at, format: :short)}, :published_at => ''}, + selectable: true, links: [:show, :edit, :archive, :unarchive, :delete], cls: 'table has-filter has-search' / [:delete], diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index 165e9c3f4..dd4db0840 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -110,6 +110,7 @@ describe TableBuilderHelper, type: :helper do :created_at => Proc.new {|w| l(w.created_at, format: :short)}, :updated_at => Proc.new {|w| l(w.updated_at, format: :short)}, :published_at => ''}, + selectable: true, links: [:show, :edit, :archive, :unarchive, :delete], cls: 'table has-filter has-search' ) -- cgit v1.2.3 From aba69469b1eb19d5347fb9cf1d7d28efe055d33f Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Fri, 9 Jun 2017 15:34:55 +0200 Subject: TableBuilderHelper: Extract checkboxes to a private method Build checkboxes using a new private method to reduce duplication and increase naming. Used the name `id_name` because the value gets used for both the `id` and `name` attributes of the checkbox input. Refs #3479 --- app/helpers/table_builder_helper.rb | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 3e8602dac..ef728a12d 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -32,10 +32,7 @@ module TableBuilderHelper # Adds checkbox to table header if selectable - cbx = content_tag :div, '', class: 'checkbox' do - check_box_tag('0', 'all').concat(content_tag(:label, '', for: '0')) - end - hcont << content_tag(:th, cbx) + hcont << content_tag(:th, checkbox(id_name: '0', value: 'all')) end columns.map do |k, v| @@ -62,14 +59,11 @@ module TableBuilderHelper content_tag :tr do bcont = [] - # Adds item checkboxes whose value = the row object's id - # Apparently the object id is also used in the HTML id attribute without any prefix if selectable - # TODO: Extract method `build_checkbox(attribute)` - cbx = content_tag :div, '', class: 'checkbox' do - check_box_tag(item.try(:id), item.try(:id)).concat(content_tag(:label, '', for: item.try(:id))) - end - bcont << content_tag(:td, cbx) + bcont << content_tag( + :td, + checkbox(id_name: item.try(:id), value: item.try(:id)) + ) end columns.map do |k, attribute| @@ -214,4 +208,12 @@ module TableBuilderHelper I18n.t("activerecord.attributes.#{model_key}.#{field}") end + + def checkbox(id_name:, value:) + content_tag :div, '', class: 'checkbox' do + check_box_tag(id_name, value).concat( + content_tag(:label, '', for: id_name) + ) + end + end end -- cgit v1.2.3 From 31805920c4ec9366d37a6b00d3dc8e48db72ea9f Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Fri, 9 Jun 2017 15:37:31 +0200 Subject: TableBuilderHelper#thead: Change `links` argument to `has_links` Since this method doesn't depend on the actual list of links, don't ask for it as an argument. Instead, just pass the answer to "are there any links?". Refs #3479 --- app/helpers/table_builder_helper.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index ef728a12d..95c84d50e 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -18,14 +18,14 @@ module TableBuilderHelper ) content_tag :table, - thead(collection, columns, selectable, links) + + thead(collection, columns, selectable, links.any?) + tbody(collection, columns, selectable, links), class: cls end private - def thead(collection, columns, selectable, links) + def thead(collection, columns, selectable, has_links) content_tag :thead do content_tag :tr do hcont = [] @@ -44,7 +44,7 @@ module TableBuilderHelper end end # Inserts a blank column for the gear menu - hcont << content_tag(:th, '') if links.any? + hcont << content_tag(:th, '') if has_links hcont.join.html_safe end -- cgit v1.2.3 From 8674d00185602c76b5231282ac87ea746ab1a2f2 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Fri, 9 Jun 2017 15:44:16 +0200 Subject: TableBuilderHelper: Use `#is_a?` instead of `==` for type checking Prefer `#is_a?` which is there specifically for type checking instead of `==` to compare types. Refs #3479 --- app/helpers/table_builder_helper.rb | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 95c84d50e..83c7a0689 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -77,15 +77,14 @@ module TableBuilderHelper if attribute == 'name' || attribute == 'comment' lnk = [] - # #is_a? ? ; or ? - unless item.class == Calendar || item.class == Referential + unless item.is_a?(Calendar) || item.is_a?(Referential) if current_referential lnk << current_referential lnk << item.line if item.respond_to? :line - lnk << item.route.line if item.class == Chouette::RoutingConstraintZone + lnk << item.route.line if item.is_a?(Chouette::RoutingConstraintZone) lnk << item if item.respond_to? :line_referential lnk << item.stop_area if item.respond_to? :stop_area - lnk << item if item.respond_to? :stop_points || item.class.to_s == 'Chouette::TimeTable' + lnk << item if item.respond_to? :stop_points || item.is_a?(Chouette::TimeTable) elsif item.respond_to? :referential lnk << item.referential end @@ -120,14 +119,14 @@ module TableBuilderHelper polymorph_url << action end - unless item.class == Calendar || item.class == Referential + unless item.is_a?(Calendar) || item.is_a?(Referential) if current_referential polymorph_url << current_referential polymorph_url << item.line if item.respond_to? :line - polymorph_url << item.route.line if item.class == Chouette::RoutingConstraintZone + polymorph_url << item.route.line if item.is_a?(Chouette::RoutingConstraintZone) polymorph_url << item if item.respond_to? :line_referential polymorph_url << item.stop_area if item.respond_to? :stop_area - polymorph_url << item if item.respond_to? :stop_points || item.class.to_s == 'Chouette::TimeTable' + polymorph_url << item if item.respond_to? :stop_points || item.is_a?(Chouette::TimeTable) elsif item.respond_to? :referential polymorph_url << item.referential end -- cgit v1.2.3 From 0b9e41d57fdba19bf3d9a61029e2fd688fbf61f2 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Fri, 9 Jun 2017 16:12:45 +0200 Subject: TableBuilderHelper: Extract polymorphic URL block to method Get rid of the duplication in these two sections by extracting the code to a method. Still not a huge fan of how it's set up, but this at least gives us an incremental improvement. Refs #3479 --- app/helpers/table_builder_helper.rb | 56 ++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 32 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 83c7a0689..586842d46 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -75,24 +75,8 @@ module TableBuilderHelper end # if so this column's contents get transformed into a link to the object if attribute == 'name' || attribute == 'comment' - lnk = [] - - unless item.is_a?(Calendar) || item.is_a?(Referential) - if current_referential - lnk << current_referential - lnk << item.line if item.respond_to? :line - lnk << item.route.line if item.is_a?(Chouette::RoutingConstraintZone) - lnk << item if item.respond_to? :line_referential - lnk << item.stop_area if item.respond_to? :stop_area - lnk << item if item.respond_to? :stop_points || item.is_a?(Chouette::TimeTable) - elsif item.respond_to? :referential - lnk << item.referential - end - else - lnk << item - end - - bcont << content_tag(:td, link_to(value, lnk), title: 'Voir') + polymorph_url = polymorphic_url_parts(item) + bcont << content_tag(:td, link_to(value, polymorph_url), title: 'Voir') else bcont << content_tag(:td, value) end @@ -119,20 +103,7 @@ module TableBuilderHelper polymorph_url << action end - unless item.is_a?(Calendar) || item.is_a?(Referential) - if current_referential - polymorph_url << current_referential - polymorph_url << item.line if item.respond_to? :line - polymorph_url << item.route.line if item.is_a?(Chouette::RoutingConstraintZone) - polymorph_url << item if item.respond_to? :line_referential - polymorph_url << item.stop_area if item.respond_to? :stop_area - polymorph_url << item if item.respond_to? :stop_points || item.is_a?(Chouette::TimeTable) - elsif item.respond_to? :referential - polymorph_url << item.referential - end - else - polymorph_url << item - end + polymorph_url += polymorphic_url_parts(item) if action == :delete if policy(item).present? @@ -215,4 +186,25 @@ module TableBuilderHelper ) end end + + def polymorphic_url_parts(item) + polymorph_url = [] + + unless item.is_a?(Calendar) || item.is_a?(Referential) + if current_referential + polymorph_url << current_referential + polymorph_url << item.line if item.respond_to? :line + polymorph_url << item.route.line if item.is_a?(Chouette::RoutingConstraintZone) + polymorph_url << item if item.respond_to? :line_referential + polymorph_url << item.stop_area if item.respond_to? :stop_area + polymorph_url << item if item.respond_to? :stop_points || item.is_a?(Chouette::TimeTable) + elsif item.respond_to? :referential + polymorph_url << item.referential + end + else + polymorph_url << item + end + + polymorph_url + end end -- cgit v1.2.3 From 53ede82ce2e8a1226fbb16ec67eb092cdcd12082 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 12 Jun 2017 11:18:21 +0200 Subject: TableBuilderHelper: Change `#collect` calls to `#map` Use `#map` here because the name makes more sense to me in this context. Also, the `#thead` method uses `columns.map`, so we can do the same thing in these two places for consistency. Refs #3479 --- app/helpers/table_builder_helper.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 586842d46..b3b5e3cde 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -54,7 +54,7 @@ module TableBuilderHelper def tbody(collection, columns, selectable, links) # TODO: refactor content_tag :tbody do - collection.collect do |item| + collection.map do |item| content_tag :tr do bcont = [] @@ -96,7 +96,7 @@ module TableBuilderHelper end menu = content_tag :ul, class: 'dropdown-menu' do - actions.collect do |action| + actions.map do |action| polymorph_url = [] unless [:show, :delete].include? action -- cgit v1.2.3 From f2129a26cdf9cf986832c8a780f1e35c225a1df9 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 12 Jun 2017 14:13:09 +0200 Subject: TableBuilder spec: Add a test for sortable & non-sortable columns Uses the table on the `/referentials/:id/companies` page. In that table, all columns are sortable except for the "ID Codif" column. The setup for the test is quite verbose and I don't like it. I'll be using the test to change the sortable logic, removing the hard-coded column header names checked inside the table builder in favour of a flag that gets passed as part of the arguments to `table_builder`. Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 83 +++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index dd4db0840..ab41a0936 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -119,6 +119,89 @@ describe TableBuilderHelper, type: :helper do expect(beautified_html).to eq(expected.chomp) end + + it "can set a column as non-sortable" do + company = build_stubbed(:company) + line_referential = build_stubbed( + :line_referential, + companies: [company] + ) + referential = build_stubbed( + :referential, + line_referential: line_referential + ) + + user_context = OpenStruct.new( + user: build_stubbed( + :user, + organisation: referential.organisation, + permissions: [ + 'referentials.create', + 'referentials.edit', + 'referentials.destroy' + ] + ), + context: { referential: referential } + ) + allow(helper).to receive(:current_user).and_return(user_context) + allow(helper).to receive(:current_referential).and_return(referential) + + companies = [company] + + allow(companies).to receive(:model).and_return(Chouette::Company) + + allow(helper).to receive(:params).and_return({ + controller: 'referential_companies', + action: 'index', + referential_id: referential.id + }) + + expected = <<-HTML + + + + + + + + + + + + + + + + + + + + + + + HTML + + html_str = helper.table_builder_2( + companies, + { + 'Oid' => Proc.new { |n| n.try(:objectid).try(:local_id) }, + :name => 'name' + }, + links: [:show, :edit, :delete], + cls: 'table has-search' + ) + + beautified_html = HtmlBeautifier.beautify(html_str, indent: ' ') + + expect(beautified_html).to eq(expected.chomp) + end end end -- cgit v1.2.3 From 91f6bc0b56d4b51645470c9f470d874f92d38b50 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 12 Jun 2017 14:23:58 +0200 Subject: TableBuilder spec: Fix columns in sortable test I had mistakenly copied the code from `app/views/companies/index.html.slim`. Instead, the code from the page I wanted was actually supposed to come from `app/views/referential_companies/index.html.slim`. Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index ab41a0936..e29112653 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -191,8 +191,11 @@ describe TableBuilderHelper, type: :helper do html_str = helper.table_builder_2( companies, { - 'Oid' => Proc.new { |n| n.try(:objectid).try(:local_id) }, - :name => 'name' + 'ID Codif' => Proc.new { |n| n.try(:objectid).try(:local_id) }, + :name => 'name', + :phone => 'phone', + :email => 'email', + :url => 'url' }, links: [:show, :edit, :delete], cls: 'table has-search' -- cgit v1.2.3 From e89aead78b6182a0789d53e46b230df54aee82f3 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 12 Jun 2017 14:34:46 +0200 Subject: TableBuilder: Support a column-level sortable flag Provides an option to set a column as non-sortable when defining the column in the `table_builder` call. Not written well, but it gets the idea across. We allow column hash values to be hashes themselves (while continuing to allow them to be `Proc`s and strings, this mixing of types is what I really don't like). In that column-level hash, we can pass `sortable: false` to disable sorting on that column. By default, sorting is turned on for all columns. Change the method signature of `sortable_columns`. Rename it to `build_column_header`, as it will now generate a correct column header regardless of whether the column is supposed to be sortable or not. I moved some dependencies outside of the method, like `collection` (since we only need the model from the collection), and the sort fields on `params`. Unfortunately wasn't able to completely extricate `params` for this method. Left the `params.merge` call that builds a link in this method. Ideally I'd like that removed so that the method no longer depends on global variables. In `build_column_header`, if the column is not sortable, we return the label defined by the key in the columns hash defined by the `table_builder` caller. Otherwise, we build some HTML that includes links on a couple of arrow buttons to enable sorting. In `#thead`, we check to see if the columns hash value has a `sortable` defined, in which case that flag gets used to determine what the column header will be. I renamed the block variables here to be more descriptive. Still not a huge fan of `attribute` as a name, though, because it can now also be a Hash. Another aspect of this change that I don't like. In `#tbody`, needed to add a few extra lines to check whether `attribute` is a Hash and get the real attribute if so. Really don't like that either because it doesn't look good. Update the sortable spec to pass a hash as the value of the column that should not be sorted. It defines a `:sortable` key that tells the builder not to allow this column to be sorted. Refs #3479 --- app/helpers/table_builder_helper.rb | 36 +++++++++++++++++++++++-------- spec/helpers/table_builder_helper_spec.rb | 5 ++++- 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index b3b5e3cde..954ec5314 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -35,13 +35,19 @@ module TableBuilderHelper hcont << content_tag(:th, checkbox(id_name: '0', value: 'all')) end - columns.map do |k, v| - # These columns are hard-coded to not be sortable - if ["ID Codif", "Oid", "OiD", "ID Reflex", "Arrêt de départ", "Arrêt d'arrivée", "Période de validité englobante", "Période englobante", "Nombre de courses associées", "Journées d'application"].include? k - hcont << content_tag(:th, k) - else - hcont << content_tag(:th, sortable_columns(collection, k)) + columns.map do |key, attribute| + sortable = true + if attribute.is_a?(Hash) && attribute.has_key?(:sortable) + sortable = attribute[:sortable] end + + hcont << content_tag(:th, build_column_header( + collection.model, + key, + sortable, + params[:sort], + params[:direction] + )) end # Inserts a blank column for the gear menu hcont << content_tag(:th, '') if has_links @@ -67,6 +73,10 @@ module TableBuilderHelper end columns.map do |k, attribute| + if attribute.is_a?(Hash) + attribute = attribute[:attribute] + end + value = if Proc === attribute attribute.call(item) @@ -153,12 +163,20 @@ module TableBuilderHelper end # TODO: clean up? - def sortable_columns collection, key + def build_column_header( + collection_model, + key, + sortable, + sort_on, + sort_direction + ) # #, #]> # (byebug) collection.model # Referential(id: integer, name: string, slug: string, created_at: datetime, updated_at: datetime, prefix: string, projection_type: string, time_zone: string, bounds: string, organisation_id: integer, geographical_bounds: text, user_id: integer, user_name: string, data_format: string, line_referential_id: integer, stop_area_referential_id: integer, workbench_id: integer, archived_at: datetime, created_from_id: integer, ready: boolean) # params = {"controller"=>"workbenches", "action"=>"show", "id"=>"1", "q"=>{"archived_at_not_null"=>"1", "archived_at_null"=>"1"}} - direction = (key.to_s == params[:sort] && params[:direction] == 'desc') ? 'asc' : 'desc' + return key if !sortable + + direction = (key.to_s == sort_on && sort_direction == 'desc') ? 'asc' : 'desc' link_to(params.merge({direction: direction, sort: key})) do pic1 = content_tag :span, '', class: "fa fa-sort-asc #{(direction == 'desc') ? 'active' : ''}" @@ -168,7 +186,7 @@ module TableBuilderHelper # TODO: figure out a way to maybe explicitise the dynamicness of getting the model type from the `collection`. # TODO: rename `pics` to something like `icons` or arrow icons or some such - (column_header_label(collection.model, key) + pics).html_safe + (column_header_label(collection_model, key) + pics).html_safe end end diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index e29112653..029748dd2 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -191,7 +191,10 @@ describe TableBuilderHelper, type: :helper do html_str = helper.table_builder_2( companies, { - 'ID Codif' => Proc.new { |n| n.try(:objectid).try(:local_id) }, + 'ID Codif' => { + attribute: Proc.new { |n| n.try(:objectid).try(:local_id) }, + sortable: false + }, :name => 'name', :phone => 'phone', :email => 'email', -- cgit v1.2.3 From 6fb1431b3a2d3649bbd56ebc527fb45803248c94 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 12 Jun 2017 15:25:58 +0200 Subject: TableBuilder: Make a new type for columns Instead of making the columns a hash where the types get all confused and mussed and we have to type check wherever we use the column value, make the columns into their own real type. Adds a new `Column` class to the helper module which lets us interact with table column data in a much more natural way. Here, when mapping over the column list, instead of getting a key and value which could be whatever, we get a real `Column` object that we can call methods on to get the appropriate data. It's still not perfect, as evidenced by the `#tbody` method, which has remained largely the same, but at least we're not checking `attribute.is_a?(Hash)` like I had tried before. Update the tests to use the new `TableBuilderHelper::Column` syntax for defining columns. Refs #3479 --- app/helpers/table_builder_helper.rb | 48 +++++++++++++-------- spec/helpers/table_builder_helper_spec.rb | 72 +++++++++++++++++++++++-------- 2 files changed, 86 insertions(+), 34 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 954ec5314..e13dcd6c9 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -1,4 +1,27 @@ module TableBuilderHelper + class Column + attr_reader :key, :name, :attribute, :sortable + + def initialize(key: nil, name: '', attribute:, sortable: true) + if key.nil? && name.empty? + raise ColumnMustHaveKeyOrNameError + end + + @key = key + @name = name + @attribute = attribute + @sortable = sortable + end + + def key_or_name + return @key unless @key.nil? + return @name unless @name.empty? + end + end + + class ColumnMustHaveKeyOrNameError < StandardError; end + + # TODO: rename this after migration from `table_builder` def table_builder_2( collection, @@ -35,16 +58,11 @@ module TableBuilderHelper hcont << content_tag(:th, checkbox(id_name: '0', value: 'all')) end - columns.map do |key, attribute| - sortable = true - if attribute.is_a?(Hash) && attribute.has_key?(:sortable) - sortable = attribute[:sortable] - end - + columns.map do |column| hcont << content_tag(:th, build_column_header( collection.model, - key, - sortable, + column.key_or_name, + column.sortable, params[:sort], params[:direction] )) @@ -72,19 +90,15 @@ module TableBuilderHelper ) end - columns.map do |k, attribute| - if attribute.is_a?(Hash) - attribute = attribute[:attribute] - end - + columns.map do |column| value = - if Proc === attribute - attribute.call(item) + if Proc === column.attribute + column.attribute.call(item) else - item.try(attribute) + item.try(column.attribute) end # if so this column's contents get transformed into a link to the object - if attribute == 'name' || attribute == 'comment' + if column.attribute == 'name' || column.attribute == 'comment' polymorph_url = polymorphic_url_parts(item) bcont << content_tag(:td, link_to(value, polymorph_url), title: 'Voir') else diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index 029748dd2..10cd6772b 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -101,15 +101,40 @@ describe TableBuilderHelper, type: :helper do html_str = helper.table_builder_2( referentials, - { :name => 'name', - :status => Proc.new {|w| w.archived? ? ("
Conservé
").html_safe : ("
En préparation
").html_safe}, - :status => Proc.new {|w| ("
En préparation
").html_safe}, - :organisation => Proc.new {|w| w.organisation.name}, - :validity_period => Proc.new {|w| w.validity_period.nil? ? '-' : t('validity_range', debut: l(w.try(:validity_period).try(:begin), format: :short), end: l(w.try(:validity_period).try(:end), format: :short))}, - :lines => Proc.new {|w| w.lines.count}, - :created_at => Proc.new {|w| l(w.created_at, format: :short)}, - :updated_at => Proc.new {|w| l(w.updated_at, format: :short)}, - :published_at => ''}, + [ + TableBuilderHelper::Column.new( + key: :name, + attribute: 'name' + ), + TableBuilderHelper::Column.new( + key: :status, + attribute: Proc.new {|w| w.archived? ? ("
Conservé
").html_safe : ("
En préparation
").html_safe} + ), + TableBuilderHelper::Column.new( + key: :organisation, + attribute: Proc.new {|w| w.organisation.name} + ), + TableBuilderHelper::Column.new( + key: :validity_period, + attribute: Proc.new {|w| w.validity_period.nil? ? '-' : t('validity_range', debut: l(w.try(:validity_period).try(:begin), format: :short), end: l(w.try(:validity_period).try(:end), format: :short))} + ), + TableBuilderHelper::Column.new( + key: :lines, + attribute: Proc.new {|w| w.lines.count} + ), + TableBuilderHelper::Column.new( + key: :created_at, + attribute: Proc.new {|w| l(w.created_at, format: :short)} + ), + TableBuilderHelper::Column.new( + key: :updated_at, + attribute: Proc.new {|w| l(w.updated_at, format: :short)} + ), + TableBuilderHelper::Column.new( + key: :published_at, + attribute: '' + ) + ], selectable: true, links: [:show, :edit, :archive, :unarchive, :delete], cls: 'table has-filter has-search' @@ -190,16 +215,29 @@ describe TableBuilderHelper, type: :helper do html_str = helper.table_builder_2( companies, - { - 'ID Codif' => { + [ + TableBuilderHelper::Column.new( + name: 'ID Codif', attribute: Proc.new { |n| n.try(:objectid).try(:local_id) }, sortable: false - }, - :name => 'name', - :phone => 'phone', - :email => 'email', - :url => 'url' - }, + ), + TableBuilderHelper::Column.new( + key: :name, + attribute: 'name' + ), + TableBuilderHelper::Column.new( + key: :phone, + attribute: 'phone' + ), + TableBuilderHelper::Column.new( + key: :email, + attribute: 'email' + ), + TableBuilderHelper::Column.new( + key: :url, + attribute: 'url' + ), + ], links: [:show, :edit, :delete], cls: 'table has-search' ) -- cgit v1.2.3 From 13d921fa0a3c263f30feacc91b337551b99b5865 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 12 Jun 2017 15:32:51 +0200 Subject: TableBuilder#build_column_header: Take Column instead of key & sortable Now that we have a real `Column` type, it's much easier to just pass a `Column` object to the method instead of these disconnected attributes. Now we can get the column attributes from the object itself. Remove the `#key_or_name` method as it's no longer used. Also, I decided to reorder the `#build_column_header` arguments to make `column` the first argument. Figured this made more sense as the method is all about a column, and the collection model is this weird secondary thing. Refs #3479 --- app/helpers/table_builder_helper.rb | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index e13dcd6c9..4f859dfea 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -12,11 +12,6 @@ module TableBuilderHelper @attribute = attribute @sortable = sortable end - - def key_or_name - return @key unless @key.nil? - return @name unless @name.empty? - end end class ColumnMustHaveKeyOrNameError < StandardError; end @@ -60,9 +55,8 @@ module TableBuilderHelper columns.map do |column| hcont << content_tag(:th, build_column_header( + column, collection.model, - column.key_or_name, - column.sortable, params[:sort], params[:direction] )) @@ -178,9 +172,8 @@ module TableBuilderHelper # TODO: clean up? def build_column_header( + column, collection_model, - key, - sortable, sort_on, sort_direction ) @@ -188,11 +181,11 @@ module TableBuilderHelper # (byebug) collection.model # Referential(id: integer, name: string, slug: string, created_at: datetime, updated_at: datetime, prefix: string, projection_type: string, time_zone: string, bounds: string, organisation_id: integer, geographical_bounds: text, user_id: integer, user_name: string, data_format: string, line_referential_id: integer, stop_area_referential_id: integer, workbench_id: integer, archived_at: datetime, created_from_id: integer, ready: boolean) # params = {"controller"=>"workbenches", "action"=>"show", "id"=>"1", "q"=>{"archived_at_not_null"=>"1", "archived_at_null"=>"1"}} - return key if !sortable + return column.name if !column.sortable - direction = (key.to_s == sort_on && sort_direction == 'desc') ? 'asc' : 'desc' + direction = (column.key.to_s == sort_on && sort_direction == 'desc') ? 'asc' : 'desc' - link_to(params.merge({direction: direction, sort: key})) do + link_to(params.merge({direction: direction, sort: column.key})) do pic1 = content_tag :span, '', class: "fa fa-sort-asc #{(direction == 'desc') ? 'active' : ''}" pic2 = content_tag :span, '', class: "fa fa-sort-desc #{(direction == 'asc') ? 'active' : ''}" @@ -200,7 +193,7 @@ module TableBuilderHelper # TODO: figure out a way to maybe explicitise the dynamicness of getting the model type from the `collection`. # TODO: rename `pics` to something like `icons` or arrow icons or some such - (column_header_label(collection_model, key) + pics).html_safe + (column_header_label(collection_model, column.key) + pics).html_safe end end -- cgit v1.2.3 From d259a8927b33ef5232807f383aa7d9e651c090d3 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 12 Jun 2017 15:39:09 +0200 Subject: TableBuilder spec: Improve whitespace of column definitions Add a few extra newlines to make these sections a bit easier to read. Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index 10cd6772b..c685df4e4 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -108,7 +108,13 @@ describe TableBuilderHelper, type: :helper do ), TableBuilderHelper::Column.new( key: :status, - attribute: Proc.new {|w| w.archived? ? ("
Conservé
").html_safe : ("
En préparation
").html_safe} + attribute: Proc.new do |w| + if w.archived? + ("
Conservé
").html_safe + else + ("
En préparation
").html_safe + end + end ), TableBuilderHelper::Column.new( key: :organisation, @@ -116,7 +122,17 @@ describe TableBuilderHelper, type: :helper do ), TableBuilderHelper::Column.new( key: :validity_period, - attribute: Proc.new {|w| w.validity_period.nil? ? '-' : t('validity_range', debut: l(w.try(:validity_period).try(:begin), format: :short), end: l(w.try(:validity_period).try(:end), format: :short))} + attribute: Proc.new do |w| + if w.validity_period.nil? + '-' + else + t( + 'validity_range', + debut: l(w.try(:validity_period).try(:begin), format: :short), + end: l(w.try(:validity_period).try(:end), format: :short) + ) + end + end ), TableBuilderHelper::Column.new( key: :lines, -- cgit v1.2.3 From 295e1fe4503bfb580da1e1a55601897786d87a8b Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 12 Jun 2017 15:43:08 +0200 Subject: TableBuilderHelper#build_column_header: Improve whitespace Refs #3479 --- app/helpers/table_builder_helper.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 4f859dfea..97234753b 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -183,7 +183,12 @@ module TableBuilderHelper # params = {"controller"=>"workbenches", "action"=>"show", "id"=>"1", "q"=>{"archived_at_not_null"=>"1", "archived_at_null"=>"1"}} return column.name if !column.sortable - direction = (column.key.to_s == sort_on && sort_direction == 'desc') ? 'asc' : 'desc' + direction = + if column.key.to_s == sort_on && sort_direction == 'desc' + 'asc' + else + 'desc' + end link_to(params.merge({direction: direction, sort: column.key})) do pic1 = content_tag :span, '', class: "fa fa-sort-asc #{(direction == 'desc') ? 'active' : ''}" -- cgit v1.2.3 From 0159b8b8127b91a4eff8bfeaccc17df9504aa3b1 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 12 Jun 2017 15:48:20 +0200 Subject: TableBuilder#build_column_header: Rename `pic` variables to `arrow` The name "pic" felt too ambiguous. Instead, give these variables names that reflect the icons that they represent. Also add some whitespace around to get the lines below 80 columns. Refs #3479 --- app/helpers/table_builder_helper.rb | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 97234753b..b61b169f1 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -191,14 +191,24 @@ module TableBuilderHelper end link_to(params.merge({direction: direction, sort: column.key})) do - pic1 = content_tag :span, '', class: "fa fa-sort-asc #{(direction == 'desc') ? 'active' : ''}" - pic2 = content_tag :span, '', class: "fa fa-sort-desc #{(direction == 'asc') ? 'active' : ''}" + arrow_up = content_tag( + :span, + '', + class: "fa fa-sort-asc #{direction == 'desc' ? 'active' : ''}" + ) + arrow_down = content_tag( + :span, + '', + class: "fa fa-sort-desc #{direction == 'asc' ? 'active' : ''}" + ) - pics = content_tag :span, pic1 + pic2, class: 'orderers' + arrow_icons = content_tag :span, arrow_up + arrow_down, class: 'orderers' # TODO: figure out a way to maybe explicitise the dynamicness of getting the model type from the `collection`. - # TODO: rename `pics` to something like `icons` or arrow icons or some such - (column_header_label(collection_model, column.key) + pics).html_safe + ( + column_header_label(collection_model, column.key) + + arrow_icons + ).html_safe end end -- cgit v1.2.3 From e7b663195bcf3370a4cca10d97453e62101e3f38 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 12 Jun 2017 16:10:06 +0200 Subject: TableBuilder#build_column_header: Pass `params` explicitly I don't like that the method is using a global variable. It makes it harder to read. Still don't like that we even use `params` in this method. It would be a lot better if we could extricate it completely, but I'll settle for this for now as there are other refactorings to get to. Refs #3479 --- app/helpers/table_builder_helper.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index b61b169f1..090f78c0a 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -57,6 +57,7 @@ module TableBuilderHelper hcont << content_tag(:th, build_column_header( column, collection.model, + params, params[:sort], params[:direction] )) @@ -174,6 +175,7 @@ module TableBuilderHelper def build_column_header( column, collection_model, + params, sort_on, sort_direction ) -- cgit v1.2.3 From d488e4e7251d3e0cc5b8ffe8ddbe6960fa99d14c Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 12 Jun 2017 16:13:34 +0200 Subject: TableBuilder: Add a couple TODOs I had added these last week uncommitted, but still haven't gotten to them. Figure I should just commit them to save them and remove them when I get to them. Refs #3479 --- app/helpers/table_builder_helper.rb | 1 + spec/helpers/table_builder_helper_spec.rb | 1 + 2 files changed, 2 insertions(+) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 090f78c0a..7cdc7eabb 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -127,6 +127,7 @@ module TableBuilderHelper if action == :delete if policy(item).present? if policy(item).destroy? + # TODO: This tag is exactly the same as the one below it content_tag :li, '', class: 'delete-action' do link_to(polymorph_url, method: :delete, data: { confirm: 'Etes-vous sûr(e) de vouloir effectuer cette action ?' }) do txt = t("actions.#{action}") diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index c685df4e4..b90053015 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -39,6 +39,7 @@ describe TableBuilderHelper, type: :helper do id: referentials[0].workbench.id, # These are added by WorkbenchesController#query_params + # TODO: Remove these params from here and the expected HTML as they don't relate to the test at hand q: { archived_at_not_null: 1, archived_at_null: 1 -- cgit v1.2.3 From 636cc10744d4a1f3c4ff5c1579a9c83570e23f08 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 12 Jun 2017 16:15:42 +0200 Subject: TableBuilder#build_column_header: Remove TODOs and reference comments Deciding that I'm finished with the cleanup of this method for now. Suggestions for improvement: - Remove dependency on `params` - Figure out a way to get the name without `collection_model` Refs #3479 --- app/helpers/table_builder_helper.rb | 6 ------ 1 file changed, 6 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 7cdc7eabb..1642ecc8f 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -172,7 +172,6 @@ module TableBuilderHelper end - # TODO: clean up? def build_column_header( column, collection_model, @@ -180,10 +179,6 @@ module TableBuilderHelper sort_on, sort_direction ) - # #, #]> - # (byebug) collection.model - # Referential(id: integer, name: string, slug: string, created_at: datetime, updated_at: datetime, prefix: string, projection_type: string, time_zone: string, bounds: string, organisation_id: integer, geographical_bounds: text, user_id: integer, user_name: string, data_format: string, line_referential_id: integer, stop_area_referential_id: integer, workbench_id: integer, archived_at: datetime, created_from_id: integer, ready: boolean) - # params = {"controller"=>"workbenches", "action"=>"show", "id"=>"1", "q"=>{"archived_at_not_null"=>"1", "archived_at_null"=>"1"}} return column.name if !column.sortable direction = @@ -206,7 +201,6 @@ module TableBuilderHelper ) arrow_icons = content_tag :span, arrow_up + arrow_down, class: 'orderers' - # TODO: figure out a way to maybe explicitise the dynamicness of getting the model type from the `collection`. ( column_header_label(collection_model, column.key) + -- cgit v1.2.3 From 54be146969fb0bfd099e12b5b66ef102dbca3258 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 12 Jun 2017 16:18:39 +0200 Subject: TableBuilder: Add whitespace Refs #3479 --- app/helpers/table_builder_helper.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 1642ecc8f..acb3aec8a 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -62,6 +62,7 @@ module TableBuilderHelper params[:direction] )) end + # Inserts a blank column for the gear menu hcont << content_tag(:th, '') if has_links -- cgit v1.2.3 From 416ac094aa3ae1a8e0ea4c088da8976815c63a2e Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 12 Jun 2017 16:20:34 +0200 Subject: TableBuilder spec: Remove `q` params from test These params are not relevant to this test, they just clutter things up. I had originally added them because the `/workbenches/:id` page used them and I wanted to mimic the table builder result as closely as possible. Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index b90053015..756fac861 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -36,14 +36,7 @@ describe TableBuilderHelper, type: :helper do allow(helper).to receive(:params).and_return({ controller: 'workbenches', action: 'show', - id: referentials[0].workbench.id, - - # These are added by WorkbenchesController#query_params - # TODO: Remove these params from here and the expected HTML as they don't relate to the test at hand - q: { - archived_at_not_null: 1, - archived_at_null: 1 - } + id: referentials[0].workbench.id }) expected = <<-HTML @@ -53,14 +46,14 @@ describe TableBuilderHelper, type: :helper do
- Nom - Etat - Organisation - Période de validité englobante - Lignes - Créé le - Edité le - Intégré le + Nom + Etat + Organisation + Période de validité englobante + Lignes + Créé le + Edité le + Intégré le -- cgit v1.2.3 From 6125263e31fb5518606b6c61ceb271978c9233c1 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 12 Jun 2017 16:24:30 +0200 Subject: TableBuilderHelper::Column: Add TODO for better `attribute` handling Refs #3479 --- app/helpers/table_builder_helper.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index acb3aec8a..1244464fc 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -7,6 +7,11 @@ module TableBuilderHelper raise ColumnMustHaveKeyOrNameError end + # TODO: Make a similar _ && _ for either `attribute`, which should be a + # string attribute, and another param, which should represent a proc. + # Then maybe have an `#attribute` method that returns either the string + # or the result of the proc + @key = key @name = name @attribute = attribute -- cgit v1.2.3 From 26d3971892dbeacffba678a3dd9593095da14b29 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 12 Jun 2017 16:58:25 +0200 Subject: ReferentialsController#show: Fix whitespace Method was indented by three spaces. Should be two. Refs #3479 --- app/controllers/referentials_controller.rb | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/app/controllers/referentials_controller.rb b/app/controllers/referentials_controller.rb index 437444f29..c65e6552c 100644 --- a/app/controllers/referentials_controller.rb +++ b/app/controllers/referentials_controller.rb @@ -24,19 +24,19 @@ class ReferentialsController < BreadcrumbController end def show - resource.switch - show! do |format| - format.json { - render :json => { :lines_count => resource.lines.count, - :networks_count => resource.networks.count, - :vehicle_journeys_count => resource.vehicle_journeys.count + resource.vehicle_journey_frequencies.count, - :time_tables_count => resource.time_tables.count, - :referential_id => resource.id} - } - format.html { build_breadcrumb :show} - end - - @reflines = lines_collection.paginate(page: params[:page], per_page: 10) + resource.switch + show! do |format| + format.json { + render :json => { :lines_count => resource.lines.count, + :networks_count => resource.networks.count, + :vehicle_journeys_count => resource.vehicle_journeys.count + resource.vehicle_journey_frequencies.count, + :time_tables_count => resource.time_tables.count, + :referential_id => resource.id} + } + format.html { build_breadcrumb :show} + end + + @reflines = lines_collection.paginate(page: params[:page], per_page: 10) end def edit -- cgit v1.2.3 From 53c68600f1e3f80d2765fe41ed3a451c17eeabea Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 12 Jun 2017 18:45:29 +0200 Subject: workbenches/show.html.slim: Convert table_builder to new columns Use our new column syntax to build the table, creating `TableBuilderHelper::Column` objects for each column. This gets our table rendering again with the new `table_builder_2` code. Remove the old table builder code as we're going to switch entirely to this one. Get rid of the TODO about the =>s as it's no longer relevant. Refs #3479 --- app/views/workbenches/show.html.slim | 55 ++++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 21 deletions(-) diff --git a/app/views/workbenches/show.html.slim b/app/views/workbenches/show.html.slim index 61b9395fa..3c2bb26ec 100644 --- a/app/views/workbenches/show.html.slim +++ b/app/views/workbenches/show.html.slim @@ -22,28 +22,41 @@ - if @wbench_refs.any? .row .col-lg-12 - /= table_builder @wbench_refs, - { :name => 'name', - :status => Proc.new {|w| w.archived? ? ("
Conservé
").html_safe : ("
En préparation
").html_safe}, - :organisation => Proc.new {|w| w.organisation.name}, - :validity_period => Proc.new {|w| w.validity_period.nil? ? '-' : t('validity_range', debut: l(w.try(:validity_period).try(:begin), format: :short), end: l(w.try(:validity_period).try(:end), format: :short))}, - :lines => Proc.new {|w| w.lines.count}, - :created_at => Proc.new {|w| l(w.created_at, format: :short)}, - :updated_at => Proc.new {|w| l(w.updated_at, format: :short)}, - :published_at => ''}, - [:show, :edit, :archive, :unarchive, :delete], - [:delete], - 'table has-filter has-search' - / TODO: change => to : = table_builder_2 @wbench_refs, - { :name => 'name', - :status => Proc.new {|w| w.archived? ? ("
Conservé
").html_safe : ("
En préparation
").html_safe}, - :organisation => Proc.new {|w| w.organisation.name}, - :validity_period => Proc.new {|w| w.validity_period.nil? ? '-' : t('validity_range', debut: l(w.try(:validity_period).try(:begin), format: :short), end: l(w.try(:validity_period).try(:end), format: :short))}, - :lines => Proc.new {|w| w.lines.count}, - :created_at => Proc.new {|w| l(w.created_at, format: :short)}, - :updated_at => Proc.new {|w| l(w.updated_at, format: :short)}, - :published_at => ''}, + [ \ + TableBuilderHelper::Column.new( \ + key: :name, \ + attribute: 'name' \ + ), \ + TableBuilderHelper::Column.new( \ + key: :status, \ + attribute: Proc.new {|w| w.archived? ? ("
Conservé
").html_safe : ("
En préparation
").html_safe} \ + ), \ + TableBuilderHelper::Column.new( \ + key: :organisation, \ + attribute: Proc.new {|w| w.organisation.name} \ + ), \ + TableBuilderHelper::Column.new( \ + key: :validity_period, \ + attribute: Proc.new {|w| w.validity_period.nil? ? '-' : t('validity_range', debut: l(w.try(:validity_period).try(:begin), format: :short), end: l(w.try(:validity_period).try(:end), format: :short))} \ + ), \ + TableBuilderHelper::Column.new( \ + key: :lines, \ + attribute: Proc.new {|w| w.lines.count} \ + ), \ + TableBuilderHelper::Column.new( \ + key: :created_at, \ + attribute: Proc.new {|w| l(w.created_at, format: :short)} \ + ), \ + TableBuilderHelper::Column.new( \ + key: :updated_at, \ + attribute: Proc.new {|w| l(w.updated_at, format: :short)} \ + ), \ + TableBuilderHelper::Column.new( \ + key: :published_at, \ + attribute: '' \ + ) \ + ], selectable: true, links: [:show, :edit, :archive, :unarchive, :delete], cls: 'table has-filter has-search' -- cgit v1.2.3 From 07ed048cd2f2231fa26345a94c725363344ee77b Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 12 Jun 2017 18:59:23 +0200 Subject: referentials/show.html.slim: Extract header buttons to a decorator Add a new `ReferentialDecorator` that we can use to define the links that appear as buttons in the header of the `/referentials/:id` page. The idea is that these links will take the place of the existing view code and in the view, we'll instead loop over the links provided by the decorator and render them as buttons. I decided to do it this way because there's a requirement that the links in the gear menu in tables show the same links that appear in the header of the page. So the Workbenches#show page would show a table of referentials, and their gear menu links should ostensibly be the same as the header links in the Referentials#show page. My goal was to abstract links, and try to separate them from the presentation layer. Still having trouble with this though. I created a new `Link` class to represent a link. We can then use this in the template to create a `link_to`. It becomes messy, though, when we want to put other elements inside a certain link, as evidenced by the `:delete` link. Don't like how that's done, but still working out the best approach to make it cleaner. Refs #3479 --- app/controllers/referentials_controller.rb | 2 ++ app/decorators/referential_decorator.rb | 47 ++++++++++++++++++++++++++++++ app/views/referentials/show.html.slim | 47 ++++++++++++++++++++---------- lib/link.rb | 10 +++++++ 4 files changed, 90 insertions(+), 16 deletions(-) create mode 100644 app/decorators/referential_decorator.rb create mode 100644 lib/link.rb diff --git a/app/controllers/referentials_controller.rb b/app/controllers/referentials_controller.rb index c65e6552c..50b2e47c6 100644 --- a/app/controllers/referentials_controller.rb +++ b/app/controllers/referentials_controller.rb @@ -26,6 +26,8 @@ class ReferentialsController < BreadcrumbController def show resource.switch show! do |format| + @referential = ReferentialDecorator.new(@referential) + format.json { render :json => { :lines_count => resource.lines.count, :networks_count => resource.networks.count, diff --git a/app/decorators/referential_decorator.rb b/app/decorators/referential_decorator.rb new file mode 100644 index 000000000..1c2347529 --- /dev/null +++ b/app/decorators/referential_decorator.rb @@ -0,0 +1,47 @@ +class ReferentialDecorator < Draper::Decorator + delegate_all + + def action_links + links = [ + Link.new( + name: h.t('time_tables.index.title'), + href: h.referential_time_tables_path(object) + ) + ] + + if h.policy(object).clone? + links << Link.new( + name: h.t('actions.clone'), + href: h.new_referential_path(from: object.id) + ) + end + + if h.policy(object).edit? + # button.btn.btn-primary type='button' data-toggle='modal' data-target='#purgeModal' Purger + + if object.archived? + links << Link.new( + name: h.t('actions.unarchive'), + href: h.unarchive_referential_path(object.id), + method: :put + ) + else + links << Link.new( + name: h.t('actions.archive'), + href: h.archive_referential_path(object.id), + method: :put + ) + end + end + + if h.policy(object).destroy? + links << Link.new( + href: h.referential_path(object), + method: :delete, + data: { confirm: h.t('referentials.actions.destroy_confirm') } + ) + end + + links + end +end diff --git a/app/views/referentials/show.html.slim b/app/views/referentials/show.html.slim index 3c1e36302..1bbf350b4 100644 --- a/app/views/referentials/show.html.slim +++ b/app/views/referentials/show.html.slim @@ -8,23 +8,38 @@ / Below is secondary actions & optional contents (filters, ...) .row.mb-sm .col-lg-12.text-right - = link_to t('time_tables.index.title'), referential_time_tables_path(@referential), class: 'btn btn-primary' - - - if policy(@referential).clone? - = link_to t('actions.clone'), new_referential_path(from: @referential.id), class: 'btn btn-primary' - - - if policy(@referential).edit? - button.btn.btn-primary type='button' data-toggle='modal' data-target='#purgeModal' Purger - - - if @referential.archived? - = link_to t('actions.unarchive'), unarchive_referential_path(@referential.id), method: :put, class: 'btn btn-primary' + /= link_to t('time_tables.index.title'), referential_time_tables_path(@referential), class: 'btn btn-primary' + / + /- if policy(@referential).clone? + / = link_to t('actions.clone'), new_referential_path(from: @referential.id), class: 'btn btn-primary' + / + /- if policy(@referential).edit? + / button.btn.btn-primary type='button' data-toggle='modal' data-target='#purgeModal' Purger + / + / - if @referential.archived? + / = link_to t('actions.unarchive'), unarchive_referential_path(@referential.id), method: :put, class: 'btn btn-primary' + / - else + / = link_to t('actions.archive'), archive_referential_path(@referential.id), method: :put, class: 'btn btn-primary' + / + /- if policy(@referential).destroy? + / = link_to referential_path(@referential), method: :delete, data: {confirm: t('referentials.actions.destroy_confirm')}, class: 'btn btn-primary' do + / span.fa.fa-trash + / span = t('actions.destroy') + /= leslinks.map { |l| link_to l.href, l.label } + - @referential.action_links.each do |link| + - if link.method == :delete + = link_to link.href, + method: link.method, + data: link.data, + class: 'btn btn-primary' do + span.fa.fa-trash + span = t('actions.destroy') - else - = link_to t('actions.archive'), archive_referential_path(@referential.id), method: :put, class: 'btn btn-primary' - - - if policy(@referential).destroy? - = link_to referential_path(@referential), method: :delete, data: {confirm: t('referentials.actions.destroy_confirm')}, class: 'btn btn-primary' do - span.fa.fa-trash - span = t('actions.destroy') + = link_to link.name, + link.href, + method: link.method, + data: link.data, + class: 'btn btn-primary' / PageContent .page_content diff --git a/lib/link.rb b/lib/link.rb new file mode 100644 index 000000000..911f189c9 --- /dev/null +++ b/lib/link.rb @@ -0,0 +1,10 @@ +class Link + attr_reader :name, :href, :method, :data + + def initialize(name: nil, href:, method: :get, data: nil) + @name = name + @href = href + @method = method + @data = data + end +end -- cgit v1.2.3 From 4ec831984516cb29e93f8d45f205bdbaf04096b7 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Tue, 13 Jun 2017 11:42:39 +0200 Subject: TableBuilder: Move comment and rewrite for others Previously I had written this comment for myself. Refs #3479 --- app/helpers/table_builder_helper.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 1244464fc..112a2f033 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -98,8 +98,9 @@ module TableBuilderHelper else item.try(column.attribute) end - # if so this column's contents get transformed into a link to the object + if column.attribute == 'name' || column.attribute == 'comment' + # Build a link to the `item` polymorph_url = polymorphic_url_parts(item) bcont << content_tag(:td, link_to(value, polymorph_url), title: 'Voir') else -- cgit v1.2.3 From d8f9a60e6ce5e334bd8022ed571b06cccb90ec2a Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Tue, 13 Jun 2017 11:45:44 +0200 Subject: TableBuilder: Rename `#links_builder` to `#build_links` The method name makes more sense to me as a verb. Also add a little writespace making the code more vertical for readability. Refs #3479 --- app/helpers/table_builder_helper.rb | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 112a2f033..2d32207aa 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -107,7 +107,14 @@ module TableBuilderHelper bcont << content_tag(:td, value) end end - bcont << content_tag(:td, links_builder(item, links), class: 'actions') if links.any? + + if links.any? + bcont << content_tag( + :td, + build_links(item, links), + class: 'actions' + ) + end bcont.join.html_safe end @@ -115,8 +122,7 @@ module TableBuilderHelper end end - # TODO: `def build_link[s]` - def links_builder(item, actions) + def build_links(item, actions) trigger = content_tag :div, class: 'btn dropdown-toggle', data: { toggle: 'dropdown' } do content_tag :span, '', class: 'fa fa-cog' end -- cgit v1.2.3 From 1c70b12b7c71a428bfafc601d5538392ad2113bb Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Tue, 13 Jun 2017 18:11:25 +0200 Subject: TableBuilderHelper::Column: Add #value method A new method that will return the value that should be in the column given an object. This extracts the "if Proc..." code that we used to have in the `#tbody` method, making it a little cleaner. Refs #3479 --- app/helpers/table_builder_helper.rb | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 2d32207aa..6f9073662 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -7,16 +7,19 @@ module TableBuilderHelper raise ColumnMustHaveKeyOrNameError end - # TODO: Make a similar _ && _ for either `attribute`, which should be a - # string attribute, and another param, which should represent a proc. - # Then maybe have an `#attribute` method that returns either the string - # or the result of the proc - @key = key @name = name @attribute = attribute @sortable = sortable end + + def value(obj) + if @attribute.is_a?(Proc) + @attribute.call(obj) + else + obj.try(@attribute) + end + end end class ColumnMustHaveKeyOrNameError < StandardError; end @@ -92,12 +95,7 @@ module TableBuilderHelper end columns.map do |column| - value = - if Proc === column.attribute - column.attribute.call(item) - else - item.try(column.attribute) - end + value = column.value(item) if column.attribute == 'name' || column.attribute == 'comment' # Build a link to the `item` -- cgit v1.2.3 From 734de24950c6ae490d4781799c8a6d805944e6aa Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Wed, 14 Jun 2017 11:14:16 +0200 Subject: Workbenches#show: Use common referential action_links in table gear menu Get our `ReferentialDecorator` working for a collection of `Referential`s. We decorate the `ActiveRecord::Relation` of referentials that gets passed to the table builder. This enables us to call the `#action_links` method on those referentials, getting a list of links that should appear in the gear menu. From that list, we can generate the proper HTML to stick into the menu. This was my first test of reusing the `#action_links` for both the header buttons in Referentials#show and the list in Workbenches#show. Ran into some trouble. The issue was that decorating the collection with Draper's defaults doesn't delegate methods on the collection object (in other words, `ActiveRecord::Relation`). We call methods on the collection in our code, and doing so on our new decorated collection caused errors. As a first step to get around the first batch of errors, I changed the `_filters.html.slim` template to call `#human_attribute_name` on `Referential` model directly, instead of on the Relation. There was some additional weirdness that Luc tipped me off to as we have a `Chouette::ActiveRecord` class that defines its own `#human_attribute_name` method. Switching the callee to the model fixed this error. Subsequently, I ran into a problem in the `TableBuilderHelper`. In that code, we call `collection.model` to determine the class of objects in our table. Now that I think about it, maybe we should just pass this information when calling `table_builder`. Let's do that because it would be better. Anyway, to enable that `ActiveRecord::Relation#model` method to be called, I created a Draper `CollectionDecorator` that delegates the `#model` method. This allows us to call `#model` on the decorated collection of referentials (or other objects). Finally, I ran into another delegate problem for our pagination method calls. Fortunately, Draper provides some code for a `CollectionDecorator` for pagination. I copied this and used it in conjunction with my custom `ModelDecorator` to properly delegate all necessary collection methods for a collection of Referentials. Stuck my `PaginatingDecorator` and `ModelDecorator` right in the `referential_decorator.rb` file for now just for testing until I got things working. Needed to import this file in the controller where we decorate the collection. Not entirely sure why but my guess is that it's because the loader connects classes to file names and we have several different classes in that file. Maybe I'm totally wrong on that. Anyway, will be moving those classes to separate files later. In the controller, decorate our `Referential` collection with the `ModelDecorator` (which inherits from `PaginatingDecorator`, giving us those delegated methods), and ensure the objects inside the collection get decorated with our original `ReferentialDecorator`. In `TableBuilderHelper#build_links`, comment out all the existing link building code and instead map over the `#action_links` provided by the decorator. Currently this should only work for `Referential` collections, but the idea is to use that pattern for all table objects. NOTE: We should add a doc comment to the table builder that tells people that they need a decorator for their model in order for it to work. Refs #3479 --- app/controllers/workbenches_controller.rb | 7 ++ app/decorators/referential_decorator.rb | 17 +++++ app/helpers/table_builder_helper.rb | 109 ++++++++++++++++-------------- app/views/workbenches/_filters.html.slim | 4 +- 4 files changed, 85 insertions(+), 52 deletions(-) diff --git a/app/controllers/workbenches_controller.rb b/app/controllers/workbenches_controller.rb index ccd55965b..56f9136b3 100644 --- a/app/controllers/workbenches_controller.rb +++ b/app/controllers/workbenches_controller.rb @@ -1,3 +1,6 @@ +# TODO: Try not importing +require 'referential_decorator' + class WorkbenchesController < BreadcrumbController before_action :query_params, only: [:show] @@ -14,6 +17,10 @@ class WorkbenchesController < BreadcrumbController q_for_result = scope.ransack(params[:q].merge(archived_at_not_null: nil, archived_at_null: nil)) @wbench_refs = sort_result(q_for_result.result).paginate(page: params[:page], per_page: 30) + @wbench_refs = ModelDecorator.decorate( + @wbench_refs, + with: ReferentialDecorator + ) @q = scope.ransack(params[:q]) show! do diff --git a/app/decorators/referential_decorator.rb b/app/decorators/referential_decorator.rb index 1c2347529..45a4b38d1 100644 --- a/app/decorators/referential_decorator.rb +++ b/app/decorators/referential_decorator.rb @@ -1,3 +1,20 @@ +# Delegates 'will_paginate' methods +class PaginatingDecorator < Draper::CollectionDecorator + delegate( + :current_page, + :per_page, + :offset, + :total_entries, + :total_pages + ) +end + + +class ModelDecorator < PaginatingDecorator + delegate :model +end + + class ReferentialDecorator < Draper::Decorator delegate_all diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 6f9073662..47401a807 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -126,57 +126,66 @@ module TableBuilderHelper end menu = content_tag :ul, class: 'dropdown-menu' do - actions.map do |action| - polymorph_url = [] - - unless [:show, :delete].include? action - polymorph_url << action - end - - polymorph_url += polymorphic_url_parts(item) - - if action == :delete - if policy(item).present? - if policy(item).destroy? - # TODO: This tag is exactly the same as the one below it - content_tag :li, '', class: 'delete-action' do - link_to(polymorph_url, method: :delete, data: { confirm: 'Etes-vous sûr(e) de vouloir effectuer cette action ?' }) do - txt = t("actions.#{action}") - pic = content_tag :span, '', class: 'fa fa-trash' - pic + txt - end - end - end - else - content_tag :li, '', class: 'delete-action' do - link_to(polymorph_url, method: :delete, data: { confirm: 'Etes-vous sûr(e) de vouloir effectuer cette action ?' }) do - txt = t("actions.#{action}") - pic = content_tag :span, '', class: 'fa fa-trash' - pic + txt - end - end - end - - elsif action == :edit - if policy(item).present? - if policy(item).update? - content_tag :li, link_to(t("actions.#{action}"), polymorph_url) - end - else - content_tag :li, link_to(t("actions.#{action}"), polymorph_url) - end - elsif action == :archive - unless item.archived? - content_tag :li, link_to(t("actions.#{action}"), polymorph_url, method: :put) - end - elsif action == :unarchive - if item.archived? - content_tag :li, link_to(t("actions.#{action}"), polymorph_url, method: :put) - end - else - content_tag :li, link_to(t("actions.#{action}"), polymorph_url) - end + item.action_links.map do |link| + content_tag :li, link_to( + link.name, + link.href, + method: link.method, + data: link.data + ) end.join.html_safe + + # actions.map do |action| + # polymorph_url = [] + # + # unless [:show, :delete].include? action + # polymorph_url << action + # end + # + # polymorph_url += polymorphic_url_parts(item) + # + # if action == :delete + # if policy(item).present? + # if policy(item).destroy? + # # TODO: This tag is exactly the same as the one below it + # content_tag :li, '', class: 'delete-action' do + # link_to(polymorph_url, method: :delete, data: { confirm: 'Etes-vous sûr(e) de vouloir effectuer cette action ?' }) do + # txt = t("actions.#{action}") + # pic = content_tag :span, '', class: 'fa fa-trash' + # pic + txt + # end + # end + # end + # else + # content_tag :li, '', class: 'delete-action' do + # link_to(polymorph_url, method: :delete, data: { confirm: 'Etes-vous sûr(e) de vouloir effectuer cette action ?' }) do + # txt = t("actions.#{action}") + # pic = content_tag :span, '', class: 'fa fa-trash' + # pic + txt + # end + # end + # end + # + # elsif action == :edit + # if policy(item).present? + # if policy(item).update? + # content_tag :li, link_to(t("actions.#{action}"), polymorph_url) + # end + # else + # content_tag :li, link_to(t("actions.#{action}"), polymorph_url) + # end + # elsif action == :archive + # unless item.archived? + # content_tag :li, link_to(t("actions.#{action}"), polymorph_url, method: :put) + # end + # elsif action == :unarchive + # if item.archived? + # content_tag :li, link_to(t("actions.#{action}"), polymorph_url, method: :put) + # end + # else + # content_tag :li, link_to(t("actions.#{action}"), polymorph_url) + # end + # end.join.html_safe end content_tag :div, trigger + menu, class: 'btn-group' diff --git a/app/views/workbenches/_filters.html.slim b/app/views/workbenches/_filters.html.slim index 7c5055963..0aedbdd62 100644 --- a/app/views/workbenches/_filters.html.slim +++ b/app/views/workbenches/_filters.html.slim @@ -12,7 +12,7 @@ = f.input :associated_lines_id_eq, as: :select, collection: @workbench.lines.includes(:company).order(:name), input_html: { 'data-select2ed': 'true', 'data-select2ed-placeholder': 'Indiquez une ligne...' }, label: false, label_method: :display_name, wrapper_html: { class: 'select2ed'} .form-group.togglable - = f.label @wbench_refs.human_attribute_name(:status), required: false, class: 'control-label' + = f.label Referential.human_attribute_name(:status), required: false, class: 'control-label' .form-group.checkbox_list = f.input :archived_at_not_null, label: ("Conservé").html_safe, as: :boolean, wrapper_html: { class: 'checkbox-wrapper' } = f.input :archived_at_null, label: ("En préparation").html_safe, as: :boolean, wrapper_html: { class: 'checkbox-wrapper' } @@ -22,7 +22,7 @@ = f.input :organisation_name_eq_any, collection: Organisation.order('name').pluck(:name), as: :check_boxes, label: false, label_method: lambda{|w| ("#{w}").html_safe}, required: false, wrapper_html: { class: 'checkbox_list' } .form-group.togglable - = f.label @wbench_refs.human_attribute_name(:validity_period), required: false, class: 'control-label' + = f.label Referential.human_attribute_name(:validity_period), required: false, class: 'control-label' .filter_menu = f.simple_fields_for :validity_period do |p| = p.input :begin_gteq, as: :date, label: t('simple_form.from'), wrapper_html: { class: 'date filter_menu-item' }, default: @begin_range, include_blank: @begin_range ? false : true -- cgit v1.2.3 From 5266d9099b0cc82bfab20417d382e4324cdab013 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Wed, 14 Jun 2017 11:57:02 +0200 Subject: TableBuilder#tbody: Remove TODO This is looking good enough now to me. Refs #3479 --- app/helpers/table_builder_helper.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 47401a807..b65747056 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -80,7 +80,6 @@ module TableBuilderHelper end def tbody(collection, columns, selectable, links) - # TODO: refactor content_tag :tbody do collection.map do |item| -- cgit v1.2.3 From cd510d491181eb7a65559b9bbab600316e4f7b7b Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Wed, 14 Jun 2017 12:00:40 +0200 Subject: TableBuilderHelper#tbody: Move condition to method Give this check a clearer name. Refs #3479 --- app/helpers/table_builder_helper.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index b65747056..ef44d98c7 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -96,7 +96,7 @@ module TableBuilderHelper columns.map do |column| value = column.value(item) - if column.attribute == 'name' || column.attribute == 'comment' + if column_is_linkable?(column) # Build a link to the `item` polymorph_url = polymorphic_url_parts(item) bcont << content_tag(:td, link_to(value, polymorph_url), title: 'Voir') @@ -243,6 +243,10 @@ module TableBuilderHelper end end + def column_is_linkable?(column) + column.attribute == 'name' || column.attribute == 'comment' + end + def polymorphic_url_parts(item) polymorph_url = [] -- cgit v1.2.3 From f327ae88ebc45d291dc8485a0ade20a7e335740f Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Wed, 14 Jun 2017 15:37:35 +0200 Subject: TableBuilder: Trial refactor of `#build_links` Instead of the existing code that we have for `if policy...` etc. in `#build_links`, create a few new methods that ostensibly do the same thing using my `Link` class. It should create a bunch of `Link`s that can be combined with those that come from the `[TABLE-OBJECT].action_links` (like those in `ReferentialDecorator#action_links`). The combination of these will then be turned into `link_to`s by our existing code. Renamed the `actions` argument to `#build_links` to `links` because they come into `table_builder_2` as a `links` variable and I got confused by the name not being the same. The policy checking is now in its own method, `#actions_after_policy_check`. Need to verify that the logic is right, but otherwise it keeps things separate. Might also want to write special methods for each of the conditions between the `||`s. TODO: Move the new private methods to a class. Also test them. Refs #3479 --- app/helpers/table_builder_helper.rb | 60 +++++++++++++++++++++++++++++++++++-- 1 file changed, 58 insertions(+), 2 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index ef44d98c7..f4a3f9eb4 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -119,13 +119,13 @@ module TableBuilderHelper end end - def build_links(item, actions) + def build_links(item, links) trigger = content_tag :div, class: 'btn dropdown-toggle', data: { toggle: 'dropdown' } do content_tag :span, '', class: 'fa fa-cog' end menu = content_tag :ul, class: 'dropdown-menu' do - item.action_links.map do |link| + (action_links(links, item) + item.action_links).map do |link| content_tag :li, link_to( link.name, link.href, @@ -267,4 +267,60 @@ module TableBuilderHelper polymorph_url end + + # TODO: rename + def action_links(actions, obj) + actions_after_policy_check(actions, obj).map do |action| + # TODO: Move that s to another method + polymorph_url = [] + + unless [:show, :delete].include? action + polymorph_url << action + end + + polymorph_url += polymorphic_url_parts(obj) + + Link.new( + name: t("actions.#{action}"), + href: polymorph_url, + method: action_link_method(action) + ) + end + end + + def action_link_method(action) + actions_to_http_methods = { + delete: :delete, + archive: :put, + unarchive: :put + } + + actions_to_http_methods[action] || :get + end + + def actions_after_policy_check(actions, obj) + actions.select do |action| + # if action == :delete + # if policy(item).present? && policy(item).destroy? + # action + # end + # elsif action == :edit + # if policy(item).present? && policy(item).update? + # action + # end + # elsif action == :edit + # + # end + # if (action == :delete && policy(item).present? && policy(item).destroy?) || + (action == :delete && policy(obj).present? && policy(obj).destroy?) || + (action == :delete && !policy(obj).present?) || + (action == :edit && policy(obj).present? && policy(obj).update?) || + (action == :edit && !policy(obj).present?) || + (action == :archive && !obj.archived?) || + (action == :unarchive && obj.archived?) || + ([:delete, :edit, :archive, :unarchive].include?(action)) + # action + # end + end + end end -- cgit v1.2.3 From 986f3bc5a785998d9832bdcdf3e570040fd34185 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Wed, 14 Jun 2017 17:46:01 +0200 Subject: TableBuilder: Add TODOs from this morning Refs #3479 --- app/helpers/table_builder_helper.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index f4a3f9eb4..d1afcddf7 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -1,3 +1,5 @@ +# TODO: Add doc comment about neeeding to make a decorator for your collections +# TODO: Document global variables this uses module TableBuilderHelper class Column attr_reader :key, :name, :attribute, :sortable -- cgit v1.2.3 From b616626283a5e24d2b9996669c0978787229d9db Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Wed, 14 Jun 2017 17:50:00 +0200 Subject: TableBuilder: Extract custom action link methods to a new class Move `#action_links`, `#action_link_method`, and `#actions_after_policy_check` to a new class. They all depend on an action and/or a model object, so it made sense to group these together. Also, these should really be tested, and moved out of a private method context. They now live in `TableBuilderHelper::CustomLinks`. The advantage is that we now have these methods grouped together in a separate module that can be tested separately. Needed to change some things around now that they're in a class: * `obj` and `action` are now instance variables * In order to call Pundit's `policy` method, we have to call it directly on `Pundit`, since we're no longer in the context of a helper/controller/view. * Create a `UserContext` that can be passed to Pundit based on the one created in `ApplicationController`. * Rename some methods to make more sense in this new context. * Move `actions_to_http_methods` to a class constant, so we're not redefining it in every call to `method_for_action`. * Use `I18n.t` instead of `t` alone, otherwise getting the translation doesn't work. * Move `TableBuilderHelper#polymorphic_url_parts` to a new class `TableBuilderHelper::URL`. This enables us to use it in both `CustomLinks` and the `TableBuilderHelper`. We can't use it from `CustomLinks` if it's a public method in `TableBuilderHelper`, and we can't use it in `TableBuilderHelper#tbody` if it's a class method in `TableBuilderHelper`. There's a problem with the action symbols that aren't directly handled in `actions_after_policy_check`. The `:show` action was passed by the workbenches/show.html.slim template, but it doesn't appear in the resulting output even though it should. Refs #3479 --- app/helpers/table_builder_helper.rb | 92 ++++-------------------- app/helpers/table_builder_helper/custom_links.rb | 61 ++++++++++++++++ app/helpers/table_builder_helper/url.rb | 24 +++++++ 3 files changed, 98 insertions(+), 79 deletions(-) create mode 100644 app/helpers/table_builder_helper/custom_links.rb create mode 100644 app/helpers/table_builder_helper/url.rb diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index d1afcddf7..98c170b85 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -1,3 +1,6 @@ +require 'table_builder_helper/custom_links' +require 'table_builder_helper/url' + # TODO: Add doc comment about neeeding to make a decorator for your collections # TODO: Document global variables this uses module TableBuilderHelper @@ -100,7 +103,7 @@ module TableBuilderHelper if column_is_linkable?(column) # Build a link to the `item` - polymorph_url = polymorphic_url_parts(item) + polymorph_url = URL.polymorphic_url_parts(item) bcont << content_tag(:td, link_to(value, polymorph_url), title: 'Voir') else bcont << content_tag(:td, value) @@ -122,12 +125,20 @@ module TableBuilderHelper end def build_links(item, links) + user_context = UserContext.new( + current_user, + referential: self.try(:current_referential) + ) + trigger = content_tag :div, class: 'btn dropdown-toggle', data: { toggle: 'dropdown' } do content_tag :span, '', class: 'fa fa-cog' end menu = content_tag :ul, class: 'dropdown-menu' do - (action_links(links, item) + item.action_links).map do |link| + ( + CustomLinks.new(item, user_context, links).links + + item.action_links + ).map do |link| content_tag :li, link_to( link.name, link.href, @@ -248,81 +259,4 @@ module TableBuilderHelper def column_is_linkable?(column) column.attribute == 'name' || column.attribute == 'comment' end - - def polymorphic_url_parts(item) - polymorph_url = [] - - unless item.is_a?(Calendar) || item.is_a?(Referential) - if current_referential - polymorph_url << current_referential - polymorph_url << item.line if item.respond_to? :line - polymorph_url << item.route.line if item.is_a?(Chouette::RoutingConstraintZone) - polymorph_url << item if item.respond_to? :line_referential - polymorph_url << item.stop_area if item.respond_to? :stop_area - polymorph_url << item if item.respond_to? :stop_points || item.is_a?(Chouette::TimeTable) - elsif item.respond_to? :referential - polymorph_url << item.referential - end - else - polymorph_url << item - end - - polymorph_url - end - - # TODO: rename - def action_links(actions, obj) - actions_after_policy_check(actions, obj).map do |action| - # TODO: Move that s to another method - polymorph_url = [] - - unless [:show, :delete].include? action - polymorph_url << action - end - - polymorph_url += polymorphic_url_parts(obj) - - Link.new( - name: t("actions.#{action}"), - href: polymorph_url, - method: action_link_method(action) - ) - end - end - - def action_link_method(action) - actions_to_http_methods = { - delete: :delete, - archive: :put, - unarchive: :put - } - - actions_to_http_methods[action] || :get - end - - def actions_after_policy_check(actions, obj) - actions.select do |action| - # if action == :delete - # if policy(item).present? && policy(item).destroy? - # action - # end - # elsif action == :edit - # if policy(item).present? && policy(item).update? - # action - # end - # elsif action == :edit - # - # end - # if (action == :delete && policy(item).present? && policy(item).destroy?) || - (action == :delete && policy(obj).present? && policy(obj).destroy?) || - (action == :delete && !policy(obj).present?) || - (action == :edit && policy(obj).present? && policy(obj).update?) || - (action == :edit && !policy(obj).present?) || - (action == :archive && !obj.archived?) || - (action == :unarchive && obj.archived?) || - ([:delete, :edit, :archive, :unarchive].include?(action)) - # action - # end - end - end end diff --git a/app/helpers/table_builder_helper/custom_links.rb b/app/helpers/table_builder_helper/custom_links.rb new file mode 100644 index 000000000..9703940d6 --- /dev/null +++ b/app/helpers/table_builder_helper/custom_links.rb @@ -0,0 +1,61 @@ +require 'table_builder_helper/url' + +module TableBuilderHelper + class CustomLinks + ACTIONS_TO_HTTP_METHODS = { + delete: :delete, + archive: :put, + unarchive: :put + } + + def initialize(obj, user_context, actions) + @obj = obj + @user_context = user_context + @actions = actions + end + + def links + actions_after_policy_check.map do |action| + Link.new( + name: I18n.t("actions.#{action}"), + href: polymorphic_url(action), + method: method_for_action(action) + ) + end + end + + def polymorphic_url(action) + polymorph_url = [] + + unless [:show, :delete].include?(action) + polymorph_url << action + end + + polymorph_url += URL.polymorphic_url_parts(@obj) + end + + def method_for_action(action) + ACTIONS_TO_HTTP_METHODS[action] || :get + end + + def actions_after_policy_check + @actions.select do |action| + (action == :delete && + Pundit.policy(@user_context, @obj).present? && + Pundit.policy(@user_context, @obj).destroy?) || + (action == :delete && + !Pundit.policy(@user_context, @obj).present?) || + (action == :edit && + Pundit.policy(@user_context, @obj).present? && + Pundit.policy(@user_context, @obj).update?) || + (action == :edit && + !Pundit.policy(@user_context, @obj).present?) || + (action == :archive && !@obj.archived?) || + (action == :unarchive && @obj.archived?) || + + # TODO: Something wrong here for actions not handled + ([:delete, :edit, :archive, :unarchive].include?(action)) + end + end + end +end diff --git a/app/helpers/table_builder_helper/url.rb b/app/helpers/table_builder_helper/url.rb new file mode 100644 index 000000000..59099ee99 --- /dev/null +++ b/app/helpers/table_builder_helper/url.rb @@ -0,0 +1,24 @@ +module TableBuilderHelper + class URL + def self.polymorphic_url_parts(item) + polymorph_url = [] + + unless item.is_a?(Calendar) || item.is_a?(Referential) + if current_referential + polymorph_url << current_referential + polymorph_url << item.line if item.respond_to? :line + polymorph_url << item.route.line if item.is_a?(Chouette::RoutingConstraintZone) + polymorph_url << item if item.respond_to? :line_referential + polymorph_url << item.stop_area if item.respond_to? :stop_area + polymorph_url << item if item.respond_to? :stop_points || item.is_a?(Chouette::TimeTable) + elsif item.respond_to? :referential + polymorph_url << item.referential + end + else + polymorph_url << item + end + + polymorph_url + end + end +end -- cgit v1.2.3 From 07c50e21b45b8fcf219410cbfb71f1c1e67e81b6 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Wed, 14 Jun 2017 18:04:37 +0200 Subject: TableBuilderHelper#build_links: Use existing `pundit_user` method Instead of creating our own `user_context`, reuse the existing `ApplicationController#pundit_user` method to get the context. When I defined `user_context` I had done a project-find, found the one place where we created a `UserContext`, and duplicated the code in my method. Turns out after looking at the full `ApplicationController` file, there's a handy method already there for us to use. Refs #3479 --- app/helpers/table_builder_helper.rb | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 98c170b85..361fe1aae 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -125,18 +125,13 @@ module TableBuilderHelper end def build_links(item, links) - user_context = UserContext.new( - current_user, - referential: self.try(:current_referential) - ) - trigger = content_tag :div, class: 'btn dropdown-toggle', data: { toggle: 'dropdown' } do content_tag :span, '', class: 'fa fa-cog' end menu = content_tag :ul, class: 'dropdown-menu' do ( - CustomLinks.new(item, user_context, links).links + + CustomLinks.new(item, pundit_user, links).links + item.action_links ).map do |link| content_tag :li, link_to( -- cgit v1.2.3 From d2b1ab114348ee0895703cd650734f585b11e781 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Wed, 14 Jun 2017 18:24:17 +0200 Subject: TableBuilderHelper::CustomLinks: Fix policy check for unhandled actions In `#actions_after_policy_check`, we weren't correctly including actions that weren't handled by the prior policy checks. Thus actions that weren't [:delete, :edit, :archive, :unarchive] wouldn't get included in the resulting list of actions. Fix my logic error. Refs #3479 --- app/helpers/table_builder_helper/custom_links.rb | 4 +--- .../table_builder_helper/custom_links_spec.rb | 27 ++++++++++++++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-) create mode 100644 spec/helpers/table_builder_helper/custom_links_spec.rb diff --git a/app/helpers/table_builder_helper/custom_links.rb b/app/helpers/table_builder_helper/custom_links.rb index 9703940d6..f47ef1a11 100644 --- a/app/helpers/table_builder_helper/custom_links.rb +++ b/app/helpers/table_builder_helper/custom_links.rb @@ -52,9 +52,7 @@ module TableBuilderHelper !Pundit.policy(@user_context, @obj).present?) || (action == :archive && !@obj.archived?) || (action == :unarchive && @obj.archived?) || - - # TODO: Something wrong here for actions not handled - ([:delete, :edit, :archive, :unarchive].include?(action)) + (![:delete, :edit, :archive, :unarchive].include?(action)) end end end diff --git a/spec/helpers/table_builder_helper/custom_links_spec.rb b/spec/helpers/table_builder_helper/custom_links_spec.rb new file mode 100644 index 000000000..b64e97527 --- /dev/null +++ b/spec/helpers/table_builder_helper/custom_links_spec.rb @@ -0,0 +1,27 @@ +require 'spec_helper' + +describe TableBuilderHelper::CustomLinks do + describe "#actions_after_policy_check" do + it "includes :show" do + referential = build_stubbed(:referential) + user_context = UserContext.new( + build_stubbed( + :user, + organisation: referential.organisation, + permissions: [ + 'boiv:read-offer' + ] + ), + referential: referential + ) + + expect( + TableBuilderHelper::CustomLinks.new( + referential, + user_context, + [:show] + ).actions_after_policy_check + ).to eq([:show]) + end + end +end -- cgit v1.2.3 From 5fe14f119953f3eaf5a948ea67e9070ba3d3aac0 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Wed, 14 Jun 2017 18:30:45 +0200 Subject: TableBuilderHelper: Move ::Column to its own file Now that we have other classes that belong to `TableBuilderHelper` (`URL` and `CustomLinks`), it makes more sense for this one to live in its own file. Refs #3479 --- app/helpers/table_builder_helper.rb | 27 +-------------------------- app/helpers/table_builder_helper/column.rb | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 26 deletions(-) create mode 100644 app/helpers/table_builder_helper/column.rb diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 361fe1aae..73c994100 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -1,35 +1,10 @@ +require 'table_builder_helper/column' require 'table_builder_helper/custom_links' require 'table_builder_helper/url' # TODO: Add doc comment about neeeding to make a decorator for your collections # TODO: Document global variables this uses module TableBuilderHelper - class Column - attr_reader :key, :name, :attribute, :sortable - - def initialize(key: nil, name: '', attribute:, sortable: true) - if key.nil? && name.empty? - raise ColumnMustHaveKeyOrNameError - end - - @key = key - @name = name - @attribute = attribute - @sortable = sortable - end - - def value(obj) - if @attribute.is_a?(Proc) - @attribute.call(obj) - else - obj.try(@attribute) - end - end - end - - class ColumnMustHaveKeyOrNameError < StandardError; end - - # TODO: rename this after migration from `table_builder` def table_builder_2( collection, diff --git a/app/helpers/table_builder_helper/column.rb b/app/helpers/table_builder_helper/column.rb new file mode 100644 index 000000000..54e63104d --- /dev/null +++ b/app/helpers/table_builder_helper/column.rb @@ -0,0 +1,27 @@ +module TableBuilderHelper + class Column + attr_reader :key, :name, :attribute, :sortable + + def initialize(key: nil, name: '', attribute:, sortable: true) + if key.nil? && name.empty? + raise ColumnMustHaveKeyOrNameError + end + + @key = key + @name = name + @attribute = attribute + @sortable = sortable + end + + def value(obj) + if @attribute.is_a?(Proc) + @attribute.call(obj) + else + obj.try(@attribute) + end + end + end + + + class ColumnMustHaveKeyOrNameError < StandardError; end +end -- cgit v1.2.3 From 8c9eb5e1108f4d7e182c9984d667ed7cf4adc006 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Wed, 14 Jun 2017 18:35:41 +0200 Subject: workbenches/show.html.slim: Remove duplicate links in `table_builder` Now that we have the [:archive, :unarchive, :delete] links present in `ReferentialDecorator#action_links`, we don't need to include them here. Otherwise the links are duplicated between the custom links defined here and those that come from the `ReferentialDecorator`. Refs #3479 --- app/views/workbenches/show.html.slim | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/workbenches/show.html.slim b/app/views/workbenches/show.html.slim index 3c2bb26ec..6dec68f7a 100644 --- a/app/views/workbenches/show.html.slim +++ b/app/views/workbenches/show.html.slim @@ -58,7 +58,7 @@ ) \ ], selectable: true, - links: [:show, :edit, :archive, :unarchive, :delete], + links: [:show, :edit], cls: 'table has-filter has-search' / [:delete], -- cgit v1.2.3 From 2e124ae18c975341a3afd49e29eb245ac40c1220 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Wed, 14 Jun 2017 18:54:00 +0200 Subject: CustomLinks#actions_after_policy_check: Better document conditions Add comments that describe what each `or` condition is doing. At first I thought maybe I should create separate methods to name these conditions, but settled for comments instead. Moved the final check to a method because that one seemed the most obscure about what it does. Refs #3479 --- app/helpers/table_builder_helper/custom_links.rb | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/app/helpers/table_builder_helper/custom_links.rb b/app/helpers/table_builder_helper/custom_links.rb index f47ef1a11..a4a8bba4f 100644 --- a/app/helpers/table_builder_helper/custom_links.rb +++ b/app/helpers/table_builder_helper/custom_links.rb @@ -40,20 +40,38 @@ module TableBuilderHelper def actions_after_policy_check @actions.select do |action| + # Has policy and can destroy (action == :delete && Pundit.policy(@user_context, @obj).present? && Pundit.policy(@user_context, @obj).destroy?) || + + # Doesn't have policy (action == :delete && !Pundit.policy(@user_context, @obj).present?) || + + # Has policy and can update (action == :edit && Pundit.policy(@user_context, @obj).present? && Pundit.policy(@user_context, @obj).update?) || + + # Doesn't have policy (action == :edit && !Pundit.policy(@user_context, @obj).present?) || + + # Object isn't archived (action == :archive && !@obj.archived?) || + + # Object is archived (action == :unarchive && @obj.archived?) || - (![:delete, :edit, :archive, :unarchive].include?(action)) + + action_is_allowed_regardless_of_policy(action) end end + + private + + def action_is_allowed_regardless_of_policy(action) + ![:delete, :edit, :archive, :unarchive].include?(action) + end end end -- cgit v1.2.3 From a7ead83aba3a3987feaab7ac8b4089a2caa324d3 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Wed, 14 Jun 2017 18:58:13 +0200 Subject: Move ModelDecorator to a new file; Remove duplicate PaginatingDecorator Move `ModelDecorator` to a new file now that we know it works and that we're going to use it. I had previously put these two classes in the same file as `ReferentialDecorator` because I wanted to test that it worked before making the code correct. Also remove the `PaginatingDecorator` because apparently we already have one, which I didn't realise. Use the existing one instead. Refs #3479 --- app/decorators/model_decorator.rb | 3 +++ app/decorators/referential_decorator.rb | 17 ----------------- 2 files changed, 3 insertions(+), 17 deletions(-) create mode 100644 app/decorators/model_decorator.rb diff --git a/app/decorators/model_decorator.rb b/app/decorators/model_decorator.rb new file mode 100644 index 000000000..dee014cc3 --- /dev/null +++ b/app/decorators/model_decorator.rb @@ -0,0 +1,3 @@ +class ModelDecorator < PaginatingDecorator + delegate :model +end diff --git a/app/decorators/referential_decorator.rb b/app/decorators/referential_decorator.rb index 45a4b38d1..1c2347529 100644 --- a/app/decorators/referential_decorator.rb +++ b/app/decorators/referential_decorator.rb @@ -1,20 +1,3 @@ -# Delegates 'will_paginate' methods -class PaginatingDecorator < Draper::CollectionDecorator - delegate( - :current_page, - :per_page, - :offset, - :total_entries, - :total_pages - ) -end - - -class ModelDecorator < PaginatingDecorator - delegate :model -end - - class ReferentialDecorator < Draper::Decorator delegate_all -- cgit v1.2.3 From d7ab3b3c16a96092fa777e0c90d727a9bc5b0812 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Wed, 14 Jun 2017 19:00:42 +0200 Subject: ReferentialDecorator: Add TODO Refs #3479 --- app/decorators/referential_decorator.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/decorators/referential_decorator.rb b/app/decorators/referential_decorator.rb index 1c2347529..5112f9dd4 100644 --- a/app/decorators/referential_decorator.rb +++ b/app/decorators/referential_decorator.rb @@ -17,6 +17,7 @@ class ReferentialDecorator < Draper::Decorator end if h.policy(object).edit? + # TODO: Handle buttons in the header and don't show them in the gear menu # button.btn.btn-primary type='button' data-toggle='modal' data-target='#purgeModal' Purger if object.archived? -- cgit v1.2.3 From fc74a500d9c672d855d199eee715efd398058527 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Wed, 14 Jun 2017 19:03:02 +0200 Subject: WorkbenchesController: Remove `referential_decorator` import This is no longer necessary. I had put it in because I was getting an error, but it turns out that error was probably a result of me redefining `PaginatingDecorator` inside "referential_decorator.rb". Now that we're using the `PaginatingDecorator` that Jean-Paul created originally, we no longer have a problem. Refs #3479 --- app/controllers/workbenches_controller.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/controllers/workbenches_controller.rb b/app/controllers/workbenches_controller.rb index 56f9136b3..1447c27de 100644 --- a/app/controllers/workbenches_controller.rb +++ b/app/controllers/workbenches_controller.rb @@ -1,6 +1,3 @@ -# TODO: Try not importing -require 'referential_decorator' - class WorkbenchesController < BreadcrumbController before_action :query_params, only: [:show] -- cgit v1.2.3 From 37ad0d2f30f8e16207b6123d9084b8f2cec3b592 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Wed, 14 Jun 2017 19:08:58 +0200 Subject: TableBuilderHelper: Add a couple TODOs Refs #3479 --- app/helpers/table_builder_helper.rb | 2 ++ spec/helpers/table_builder_helper_spec.rb | 1 + 2 files changed, 3 insertions(+) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 73c994100..34f610975 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -109,6 +109,8 @@ module TableBuilderHelper CustomLinks.new(item, pundit_user, links).links + item.action_links ).map do |link| + # TODO: ensure the Delete link is formatted correctly with the spacer, + # icon, and label content_tag :li, link_to( link.name, link.href, diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index 756fac861..7e2d617a3 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -87,6 +87,7 @@ describe TableBuilderHelper, type: :helper do HTML +# TODO: Create a module for the selection box #
#
-- cgit v1.2.3 From 690428d470440c604dcac52549baca8887fcd4c3 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 15 Jun 2017 16:31:22 +0200 Subject: TableBuilder spec: Use `UserContext` Instead of building our context with an `OpenStruct`, use the `UserContext` class that actually exists and represents the thing that we want to represent. I had copied the `OpenStruct` way of doing from "spec/support/pundit/policies.rb" this before I found out that `UserContext` existed. Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index 764e102fb..186f47413 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -11,12 +11,8 @@ describe TableBuilderHelper, type: :helper do referential = build_stubbed(:referential) workbench = referential.workbench - # user_context = create_user_context( - # user: build_stubbed(:user), - # referential: referential - # ) - user_context = OpenStruct.new( - user: build_stubbed( + user_context = UserContext.new( + build_stubbed( :user, organisation: referential.organisation, permissions: [ @@ -25,7 +21,7 @@ describe TableBuilderHelper, type: :helper do 'referentials.destroy' ] ), - context: { referential: referential } + referential: referential ) allow(helper).to receive(:current_user).and_return(user_context) @@ -174,8 +170,8 @@ describe TableBuilderHelper, type: :helper do line_referential: line_referential ) - user_context = OpenStruct.new( - user: build_stubbed( + user_context = UserContext.new( + build_stubbed( :user, organisation: referential.organisation, permissions: [ @@ -184,7 +180,7 @@ describe TableBuilderHelper, type: :helper do 'referentials.destroy' ] ), - context: { referential: referential } + referential: referential ) allow(helper).to receive(:current_user).and_return(user_context) allow(TableBuilderHelper::URL).to receive(:current_referential) @@ -282,8 +278,8 @@ describe TableBuilderHelper, type: :helper do line_referential: line_referential ) - user_context = OpenStruct.new( - user: build_stubbed( + user_context = UserContext.new( + build_stubbed( :user, organisation: referential.organisation, permissions: [ @@ -292,7 +288,7 @@ describe TableBuilderHelper, type: :helper do 'referentials.destroy' ] ), - context: { referential: referential } + referential: referential ) allow(helper).to receive(:current_user).and_return(user_context) allow(TableBuilderHelper::URL).to receive(:current_referential) -- cgit v1.2.3 From b011960b67c7cadecb8d948934f13071d9ae4df1 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 15 Jun 2017 16:37:33 +0200 Subject: spec/Support::Pundit::Policies: Use UserContext instead of OpenStruct There's a `UserContext` class that allows us to build user contexts. We should use the full type for this instead of creating an anonymous one for the context. --- spec/support/pundit/policies.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/support/pundit/policies.rb b/spec/support/pundit/policies.rb index e18309226..56433b2ee 100644 --- a/spec/support/pundit/policies.rb +++ b/spec/support/pundit/policies.rb @@ -9,7 +9,7 @@ module Support end def create_user_context(user:, referential:) - OpenStruct.new(user: user, context: {referential: referential}) + UserContext.new(user, referential: referential) end def add_permissions(*permissions, for_user:) -- cgit v1.2.3 From 73f8e3bb8a5e6ac47874ce75ed3bf81c63a633dd Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 15 Jun 2017 16:42:42 +0200 Subject: TableBuilderHelper#table_builder_2: Clean up arguments & add docs Remove unnecessary arguments and document the ones we are using. `current_referential` has been removed. Ultimately decided that it was simpler to just rely on the existing method that's magically "imported" instead of passing it directly. If you vote to pass it in as an arg, though, I'm all for it. Refs #3479 --- app/helpers/table_builder_helper.rb | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index 01e2e6fad..111237a7b 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -9,22 +9,19 @@ module TableBuilderHelper def table_builder_2( collection, columns, - current_referential: nil, # When false, no columns will be sortable sortable: true, + # When true, adds a column of checkboxes to the left side of the table selectable: false, - # selection_actions: [] ## this has been gotten rid of. The element based on this should be created elsewhere - links: [], # links: or actions: ? I think 'links' is better since 'actions' evokes Rails controller actions and we want to put `link_to`s here - # TODO: get rid of this argument. Going with params instead - sort_by: {}, # { column: 'name', direction: 'desc' } - cls: '' # can we rename this to "class"? - # ^^ rename to html_options = {} at the end of the non-keyword arguments? Hrm, not a fan of combining hash args and keyword args -# sort column -# sort direction - -# TODO: add `linked_column` or some such attribute that defines which column should be linked and what method to call to get it + + # A set of controller actions that will be added as links to the top of the + # gear menu + links: [], + + # A CSS class to apply to the + cls: '' ) content_tag :table, -- cgit v1.2.3 From 3aea68a27af7a96db470fba2ad9c1f6000a681b9 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 15 Jun 2017 16:48:26 +0200 Subject: TableBuilder spec: Remove TODOs These are all finished. Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index 186f47413..199373e24 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -376,15 +376,3 @@ describe TableBuilderHelper, type: :helper do end end end - - -# Replace table builder on workspaces#show page -# Make the builder work without a `current_referential` so we can actually test it -# Make a way to define a column as non-sortable. By default, columns will be sortable. Unless sortable==false and no columns should be sortable. -# -# -# TODO: -# - Finish writing workbench test -# - Port some code over to the new table builder -# - Ask Jean-Paul if there's anything he wishes could be changed or improved about the existing table builder -# - Thing that Jean-Paul didn't like was the link generation -- cgit v1.2.3 From 97c8cd58c1d3fd684f41135ac1774fac4cc46ea1 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 15 Jun 2017 16:50:37 +0200 Subject: NewapplicationHelper: Remove my investigation comments I added these comments when I was reading the code and trying to understand it. Now I basically understand all of it, and we've ported the code to `TableBuilderHelper`. These comments can be removed to revert the file back to how it looked before I messed with it. Refs #3479 --- app/helpers/newapplication_helper.rb | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/app/helpers/newapplication_helper.rb b/app/helpers/newapplication_helper.rb index 27b9cb0a4..3d43e9fc7 100644 --- a/app/helpers/newapplication_helper.rb +++ b/app/helpers/newapplication_helper.rb @@ -1,7 +1,6 @@ module NewapplicationHelper # Table Builder - # selectable means actions-for-selection def table_builder collection, columns, actions, selectable = [], cls = nil return unless collection.present? @@ -9,7 +8,6 @@ module NewapplicationHelper content_tag :tr do hcont = [] - # Adds checkbox to table header unless selectable.empty? cbx = content_tag :div, '', class: 'checkbox' do check_box_tag('0', 'all').concat(content_tag(:label, '', for: '0')) @@ -18,14 +16,12 @@ module NewapplicationHelper end columns.map do |k, v| - # These columns are hard-coded to not be sortable if ["ID Codif", "Oid", "OiD", "ID Reflex", "Arrêt de départ", "Arrêt d'arrivée", "Période de validité englobante", "Période englobante", "Nombre de courses associées", "Journées d'application", "Arrêts de l'itinéraire", "Arrêts inclus dans l'ITL"].include? k hcont << content_tag(:th, k) else hcont << content_tag(:th, sortable_columns(collection, k)) end end - # Inserts a blank column for the gear menu hcont << content_tag(:th, '') if actions.any? hcont.join.html_safe @@ -38,8 +34,6 @@ module NewapplicationHelper content_tag :tr do bcont = [] - # Adds item checkboxes whose value = the row object's id - # Apparently the object id is also used in the HTML id attribute without any prefix unless selectable.empty? cbx = content_tag :div, '', class: 'checkbox' do check_box_tag(item.try(:id), item.try(:id)).concat(content_tag(:label, '', for: item.try(:id))) @@ -54,14 +48,9 @@ module NewapplicationHelper else item.try(attribute) end - # if so this column's contents get transformed into a link to the object if attribute == 'name' or attribute == 'comment' lnk = [] - # #is_a? ? ; or ? - # TODO: Ask Jean-Paul: on which pages do we create multiple links? - # Do we actually create multiple links with this code? - # Answer: this is a polymorphic URL unless item.class == Calendar or item.class == Referential if current_referential lnk << current_referential @@ -183,7 +172,6 @@ module NewapplicationHelper pic2 = content_tag :span, '', class: "fa fa-sort-desc #{(direction == 'asc') ? 'active' : ''}" pics = content_tag :span, pic1 + pic2, class: 'orderers' - # This snake cases and downcases the class name. Should use the ActiveSupport method to do this obj = collection.model.to_s.gsub('Chouette::', '').scan(/[A-Z][a-z]+/).join('_').downcase (I18n.t("activerecord.attributes.#{obj}.#{key}") + pics).html_safe -- cgit v1.2.3 From 4a67dc49135dc154078181155640a94afbb928f5 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 15 Jun 2017 17:02:47 +0200 Subject: referential_lines/show.html.slim: Remove TODO I had added this as an idea for how to render these links both here, in the page header, and in the table gear menu of the preceding page. We've basically done that now with `TableBuilderHelper`, although in a slightly different way than I originally imagined, with decorators (like `ReferentialDecorator#action_links`). Refs #3479 --- app/views/referential_lines/show.html.slim | 1 - 1 file changed, 1 deletion(-) diff --git a/app/views/referential_lines/show.html.slim b/app/views/referential_lines/show.html.slim index 5fb1bb125..db99381d3 100644 --- a/app/views/referential_lines/show.html.slim +++ b/app/views/referential_lines/show.html.slim @@ -7,7 +7,6 @@ / Below is secundary actions & optional contents .row .col-lg-12.text-right.mb-sm - / TODO: Make a list of Link objects that can be rendered with link_tos here and in the gear menu = link_to @line.human_attribute_name(:footnotes), referential_line_footnotes_path(@referential, @line), class: 'btn btn-primary' = link_to t('routing_constraint_zones.index.title'), referential_line_routing_constraint_zones_path(@referential, @line), class: 'btn btn-primary' -- cgit v1.2.3 From 8e678905af6be3358069a9d51f7af88d3449f014 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 15 Jun 2017 17:04:58 +0200 Subject: referentials/show.html.slim: Update `action_links` loop for Link#content Now that the `Link` class no longer has a :name attribute, use the :content attribute instead and get our delete button markup handling from `ReferentialDecorator`. TODO: This messes up the styles on this page as here there's a `` around the link text and in the tables there's no span. Refs #3479 --- app/views/referentials/show.html.slim | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/app/views/referentials/show.html.slim b/app/views/referentials/show.html.slim index 1bbf350b4..f3dcf6a69 100644 --- a/app/views/referentials/show.html.slim +++ b/app/views/referentials/show.html.slim @@ -27,19 +27,11 @@ / span = t('actions.destroy') /= leslinks.map { |l| link_to l.href, l.label } - @referential.action_links.each do |link| - - if link.method == :delete - = link_to link.href, - method: link.method, - data: link.data, - class: 'btn btn-primary' do - span.fa.fa-trash - span = t('actions.destroy') - - else - = link_to link.name, - link.href, - method: link.method, - data: link.data, - class: 'btn btn-primary' + = link_to link.href, + method: link.method, + data: link.data, + class: 'btn btn-primary' do + = link.content / PageContent .page_content -- cgit v1.2.3 From 1d489cef3b9aeb474071b89be6d256e554182863 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 15 Jun 2017 17:44:55 +0200 Subject: ReferentialDecorator: Handle non-`Link` buttons Referentials#show has a button "Purger" that isn't a link. Instead, it's a `
`s. A column of checkboxes can be added to the left side of the table +# for multiple selection. Columns are sortable by default, but sorting can be +# disabled either at the table level or at the column level. An optional +# `links` argument takes a set of symbols corresponding to controller actions +# that should be inserted in a gear menu next to each row in the table. That +# menu will also be populated with links defined in `collection#action_links`, +# a list of `Link` objects defined in a decorator for the given object. +# +# Depends on `params` and `current_referential`. +# +# Example: +# table_builder_2( +# @companies, +# [ +# TableBuilderHelper::Column.new( +# name: 'ID Codif', +# attribute: Proc.new { |n| n.try(:objectid).try(:local_id) }, +# sortable: false +# ), +# TableBuilderHelper::Column.new( +# key: :name, +# attribute: 'name' +# ), +# TableBuilderHelper::Column.new( +# key: :phone, +# attribute: 'phone' +# ), +# TableBuilderHelper::Column.new( +# key: :email, +# attribute: 'email' +# ), +# TableBuilderHelper::Column.new( +# key: :url, +# attribute: 'url' +# ), +# ], +# links: [:show, :edit], +# cls: 'table has-search' +# ) module TableBuilderHelper # TODO: rename this after migration from `table_builder` def table_builder_2( + # An `ActiveRecord::Relation`, wrapped in a decorator to provide a list of + # `Link` objects via an `#action_links` method collection, + + # An array of `TableBuilderHelper::Column`s columns, # When false, no columns will be sortable diff --git a/app/helpers/table_builder_helper/url.rb b/app/helpers/table_builder_helper/url.rb index 59099ee99..f60864ac1 100644 --- a/app/helpers/table_builder_helper/url.rb +++ b/app/helpers/table_builder_helper/url.rb @@ -1,4 +1,5 @@ module TableBuilderHelper + # Depends on `current_referential`, defined in object controllers class URL def self.polymorphic_url_parts(item) polymorph_url = [] -- cgit v1.2.3 From a417f0a2f2813fbec7e3c925334433f6a724405d Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 11:27:52 +0200 Subject: TableBuilder spec: Fix hard-coded IDs in expected string Remove the hard-coded IDs and replace them with the test referential's ID. I guess I had copied the output from the spec output, because the test then passed when I ran it individually, causing me to not catch the error. Only saw it when I ran the full test suite. Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index 199373e24..a61fd2343 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -79,8 +79,8 @@ describe TableBuilderHelper, type: :helper do -- cgit v1.2.3 From ad21d8990a99d24b72b45ccc1d261ddc93179bfd Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 12:06:23 +0200 Subject: Add new #action_links decorators for a few priority models These models are being prioritised for changing their views to use the new TableBuilderHelper. We're giving them all action links, copied out of the following files: * time_tables/show.html.slim * calendars/show.html.slim * lines/show.html.slim * routing_constraint_zones/show.html.slim * routes/show.html.slim Created a new namespace in decorators/ for Chouette model decorators for consistency with the model/ directory. We'll want to move `CompanyDecorator` in there too because it lives in the `Chouette::` namespace. Refs #3479 --- app/decorators/calendar_decorator.rb | 18 +++++++++ app/decorators/chouette/line_decorator.rb | 44 ++++++++++++++++++++++ app/decorators/chouette/route_decorator.rb | 43 +++++++++++++++++++++ .../chouette/routing_constraint_zone_decorator.rb | 35 +++++++++++++++++ app/decorators/chouette/time_table_decorator.rb | 42 +++++++++++++++++++++ app/decorators/company_decorator.rb | 1 + 6 files changed, 183 insertions(+) create mode 100644 app/decorators/calendar_decorator.rb create mode 100644 app/decorators/chouette/line_decorator.rb create mode 100644 app/decorators/chouette/route_decorator.rb create mode 100644 app/decorators/chouette/routing_constraint_zone_decorator.rb create mode 100644 app/decorators/chouette/time_table_decorator.rb diff --git a/app/decorators/calendar_decorator.rb b/app/decorators/calendar_decorator.rb new file mode 100644 index 000000000..37e2cfe80 --- /dev/null +++ b/app/decorators/calendar_decorator.rb @@ -0,0 +1,18 @@ +class CalendarDecorator < Draper::Decorator + delegate_all + + def action_links + links = [] + + if h.policy(object).destroy? + links << Link.new( + content: h.destroy_link_content, + href: h.calendar_path(object), + method: :delete, + data: { confirm: h.t('calendars.actions.destroy_confirm') } + ) + end + + links + end +end diff --git a/app/decorators/chouette/line_decorator.rb b/app/decorators/chouette/line_decorator.rb new file mode 100644 index 000000000..30c093dee --- /dev/null +++ b/app/decorators/chouette/line_decorator.rb @@ -0,0 +1,44 @@ +# TODO: figure out @line_referential +class Chouette::LineDecorator < Draper::Decorator + delegate_all + + def action_links + links = [] + + links << Link.new( + content: h.t('lines.actions.show_network'), + href: [@line_referential, object.network] + ) + + links << Link.new( + content: h.t('lines.actions.show_company'), + href: [@line_referential, object.company] + ) + + if h.policy(Chouette::Line).create? && + @line_referential.organisations.include?(current_organisation) + links << Link.new( + content: h.t('lines.actions.new'), + href: h.new_line_referential_line_path(@line_referential) + ) + end + + # TODO: what if false? do we delete this? + if false && h.policy(object).update? + # = link_to t('lines.actions.edit'), edit_line_referential_line_path(@line_referential, object), class: 'btn btn-primary' + end + + if h.policy(object).destroy? + links << Link.new( +# TODO: this translation is different! +span = t('lines.actions.destroy') + content: h.destroy_link_content, + href: h.line_referential_line_path(@line_referential, object), + method: :delete, + data: { confirm: h.t('lines.actions.destroy_confirm') } + ) + end + + links + end +end diff --git a/app/decorators/chouette/route_decorator.rb b/app/decorators/chouette/route_decorator.rb new file mode 100644 index 000000000..d59179278 --- /dev/null +++ b/app/decorators/chouette/route_decorator.rb @@ -0,0 +1,43 @@ +# TODO: Figure out @referential, @line, @route_sp +class Chouette::RouteDecorator < Draper::Decorator + delegate_all + + def action_links + links = [] + + if @route_sp.any? + links << Link.new( + content: h.t('journey_patterns.index.title'), + href: [@referential, @line, object, :journey_patterns_collection] + ) + end + + if object.journey_patterns.present? + links << Link.new( + content: h.t('vehicle_journeys.actions.index'), + href: [@referential, @line, object, :vehicle_journeys] + ) + end + + links << Link.new( + content: h.t('vehicle_journey_exports.new.title'), + href: h.referential_line_route_vehicle_journey_exports_path( + @referential, + @line, + object, + format: :zip + ) + ) + + if h.policy(object).destroy? + links << Link.new( + content: h.destroy_link_content, + href: h.referential_line_route_path(@referential, @line, object), + method: :delete, + data: { confirm: h.t('routes.actions.destroy_confirm') } + ) + end + + links + end +end diff --git a/app/decorators/chouette/routing_constraint_zone_decorator.rb b/app/decorators/chouette/routing_constraint_zone_decorator.rb new file mode 100644 index 000000000..bcf278dda --- /dev/null +++ b/app/decorators/chouette/routing_constraint_zone_decorator.rb @@ -0,0 +1,35 @@ +# TODO: Figure out @referential, @line +class Chouette::RoutingConstraintZoneDecorator < Draper::Decorator + delegate_all + + def action_links + links = [] + + if h.policy(object).update? + links << Link.new( + content: h.t('actions.edit'), + href: h.edit_referential_line_routing_constraint_zone_path( + @referential, + @line, + object + ) + ) + + if h.policy(object).destroy? + links << Link.new( + content: h.destroy_link_content, + href: h.referential_line_routing_constraint_zone_path( + @referential, + @line, + object + ), + method: :delete, + data: { + confirm: h.t('routing_constraint_zones.actions.destroy_confirm') + } + ) + end + + links + end +end diff --git a/app/decorators/chouette/time_table_decorator.rb b/app/decorators/chouette/time_table_decorator.rb new file mode 100644 index 000000000..3d9994ea5 --- /dev/null +++ b/app/decorators/chouette/time_table_decorator.rb @@ -0,0 +1,42 @@ +# TODO: Add @referential to context +class Chouette::TimeTableDecorator < Draper::Decorator + delegate_all + + def action_links + links = [] + + if object.calendar + links << Link.new( + content: h.t('actions.actualize'), + href: h.actualize_referential_time_table_path(@referential, object), + method: :post + ) + end + + links << Link.new( + content: h.t('actions.combine'), + href: h.new_referential_time_table_time_table_combination_path( + @referential, + object + ) + ) + + if h.policy(object).duplicate? + links << Link.new( + content: h.t('actions.duplicate'), + href: h.duplicate_referential_time_table_path(@referential, object) + ) + end + + if h.policy(object).destroy? + Link.new( + content: h.destroy_link_content, + href: h.referential_time_table_path(@referential, object), + method: :delete, + data: { confirm: h.t('time_tables.actions.destroy_confirm') } + ) + end + + links + end +end diff --git a/app/decorators/company_decorator.rb b/app/decorators/company_decorator.rb index 629d1ff38..bd0441f43 100644 --- a/app/decorators/company_decorator.rb +++ b/app/decorators/company_decorator.rb @@ -1,3 +1,4 @@ +# TODO: Move this into the Chouette:: namespace class CompanyDecorator < Draper::Decorator decorates Chouette::Company -- cgit v1.2.3 From ce105d1ddb7f019cf8b55e41aa69c51e651c8ac9 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 12:22:39 +0200 Subject: New decorators: Move instance variables to `context` hash Draper allows us to pass a `context` hash when decorating objects. This gives us a way to bring in the instance variables required by a few of the new decorators I created just a moment ago. We can now safely get those values inside the decorators. Refs #3479 --- app/decorators/chouette/line_decorator.rb | 17 ++++++----- app/decorators/chouette/route_decorator.rb | 33 +++++++++++++++++----- .../chouette/routing_constraint_zone_decorator.rb | 14 +++++---- app/decorators/chouette/time_table_decorator.rb | 19 +++++++++---- app/decorators/company_decorator.rb | 12 ++++++-- 5 files changed, 68 insertions(+), 27 deletions(-) diff --git a/app/decorators/chouette/line_decorator.rb b/app/decorators/chouette/line_decorator.rb index 30c093dee..e32acb13b 100644 --- a/app/decorators/chouette/line_decorator.rb +++ b/app/decorators/chouette/line_decorator.rb @@ -1,31 +1,34 @@ -# TODO: figure out @line_referential class Chouette::LineDecorator < Draper::Decorator delegate_all + # Requires: + # context: { + # line_referential: + # } def action_links links = [] links << Link.new( content: h.t('lines.actions.show_network'), - href: [@line_referential, object.network] + href: [context[:line_referential], object.network] ) links << Link.new( content: h.t('lines.actions.show_company'), - href: [@line_referential, object.company] + href: [context[:line_referential], object.company] ) if h.policy(Chouette::Line).create? && - @line_referential.organisations.include?(current_organisation) + context[:line_referential].organisations.include?(current_organisation) links << Link.new( content: h.t('lines.actions.new'), - href: h.new_line_referential_line_path(@line_referential) + href: h.new_line_referential_line_path(context[:line_referential]) ) end # TODO: what if false? do we delete this? if false && h.policy(object).update? - # = link_to t('lines.actions.edit'), edit_line_referential_line_path(@line_referential, object), class: 'btn btn-primary' + # = link_to t('lines.actions.edit'), edit_line_referential_line_path(context[:line_referential], object), class: 'btn btn-primary' end if h.policy(object).destroy? @@ -33,7 +36,7 @@ class Chouette::LineDecorator < Draper::Decorator # TODO: this translation is different! span = t('lines.actions.destroy') content: h.destroy_link_content, - href: h.line_referential_line_path(@line_referential, object), + href: h.line_referential_line_path(context[:line_referential], object), method: :delete, data: { confirm: h.t('lines.actions.destroy_confirm') } ) diff --git a/app/decorators/chouette/route_decorator.rb b/app/decorators/chouette/route_decorator.rb index d59179278..f98fbfcb1 100644 --- a/app/decorators/chouette/route_decorator.rb +++ b/app/decorators/chouette/route_decorator.rb @@ -1,29 +1,44 @@ -# TODO: Figure out @referential, @line, @route_sp class Chouette::RouteDecorator < Draper::Decorator delegate_all + # Requires: + # context: { + # referential: , + # line: , + # route_sp + # } def action_links links = [] - if @route_sp.any? + if context[:route_sp].any? links << Link.new( content: h.t('journey_patterns.index.title'), - href: [@referential, @line, object, :journey_patterns_collection] + href: [ + context[:referential], + context[:line], + object, + :journey_patterns_collection + ] ) end if object.journey_patterns.present? links << Link.new( content: h.t('vehicle_journeys.actions.index'), - href: [@referential, @line, object, :vehicle_journeys] + href: [ + context[:referential], + context[:line], + object, + :vehicle_journeys + ] ) end links << Link.new( content: h.t('vehicle_journey_exports.new.title'), href: h.referential_line_route_vehicle_journey_exports_path( - @referential, - @line, + context[:referential], + context[:line], object, format: :zip ) @@ -32,7 +47,11 @@ class Chouette::RouteDecorator < Draper::Decorator if h.policy(object).destroy? links << Link.new( content: h.destroy_link_content, - href: h.referential_line_route_path(@referential, @line, object), + href: h.referential_line_route_path( + context[:referential], + context[:line], + object + ), method: :delete, data: { confirm: h.t('routes.actions.destroy_confirm') } ) diff --git a/app/decorators/chouette/routing_constraint_zone_decorator.rb b/app/decorators/chouette/routing_constraint_zone_decorator.rb index bcf278dda..f95206064 100644 --- a/app/decorators/chouette/routing_constraint_zone_decorator.rb +++ b/app/decorators/chouette/routing_constraint_zone_decorator.rb @@ -1,7 +1,11 @@ -# TODO: Figure out @referential, @line class Chouette::RoutingConstraintZoneDecorator < Draper::Decorator delegate_all + # Requires: + # context: { + # referential: , + # line: + # } def action_links links = [] @@ -9,8 +13,8 @@ class Chouette::RoutingConstraintZoneDecorator < Draper::Decorator links << Link.new( content: h.t('actions.edit'), href: h.edit_referential_line_routing_constraint_zone_path( - @referential, - @line, + context[:referential], + context[:line], object ) ) @@ -19,8 +23,8 @@ class Chouette::RoutingConstraintZoneDecorator < Draper::Decorator links << Link.new( content: h.destroy_link_content, href: h.referential_line_routing_constraint_zone_path( - @referential, - @line, + context[:referential], + context[:line], object ), method: :delete, diff --git a/app/decorators/chouette/time_table_decorator.rb b/app/decorators/chouette/time_table_decorator.rb index 3d9994ea5..7f2dbf5f1 100644 --- a/app/decorators/chouette/time_table_decorator.rb +++ b/app/decorators/chouette/time_table_decorator.rb @@ -1,14 +1,20 @@ -# TODO: Add @referential to context class Chouette::TimeTableDecorator < Draper::Decorator delegate_all + # Requires: + # context: { + # referential: , + # } def action_links links = [] if object.calendar links << Link.new( content: h.t('actions.actualize'), - href: h.actualize_referential_time_table_path(@referential, object), + href: h.actualize_referential_time_table_path( + context[:referential], + object + ), method: :post ) end @@ -16,7 +22,7 @@ class Chouette::TimeTableDecorator < Draper::Decorator links << Link.new( content: h.t('actions.combine'), href: h.new_referential_time_table_time_table_combination_path( - @referential, + context[:referential], object ) ) @@ -24,14 +30,17 @@ class Chouette::TimeTableDecorator < Draper::Decorator if h.policy(object).duplicate? links << Link.new( content: h.t('actions.duplicate'), - href: h.duplicate_referential_time_table_path(@referential, object) + href: h.duplicate_referential_time_table_path( + context[:referential], + object + ) ) end if h.policy(object).destroy? Link.new( content: h.destroy_link_content, - href: h.referential_time_table_path(@referential, object), + href: h.referential_time_table_path(context[:referential], object), method: :delete, data: { confirm: h.t('time_tables.actions.destroy_confirm') } ) diff --git a/app/decorators/company_decorator.rb b/app/decorators/company_decorator.rb index bd0441f43..3bd85319c 100644 --- a/app/decorators/company_decorator.rb +++ b/app/decorators/company_decorator.rb @@ -18,21 +18,27 @@ class CompanyDecorator < Draper::Decorator if h.policy(Chouette::Company).create? links << Link.new( content: h.t('companies.actions.new'), - href: h.new_line_referential_company_path(@line_referential) + href: h.new_line_referential_company_path(context[:line_referential]) ) end if h.policy(object).update? links << Link.new( content: h.t('companies.actions.edit'), - href: h.edit_line_referential_company_path(@line_referential, object) + href: h.edit_line_referential_company_path( + context[:line_referential], + object + ) ) end if h.policy(object).destroy? links << Link.new( content: t('companies.actions.destroy'), - href: h.line_referential_company_path(@line_referential, object), + href: h.line_referential_company_path( + context[:line_referential], + object + ), method: :delete, data: { confirm: t('companies.actions.destroy_confirm') } ) -- cgit v1.2.3 From 84ebcffb06d9ba9a56cc29d1739f1f950649f82e Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 15:31:19 +0200 Subject: Routes#show: Use RouteDecorator#action_links to render header links Instead of defining the header links in the view, get them from our decorator so we can reuse them. Need to wrap `@route` in the decorator in the controller and pass it the necessary context in order for it to be able to properly build links. Move `Chouette::RouteDecorator` to the top level app/decorators/ directory and remove it from the `Chouette` namespace. I had put it there to make it more similar to our model directory layout, but Draper isn't intelligent enough to look in subdirectories of `decorators` when using the `ActiveRecord::Base#decorate` method. Otherwise I suppose I could do `Chouette::RouteDecorator.decorate(@route)`, but that seemed not as clean since it doesn't take advantage of the default behaviour of Draper. Now going to have to do this for the other decorators. Refs #3479 --- app/controllers/routes_controller.rb | 6 +++ app/decorators/chouette/route_decorator.rb | 62 ----------------------------- app/decorators/route_decorator.rb | 64 ++++++++++++++++++++++++++++++ app/views/routes/show.html.slim | 17 +++----- 4 files changed, 76 insertions(+), 73 deletions(-) delete mode 100644 app/decorators/chouette/route_decorator.rb create mode 100644 app/decorators/route_decorator.rb diff --git a/app/controllers/routes_controller.rb b/app/controllers/routes_controller.rb index 73febc4b9..786bd57cc 100644 --- a/app/controllers/routes_controller.rb +++ b/app/controllers/routes_controller.rb @@ -42,6 +42,12 @@ class RoutesController < ChouetteController end show! do + @route = @route.decorate(context: { + referential: @referential, + line: @line, + route_sp: @route_sp + }) + build_breadcrumb :show end end diff --git a/app/decorators/chouette/route_decorator.rb b/app/decorators/chouette/route_decorator.rb deleted file mode 100644 index f98fbfcb1..000000000 --- a/app/decorators/chouette/route_decorator.rb +++ /dev/null @@ -1,62 +0,0 @@ -class Chouette::RouteDecorator < Draper::Decorator - delegate_all - - # Requires: - # context: { - # referential: , - # line: , - # route_sp - # } - def action_links - links = [] - - if context[:route_sp].any? - links << Link.new( - content: h.t('journey_patterns.index.title'), - href: [ - context[:referential], - context[:line], - object, - :journey_patterns_collection - ] - ) - end - - if object.journey_patterns.present? - links << Link.new( - content: h.t('vehicle_journeys.actions.index'), - href: [ - context[:referential], - context[:line], - object, - :vehicle_journeys - ] - ) - end - - links << Link.new( - content: h.t('vehicle_journey_exports.new.title'), - href: h.referential_line_route_vehicle_journey_exports_path( - context[:referential], - context[:line], - object, - format: :zip - ) - ) - - if h.policy(object).destroy? - links << Link.new( - content: h.destroy_link_content, - href: h.referential_line_route_path( - context[:referential], - context[:line], - object - ), - method: :delete, - data: { confirm: h.t('routes.actions.destroy_confirm') } - ) - end - - links - end -end diff --git a/app/decorators/route_decorator.rb b/app/decorators/route_decorator.rb new file mode 100644 index 000000000..99b174dff --- /dev/null +++ b/app/decorators/route_decorator.rb @@ -0,0 +1,64 @@ +class RouteDecorator < Draper::Decorator + decorates Chouette::Route + + delegate_all + + # Requires: + # context: { + # referential: , + # line: , + # route_sp + # } + def action_links + links = [] + + if context[:route_sp].any? + links << Link.new( + content: h.t('journey_patterns.index.title'), + href: [ + context[:referential], + context[:line], + object, + :journey_patterns_collection + ] + ) + end + + if object.journey_patterns.present? + links << Link.new( + content: h.t('vehicle_journeys.actions.index'), + href: [ + context[:referential], + context[:line], + object, + :vehicle_journeys + ] + ) + end + + links << Link.new( + content: h.t('vehicle_journey_exports.new.title'), + href: h.referential_line_route_vehicle_journey_exports_path( + context[:referential], + context[:line], + object, + format: :zip + ) + ) + + if h.policy(object).destroy? + links << Link.new( + content: h.destroy_link_content, + href: h.referential_line_route_path( + context[:referential], + context[:line], + object + ), + method: :delete, + data: { confirm: h.t('routes.actions.destroy_confirm') } + ) + end + + links + end +end diff --git a/app/views/routes/show.html.slim b/app/views/routes/show.html.slim index 6d2d4e90d..92a5080ae 100644 --- a/app/views/routes/show.html.slim +++ b/app/views/routes/show.html.slim @@ -8,17 +8,12 @@ / Below is secundary actions & optional contents (filters, ...) .row.mb-sm .col-lg-12.text-right - - if @route_sp.any? - = link_to t('journey_patterns.index.title'), [@referential, @line, @route, :journey_patterns_collection], class: 'btn btn-primary' - - if @route.journey_patterns.present? - = link_to t('vehicle_journeys.actions.index'), [@referential, @line, @route, :vehicle_journeys], class: 'btn btn-primary' - - = link_to t('vehicle_journey_exports.new.title'), referential_line_route_vehicle_journey_exports_path(@referential, @line, @route, format: :zip), class: 'btn btn-primary' - - - if policy(@route).destroy? - = link_to referential_line_route_path(@referential, @line, @route), method: :delete, data: {confirm: t('routes.actions.destroy_confirm')}, class: 'btn btn-primary' do - span.fa.fa-trash - span = t('actions.destroy') + - @route.action_links.each do |link| + = link_to link.href, + method: link.method, + data: link.data, + class: 'btn btn-primary' do + = link.content / PageContent .page_content -- cgit v1.2.3 From 3a9dbe0db981536ea7c138915e423ac009235c60 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 15:39:34 +0200 Subject: Move Chouette:: decorators to root decorators/ directory Draper doesn't seem to look for decorators in subdirectories of decorators/, so instead of writing things like: @line = Chouette::LineDecorator.decorate(@line) in the controller, move the decorators to the root of decorators/ to allow us to continue using this syntax: @line = @line.decorate Refs #3479 --- app/decorators/chouette/line_decorator.rb | 47 ------------------- .../chouette/routing_constraint_zone_decorator.rb | 39 ---------------- app/decorators/chouette/time_table_decorator.rb | 51 --------------------- app/decorators/company_decorator.rb | 1 - app/decorators/line_decorator.rb | 49 ++++++++++++++++++++ .../routing_constraint_zone_decorator.rb | 41 +++++++++++++++++ app/decorators/time_table_decorator.rb | 53 ++++++++++++++++++++++ 7 files changed, 143 insertions(+), 138 deletions(-) delete mode 100644 app/decorators/chouette/line_decorator.rb delete mode 100644 app/decorators/chouette/routing_constraint_zone_decorator.rb delete mode 100644 app/decorators/chouette/time_table_decorator.rb create mode 100644 app/decorators/line_decorator.rb create mode 100644 app/decorators/routing_constraint_zone_decorator.rb create mode 100644 app/decorators/time_table_decorator.rb diff --git a/app/decorators/chouette/line_decorator.rb b/app/decorators/chouette/line_decorator.rb deleted file mode 100644 index e32acb13b..000000000 --- a/app/decorators/chouette/line_decorator.rb +++ /dev/null @@ -1,47 +0,0 @@ -class Chouette::LineDecorator < Draper::Decorator - delegate_all - - # Requires: - # context: { - # line_referential: - # } - def action_links - links = [] - - links << Link.new( - content: h.t('lines.actions.show_network'), - href: [context[:line_referential], object.network] - ) - - links << Link.new( - content: h.t('lines.actions.show_company'), - href: [context[:line_referential], object.company] - ) - - if h.policy(Chouette::Line).create? && - context[:line_referential].organisations.include?(current_organisation) - links << Link.new( - content: h.t('lines.actions.new'), - href: h.new_line_referential_line_path(context[:line_referential]) - ) - end - - # TODO: what if false? do we delete this? - if false && h.policy(object).update? - # = link_to t('lines.actions.edit'), edit_line_referential_line_path(context[:line_referential], object), class: 'btn btn-primary' - end - - if h.policy(object).destroy? - links << Link.new( -# TODO: this translation is different! -span = t('lines.actions.destroy') - content: h.destroy_link_content, - href: h.line_referential_line_path(context[:line_referential], object), - method: :delete, - data: { confirm: h.t('lines.actions.destroy_confirm') } - ) - end - - links - end -end diff --git a/app/decorators/chouette/routing_constraint_zone_decorator.rb b/app/decorators/chouette/routing_constraint_zone_decorator.rb deleted file mode 100644 index f95206064..000000000 --- a/app/decorators/chouette/routing_constraint_zone_decorator.rb +++ /dev/null @@ -1,39 +0,0 @@ -class Chouette::RoutingConstraintZoneDecorator < Draper::Decorator - delegate_all - - # Requires: - # context: { - # referential: , - # line: - # } - def action_links - links = [] - - if h.policy(object).update? - links << Link.new( - content: h.t('actions.edit'), - href: h.edit_referential_line_routing_constraint_zone_path( - context[:referential], - context[:line], - object - ) - ) - - if h.policy(object).destroy? - links << Link.new( - content: h.destroy_link_content, - href: h.referential_line_routing_constraint_zone_path( - context[:referential], - context[:line], - object - ), - method: :delete, - data: { - confirm: h.t('routing_constraint_zones.actions.destroy_confirm') - } - ) - end - - links - end -end diff --git a/app/decorators/chouette/time_table_decorator.rb b/app/decorators/chouette/time_table_decorator.rb deleted file mode 100644 index 7f2dbf5f1..000000000 --- a/app/decorators/chouette/time_table_decorator.rb +++ /dev/null @@ -1,51 +0,0 @@ -class Chouette::TimeTableDecorator < Draper::Decorator - delegate_all - - # Requires: - # context: { - # referential: , - # } - def action_links - links = [] - - if object.calendar - links << Link.new( - content: h.t('actions.actualize'), - href: h.actualize_referential_time_table_path( - context[:referential], - object - ), - method: :post - ) - end - - links << Link.new( - content: h.t('actions.combine'), - href: h.new_referential_time_table_time_table_combination_path( - context[:referential], - object - ) - ) - - if h.policy(object).duplicate? - links << Link.new( - content: h.t('actions.duplicate'), - href: h.duplicate_referential_time_table_path( - context[:referential], - object - ) - ) - end - - if h.policy(object).destroy? - Link.new( - content: h.destroy_link_content, - href: h.referential_time_table_path(context[:referential], object), - method: :delete, - data: { confirm: h.t('time_tables.actions.destroy_confirm') } - ) - end - - links - end -end diff --git a/app/decorators/company_decorator.rb b/app/decorators/company_decorator.rb index 3bd85319c..4adc51cc2 100644 --- a/app/decorators/company_decorator.rb +++ b/app/decorators/company_decorator.rb @@ -1,4 +1,3 @@ -# TODO: Move this into the Chouette:: namespace class CompanyDecorator < Draper::Decorator decorates Chouette::Company diff --git a/app/decorators/line_decorator.rb b/app/decorators/line_decorator.rb new file mode 100644 index 000000000..84f6cba91 --- /dev/null +++ b/app/decorators/line_decorator.rb @@ -0,0 +1,49 @@ +class LineDecorator < Draper::Decorator + decorates Chouette::Line + + delegate_all + + # Requires: + # context: { + # line_referential: + # } + def action_links + links = [] + + links << Link.new( + content: h.t('lines.actions.show_network'), + href: [context[:line_referential], object.network] + ) + + links << Link.new( + content: h.t('lines.actions.show_company'), + href: [context[:line_referential], object.company] + ) + + if h.policy(Chouette::Line).create? && + context[:line_referential].organisations.include?(current_organisation) + links << Link.new( + content: h.t('lines.actions.new'), + href: h.new_line_referential_line_path(context[:line_referential]) + ) + end + + # TODO: what if false? do we delete this? + if false && h.policy(object).update? + # = link_to t('lines.actions.edit'), edit_line_referential_line_path(context[:line_referential], object), class: 'btn btn-primary' + end + + if h.policy(object).destroy? + links << Link.new( +# TODO: this translation is different! +span = t('lines.actions.destroy') + content: h.destroy_link_content, + href: h.line_referential_line_path(context[:line_referential], object), + method: :delete, + data: { confirm: h.t('lines.actions.destroy_confirm') } + ) + end + + links + end +end diff --git a/app/decorators/routing_constraint_zone_decorator.rb b/app/decorators/routing_constraint_zone_decorator.rb new file mode 100644 index 000000000..4b5df20c1 --- /dev/null +++ b/app/decorators/routing_constraint_zone_decorator.rb @@ -0,0 +1,41 @@ +class RoutingConstraintZoneDecorator < Draper::Decorator + decorates Chouette::RoutingConstraintZone + + delegate_all + + # Requires: + # context: { + # referential: , + # line: + # } + def action_links + links = [] + + if h.policy(object).update? + links << Link.new( + content: h.t('actions.edit'), + href: h.edit_referential_line_routing_constraint_zone_path( + context[:referential], + context[:line], + object + ) + ) + + if h.policy(object).destroy? + links << Link.new( + content: h.destroy_link_content, + href: h.referential_line_routing_constraint_zone_path( + context[:referential], + context[:line], + object + ), + method: :delete, + data: { + confirm: h.t('routing_constraint_zones.actions.destroy_confirm') + } + ) + end + + links + end +end diff --git a/app/decorators/time_table_decorator.rb b/app/decorators/time_table_decorator.rb new file mode 100644 index 000000000..57661c627 --- /dev/null +++ b/app/decorators/time_table_decorator.rb @@ -0,0 +1,53 @@ +class TimeTableDecorator < Draper::Decorator + decorates Chouette::TimeTable + + delegate_all + + # Requires: + # context: { + # referential: , + # } + def action_links + links = [] + + if object.calendar + links << Link.new( + content: h.t('actions.actualize'), + href: h.actualize_referential_time_table_path( + context[:referential], + object + ), + method: :post + ) + end + + links << Link.new( + content: h.t('actions.combine'), + href: h.new_referential_time_table_time_table_combination_path( + context[:referential], + object + ) + ) + + if h.policy(object).duplicate? + links << Link.new( + content: h.t('actions.duplicate'), + href: h.duplicate_referential_time_table_path( + context[:referential], + object + ) + ) + end + + if h.policy(object).destroy? + Link.new( + content: h.destroy_link_content, + href: h.referential_time_table_path(context[:referential], object), + method: :delete, + data: { confirm: h.t('time_tables.actions.destroy_confirm') } + ) + end + + links + end +end -- cgit v1.2.3 From 6f1fa09dd12979933bef8bb97cdc6ae4ff10a9f0 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 15:58:02 +0200 Subject: RoutingConstraintZones#show: Render header links from #action_links Use the `RoutingConstraintZonesDecorator` to render the header links instead of constructing them in the view. This allows us to reuse the links. To be honest, I haven't actually tested this for real. Don't know how to set up an Interdiction de Traffic Local. Refs #3479 --- app/controllers/routing_constraint_zones_controller.rb | 4 ++++ app/views/routing_constraint_zones/show.html.slim | 13 ++++++------- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/app/controllers/routing_constraint_zones_controller.rb b/app/controllers/routing_constraint_zones_controller.rb index 7707427b0..9d2fd712c 100644 --- a/app/controllers/routing_constraint_zones_controller.rb +++ b/app/controllers/routing_constraint_zones_controller.rb @@ -16,6 +16,10 @@ class RoutingConstraintZonesController < ChouetteController def show @routing_constraint_zone = collection.find(params[:id]) + @routing_constraint_zone = @routing_constraint_zone.decorate(context: { + referential: @referential, + line: @line + }) end protected diff --git a/app/views/routing_constraint_zones/show.html.slim b/app/views/routing_constraint_zones/show.html.slim index f0c244387..1dad4f561 100644 --- a/app/views/routing_constraint_zones/show.html.slim +++ b/app/views/routing_constraint_zones/show.html.slim @@ -7,13 +7,12 @@ / Below is secundary actions & optional contents .row .col-lg-12.text-right.mb-sm - - if policy(@routing_constraint_zone).update? - = link_to t('actions.edit'), edit_referential_line_routing_constraint_zone_path(@referential, @line, @routing_constraint_zone), class: 'btn btn-primary' - - - if policy(@routing_constraint_zone).destroy? - = link_to referential_line_routing_constraint_zone_path(@referential, @line, @routing_constraint_zone), method: :delete, data: {confirm: t('routing_constraint_zones.actions.destroy_confirm')}, class: 'btn btn-primary' do - span.fa.fa-trash - span = t('actions.destroy') + - @routing_constraint_zone.action_links.each do |link| + = link_to link.href, + method: link.method, + data: link.data, + class: 'btn btn-primary' do + = link.content / PageContent .page_content -- cgit v1.2.3 From 7d45b2f803d4f14f1aa0d713b0c43ed9f41a41e7 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 17:22:13 +0200 Subject: CompanyDecorator#action_links: Add comment about `context` requirement Show what keys are required to be passed as `context` to the decorator. Missed this when I added the `context` dependency. Refs #3479 --- app/decorators/company_decorator.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/decorators/company_decorator.rb b/app/decorators/company_decorator.rb index 4adc51cc2..5393a5c3a 100644 --- a/app/decorators/company_decorator.rb +++ b/app/decorators/company_decorator.rb @@ -11,6 +11,10 @@ class CompanyDecorator < Draper::Decorator object.lines.count end + # Requires: + # context: { + # line_referential: + # } def action_links links = [] -- cgit v1.2.3 From d4de52d63f400f30dabb7034e14202f5c9bd0192 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 17:24:15 +0200 Subject: Lines#show: Render header links from #action_links Get the header links from the decorator so we can reuse them instead of defining them directly in the template. Add an option to the `destroy_link_content` Rails helper method to allow us to specify alternate translation keys, giving us different text in the button label. Added this because the template in question uses a different label than the others (usually it's `actions.destroy`). Refs #3479 --- app/controllers/lines_controller.rb | 4 ++++ app/decorators/line_decorator.rb | 4 +--- app/helpers/links_helper.rb | 4 ++-- app/views/lines/show.html.slim | 17 ++++++----------- 4 files changed, 13 insertions(+), 16 deletions(-) diff --git a/app/controllers/lines_controller.rb b/app/controllers/lines_controller.rb index 7eedaeb05..c3378208b 100644 --- a/app/controllers/lines_controller.rb +++ b/app/controllers/lines_controller.rb @@ -25,6 +25,10 @@ class LinesController < BreadcrumbController def show @group_of_lines = resource.group_of_lines show! do + @line = @line.decorate(context: { + line_referential: @line_referential + }) + build_breadcrumb :show end end diff --git a/app/decorators/line_decorator.rb b/app/decorators/line_decorator.rb index 84f6cba91..53053ed2c 100644 --- a/app/decorators/line_decorator.rb +++ b/app/decorators/line_decorator.rb @@ -35,9 +35,7 @@ class LineDecorator < Draper::Decorator if h.policy(object).destroy? links << Link.new( -# TODO: this translation is different! -span = t('lines.actions.destroy') - content: h.destroy_link_content, + content: h.destroy_link_content('lines.actions.destroy_confirm'), href: h.line_referential_line_path(context[:line_referential], object), method: :delete, data: { confirm: h.t('lines.actions.destroy_confirm') } diff --git a/app/helpers/links_helper.rb b/app/helpers/links_helper.rb index b9923db2f..683b66a52 100644 --- a/app/helpers/links_helper.rb +++ b/app/helpers/links_helper.rb @@ -1,5 +1,5 @@ module LinksHelper - def destroy_link_content - content_tag(:span, nil, class: 'fa fa-trash') + t('actions.destroy') + def destroy_link_content(translation_key = 'actions.destroy') + content_tag(:span, nil, class: 'fa fa-trash') + t(translation_key) end end diff --git a/app/views/lines/show.html.slim b/app/views/lines/show.html.slim index dbc019e72..6f75432e1 100644 --- a/app/views/lines/show.html.slim +++ b/app/views/lines/show.html.slim @@ -7,17 +7,12 @@ / Below is secundary actions & optional contents .row .col-lg-12.text-right.mb-sm - = link_to t('lines.actions.show_network'), [@line_referential, @line.network], class: 'btn btn-primary' - = link_to t('lines.actions.show_company'), [@line_referential, @line.company], class: 'btn btn-primary' - - - if policy(Chouette::Line).create? && @line_referential.organisations.include?(current_organisation) - = link_to t('lines.actions.new'), new_line_referential_line_path(@line_referential), class: 'btn btn-primary' - - if false && policy(@line).update? - = link_to t('lines.actions.edit'), edit_line_referential_line_path(@line_referential, @line), class: 'btn btn-primary' - - if policy(@line).destroy? - = link_to line_referential_line_path(@line_referential, @line), method: :delete, data: {confirm: t('lines.actions.destroy_confirm')}, class: 'btn btn-primary' do - span.fa.fa-trash - span = t('lines.actions.destroy') + - @line.action_links.each do |link| + = link_to link.href, + method: link.method, + data: link.data, + class: 'btn btn-primary' do + = link.content / PageContent .page_content -- cgit v1.2.3 From 7fe8b7d100e92060a14f7c7bc32a097947dd860e Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 17:27:06 +0200 Subject: TimeTableDecorator#action_links: Add missing destroy link Wasn't adding the destroy link to the `links` list, so it didn't get added to the header buttons. Refs #3479 --- app/decorators/time_table_decorator.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/decorators/time_table_decorator.rb b/app/decorators/time_table_decorator.rb index 57661c627..526537310 100644 --- a/app/decorators/time_table_decorator.rb +++ b/app/decorators/time_table_decorator.rb @@ -40,7 +40,7 @@ class TimeTableDecorator < Draper::Decorator end if h.policy(object).destroy? - Link.new( + links << Link.new( content: h.destroy_link_content, href: h.referential_time_table_path(context[:referential], object), method: :delete, -- cgit v1.2.3 From 4e81e22aff65221316c1528a3f2fa82272345c65 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 17:28:27 +0200 Subject: Calendars#show: Use #action_links to render header buttons To abstract the links and make them reusable in other contexts, render the buttons in the header from `CalendarDecorator#action_links`. Refs #3479 --- app/controllers/calendars_controller.rb | 4 ++++ app/views/calendars/show.html.slim | 10 ++++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/controllers/calendars_controller.rb b/app/controllers/calendars_controller.rb index 86d567882..432e528f0 100644 --- a/app/controllers/calendars_controller.rb +++ b/app/controllers/calendars_controller.rb @@ -5,6 +5,10 @@ class CalendarsController < BreadcrumbController respond_to :html respond_to :js, only: :index + def show + @calendar = @calendar.decorate + end + private def calendar_params permitted_params = [:id, :name, :short_name, periods_attributes: [:id, :begin, :end, :_destroy], date_values_attributes: [:id, :value, :_destroy]] diff --git a/app/views/calendars/show.html.slim b/app/views/calendars/show.html.slim index 3886cefaa..26248cea8 100644 --- a/app/views/calendars/show.html.slim +++ b/app/views/calendars/show.html.slim @@ -8,10 +8,12 @@ / Below is secondary actions & optional contents (filters, ...) .row.mb-sm .col-lg-12.text-right - - if policy(@calendar).destroy? - = link_to calendar_path(@calendar), method: :delete, data: { confirm: t('calendars.actions.destroy_confirm') }, class: 'btn btn-primary' do - span.fa.fa-trash - span = t('actions.destroy') + - @calendar.action_links.each do |link| + = link_to link.href, + method: link.method, + data: link.data, + class: 'btn btn-primary' do + = link.content / PageContent .page_content -- cgit v1.2.3 From 9de20c519a60443157ed277a1c68b46f00229598 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 17:29:32 +0200 Subject: TimeTables#show: Render header buttons from #action_links This enables us to reuse these links in other parts of the application. Get the links from `TimeTableDecorator#action_links`. Refs #3479 --- app/controllers/time_tables_controller.rb | 4 ++++ app/views/time_tables/show.html.slim | 21 ++++++--------------- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/app/controllers/time_tables_controller.rb b/app/controllers/time_tables_controller.rb index 5c4552afb..3704f2885 100644 --- a/app/controllers/time_tables_controller.rb +++ b/app/controllers/time_tables_controller.rb @@ -14,6 +14,10 @@ class TimeTablesController < ChouetteController @year = params[:year] ? params[:year].to_i : Date.today.cwyear @time_table_combination = TimeTableCombination.new show! do + @time_table = @time_table.decorate(context: { + referential: @referential + }) + build_breadcrumb :show end end diff --git a/app/views/time_tables/show.html.slim b/app/views/time_tables/show.html.slim index 2e71ebb9e..f596fd480 100644 --- a/app/views/time_tables/show.html.slim +++ b/app/views/time_tables/show.html.slim @@ -10,21 +10,12 @@ / Below is secundary actions & optional contents (filters, ...) .row.mb-sm .col-lg-12.text-right - / - if policy(@time_table).create? && @referential.organisation == current_organisation - / = link_to t('time_tables.actions.new'), new_referential_time_table_path(@referential), class: 'btn btn-primary' - - if @time_table.calendar - = link_to t('actions.actualize'), actualize_referential_time_table_path(@referential, @time_table), method: :post, class: 'btn btn-primary' - - /- if policy(@time_table).create? && @referential.organisation == current_organisation - = link_to t('actions.combine'), new_referential_time_table_time_table_combination_path(@referential, @time_table), class: 'btn btn-primary' - - - if policy(@time_table).duplicate? - = link_to t('actions.duplicate'), duplicate_referential_time_table_path(@referential, @time_table), class: 'btn btn-primary' - - - if policy(@time_table).destroy? - = link_to referential_time_table_path(@referential, @time_table), method: :delete, data: {confirm: t('time_tables.actions.destroy_confirm')}, class: 'btn btn-primary' do - span.fa.fa-trash - span = t('actions.destroy') + - @time_table.action_links.each do |link| + = link_to link.href, + method: link.method, + data: link.data, + class: 'btn btn-primary' do + = link.content / PageContent .page_content -- cgit v1.2.3 From 1f569d51d82f741811b36080df47bde94c3ad58c Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 17:31:20 +0200 Subject: LineDecorator: Remove unused link This link was brought in from the "time_tables/show.html.slim" template when I ported the links over to `LineDecorator#action_links`. The link was wrapped in an `if false` from 5bd3973fae2c1e02c2bef31d5b12dc10b1a133c9. Presumably we no longer care about that edit link, so since the code never executes, I'm deleting it. Refs #3479 --- app/decorators/line_decorator.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/app/decorators/line_decorator.rb b/app/decorators/line_decorator.rb index 53053ed2c..d4a616a97 100644 --- a/app/decorators/line_decorator.rb +++ b/app/decorators/line_decorator.rb @@ -28,11 +28,6 @@ class LineDecorator < Draper::Decorator ) end - # TODO: what if false? do we delete this? - if false && h.policy(object).update? - # = link_to t('lines.actions.edit'), edit_line_referential_line_path(context[:line_referential], object), class: 'btn btn-primary' - end - if h.policy(object).destroy? links << Link.new( content: h.destroy_link_content('lines.actions.destroy_confirm'), -- cgit v1.2.3 From f483c2db410f78287e77ac968a7229b0d5e5b835 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 17:47:40 +0200 Subject: TableBuilder spec: Remove TODO We're how have a separate module for the box containing links to perform actions on selections of multiple objects in the table. I had put this in `MultipleSelectionToolboxHelper`. Refs #3479 --- spec/helpers/table_builder_helper_spec.rb | 6 ------ 1 file changed, 6 deletions(-) diff --git a/spec/helpers/table_builder_helper_spec.rb b/spec/helpers/table_builder_helper_spec.rb index a61fd2343..32a6a3bfd 100644 --- a/spec/helpers/table_builder_helper_spec.rb +++ b/spec/helpers/table_builder_helper_spec.rb @@ -90,12 +90,6 @@ describe TableBuilderHelper, type: :helper do
HTML -# TODO: Create a module for the selection box -#
-#
    -#
  • -#
0 élément(s) sélectionné(s) -#
html_str = helper.table_builder_2( referentials, -- cgit v1.2.3 From a7701750889ea809fdaa1a745679131a00e9b8a1 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 17:51:09 +0200 Subject: CompanyDecorator: Fix helper method call The helper method should be called on the `h` object because we're in a decorator. Also get rid of the extra space. Refs #3479 --- app/decorators/company_decorator.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/decorators/company_decorator.rb b/app/decorators/company_decorator.rb index 5393a5c3a..51c1f3c61 100644 --- a/app/decorators/company_decorator.rb +++ b/app/decorators/company_decorator.rb @@ -43,7 +43,7 @@ class CompanyDecorator < Draper::Decorator object ), method: :delete, - data: { confirm: t('companies.actions.destroy_confirm') } + data: { confirm: h.t('companies.actions.destroy_confirm') } ) end -- cgit v1.2.3 From 493c13421a0c47db72185bc8f0ad52ef6d31fc97 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 17:56:27 +0200 Subject: LineDecorator: Remove extra whitespace Refs #3479 --- app/decorators/line_decorator.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/decorators/line_decorator.rb b/app/decorators/line_decorator.rb index d4a616a97..42d903b10 100644 --- a/app/decorators/line_decorator.rb +++ b/app/decorators/line_decorator.rb @@ -33,7 +33,7 @@ class LineDecorator < Draper::Decorator content: h.destroy_link_content('lines.actions.destroy_confirm'), href: h.line_referential_line_path(context[:line_referential], object), method: :delete, - data: { confirm: h.t('lines.actions.destroy_confirm') } + data: { confirm: h.t('lines.actions.destroy_confirm') } ) end -- cgit v1.2.3 From c7f5fa2bb5eb1b166d30bdf8c16efd8653fd24f6 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 17:56:47 +0200 Subject: RoutingConstraintZoneDecorator: Add missing `end` statement Must have bugged when porting this from Slim template code. Refs #3479 --- app/decorators/routing_constraint_zone_decorator.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/decorators/routing_constraint_zone_decorator.rb b/app/decorators/routing_constraint_zone_decorator.rb index 4b5df20c1..0b438a554 100644 --- a/app/decorators/routing_constraint_zone_decorator.rb +++ b/app/decorators/routing_constraint_zone_decorator.rb @@ -20,6 +20,7 @@ class RoutingConstraintZoneDecorator < Draper::Decorator object ) ) + end if h.policy(object).destroy? links << Link.new( -- cgit v1.2.3 From 443f281c763670aeee62109cf7bd510f1d88d7c6 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 18:02:20 +0200 Subject: TableBuilderHelper: Get rid of extra unnecessary whitespace Refs #3479 --- app/helpers/table_builder_helper.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/helpers/table_builder_helper.rb b/app/helpers/table_builder_helper.rb index ebffb0da6..b93e9b22b 100644 --- a/app/helpers/table_builder_helper.rb +++ b/app/helpers/table_builder_helper.rb @@ -67,7 +67,6 @@ module TableBuilderHelper # A CSS class to apply to the cls: '' ) - content_tag :table, thead(collection, columns, sortable, selectable, links.any?) + tbody(collection, columns, selectable, links), -- cgit v1.2.3 From 533c1a55df630080cd0ff449e229717ba7ace212 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 18:08:28 +0200 Subject: Revert "factories/workbenches: Add a new `:with_referential` trait" This reverts commit 073ac0eb3330787532f551d20abe4600a0615446. I no longer need this factory. Originally added it for a test where I thought I needed a collection of `Workbench`es. Turns out I instead needed a collection of `Referential`s. Thus we don't have a need of creating a workbench with a referential together. --- spec/factories/workbenches.rb | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/spec/factories/workbenches.rb b/spec/factories/workbenches.rb index 154482213..f51e7d94c 100644 --- a/spec/factories/workbenches.rb +++ b/spec/factories/workbenches.rb @@ -5,24 +5,5 @@ FactoryGirl.define do association :organisation, :factory => :organisation association :line_referential association :stop_area_referential - - trait :with_referential do - # TODO: change all => to : - # association :referential, - # organisation: { organisation } - - # after(:stub) do |workbench, evaluator| - # - # end - - referentials do |workbench| - [association( - :referential, - organisation: workbench.organisation, - line_referential: workbench.line_referential, - stop_area_referential: workbench.stop_area_referential - )] - end - end end end -- cgit v1.2.3 From b281113868ee80edb61de5b03adb761e4ceca036 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 18:20:59 +0200 Subject: CalendarsController#show: Wrap `@calendar` assignment in `#show!` Don't really know what this is (assuming it comes from 'inherited_resources'), but putting the decoration inside this block seemed to work in the other cases I tried recently (like `LinesController#show`). Refs #3479 --- app/controllers/calendars_controller.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/controllers/calendars_controller.rb b/app/controllers/calendars_controller.rb index 432e528f0..5662316b8 100644 --- a/app/controllers/calendars_controller.rb +++ b/app/controllers/calendars_controller.rb @@ -6,7 +6,9 @@ class CalendarsController < BreadcrumbController respond_to :js, only: :index def show - @calendar = @calendar.decorate + show! do + @calendar = @calendar.decorate + end end private -- cgit v1.2.3 From 140270297b1db064b78472ccd7e59b8279d21be7 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 18:35:26 +0200 Subject: lines/show.html.erb_spec.rb: Fix specs resulting from `LineDecorator` The tests failed because we weren't using a decorated `Chouette::Line` object, but the views now expect decorated lines in order to be able to call the `#action_links` method. Here are our failures: 5) /lines/show should render h1 with the line name Failure/Error: - @line.action_links.each do |link| ActionView::Template::Error: undefined method `action_links' for # # ./app/views/lines/show.html.slim:10:in `block in _app_views_lines_show_html_slim___3813514632397395671_70250211127360' # ./app/helpers/newapplication_helper.rb:246:in `block (2 levels) in pageheader' # ./app/helpers/newapplication_helper.rb:244:in `block in pageheader' # ./app/helpers/newapplication_helper.rb:243:in `pageheader' # ./app/views/lines/show.html.slim:2:in `_app_views_lines_show_html_slim___3813514632397395671_70250211127360' # ./spec/views/lines/show.html.erb_spec.rb:16:in `block (2 levels) in ' # -e:1:in `
' # ------------------ # --- Caused by: --- # NoMethodError: # undefined method `action_links' for # # ./app/views/lines/show.html.slim:10:in `block in _app_views_lines_show_html_slim___3813514632397395671_70250211127360' 6) /lines/show should render a link to remove the line Failure/Error: - @line.action_links.each do |link| ActionView::Template::Error: undefined method `action_links' for # # ./app/views/lines/show.html.slim:10:in `block in _app_views_lines_show_html_slim___3813514632397395671_70250211127360' # ./app/helpers/newapplication_helper.rb:246:in `block (2 levels) in pageheader' # ./app/helpers/newapplication_helper.rb:244:in `block in pageheader' # ./app/helpers/newapplication_helper.rb:243:in `pageheader' # ./app/views/lines/show.html.slim:2:in `_app_views_lines_show_html_slim___3813514632397395671_70250211127360' # ./spec/views/lines/show.html.erb_spec.rb:31:in `block (2 levels) in ' # -e:1:in `
' # ------------------ # --- Caused by: --- # NoMethodError: # undefined method `action_links' for # # ./app/views/lines/show.html.slim:10:in `block in _app_views_lines_show_html_slim___3813514632397395671_70250211127360' Update the test to decorate the lines used in them. Turn `current_organisation` into a `context` field on `LineDecorator` because otherwise it's a global variable that comes from wherever that's required in `LineDecorator`. And we'd have to mock `current_organisation` on `line` in the test, which I wasn't keen on. Refs #3479 --- app/controllers/lines_controller.rb | 3 ++- app/decorators/line_decorator.rb | 7 +++++-- spec/views/lines/show.html.erb_spec.rb | 8 +++++++- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/app/controllers/lines_controller.rb b/app/controllers/lines_controller.rb index c3378208b..1e2056aad 100644 --- a/app/controllers/lines_controller.rb +++ b/app/controllers/lines_controller.rb @@ -26,7 +26,8 @@ class LinesController < BreadcrumbController @group_of_lines = resource.group_of_lines show! do @line = @line.decorate(context: { - line_referential: @line_referential + line_referential: @line_referential, + current_organisation: current_organisation }) build_breadcrumb :show diff --git a/app/decorators/line_decorator.rb b/app/decorators/line_decorator.rb index 42d903b10..f351103b2 100644 --- a/app/decorators/line_decorator.rb +++ b/app/decorators/line_decorator.rb @@ -5,7 +5,8 @@ class LineDecorator < Draper::Decorator # Requires: # context: { - # line_referential: + # line_referential: , + # current_organisation: # } def action_links links = [] @@ -21,7 +22,9 @@ class LineDecorator < Draper::Decorator ) if h.policy(Chouette::Line).create? && - context[:line_referential].organisations.include?(current_organisation) + context[:line_referential].organisations.include?( + context[:current_organisation] + ) links << Link.new( content: h.t('lines.actions.new'), href: h.new_line_referential_line_path(context[:line_referential]) diff --git a/spec/views/lines/show.html.erb_spec.rb b/spec/views/lines/show.html.erb_spec.rb index 3a9efa0ce..7bc120f1a 100644 --- a/spec/views/lines/show.html.erb_spec.rb +++ b/spec/views/lines/show.html.erb_spec.rb @@ -3,7 +3,13 @@ require 'spec_helper' describe "/lines/show", :type => :view do assign_referential - let!(:line) { assign :line, create(:line) } + let!(:line) do + line = create(:line) + assign :line, line.decorate(context: { + line_referential: line.line_referential, + current_organisation: referential.organisation + }) + end let!(:line_referential) { assign :line_referential, line.line_referential } let!(:routes) { assign :routes, Array.new(2) { create(:route, :line => line) }.paginate } let!(:map) { assign(:map, double(:to_html => '
'.html_safe)) } -- cgit v1.2.3 From 3343920fe8df5378f7bced238b03860e249ac1cf Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 18:43:16 +0200 Subject: time_tables/show.html.erb_spec.rb: Fix specs from `TimeTableDecorator` Was getting these failures after switching the controller over to use a decorated `@time_table`: Failures: 1) /time_tables/show should render h2 with the time_table comment Failure/Error: - @time_table.action_links.each do |link| ActionView::Template::Error: undefined method `action_links' for # # ./app/views/time_tables/show.html.slim:13:in `block in _app_views_time_tables_show_html_slim__2702796193816469913_70250218141540' # ./app/helpers/newapplication_helper.rb:246:in `block (2 levels) in pageheader' # ./app/helpers/newapplication_helper.rb:244:in `block in pageheader' # ./app/helpers/newapplication_helper.rb:243:in `pageheader' # ./app/views/time_tables/show.html.slim:4:in `_app_views_time_tables_show_html_slim__2702796193816469913_70250218141540' # ./spec/views/time_tables/show.html.erb_spec.rb:15:in `block (2 levels) in ' # -e:1:in `
' # ------------------ # --- Caused by: --- # NoMethodError: # undefined method `action_links' for # # ./app/views/time_tables/show.html.slim:13:in `block in _app_views_time_tables_show_html_slim__2702796193816469913_70250218141540' 2) /time_tables/show should render a link to edit the time_table Failure/Error: - @time_table.action_links.each do |link| ActionView::Template::Error: undefined method `action_links' for # # ./app/views/time_tables/show.html.slim:13:in `block in _app_views_time_tables_show_html_slim__2702796193816469913_70250218141540' # ./app/helpers/newapplication_helper.rb:246:in `block (2 levels) in pageheader' # ./app/helpers/newapplication_helper.rb:244:in `block in pageheader' # ./app/helpers/newapplication_helper.rb:243:in `pageheader' # ./app/views/time_tables/show.html.slim:4:in `_app_views_time_tables_show_html_slim__2702796193816469913_70250218141540' # ./spec/views/time_tables/show.html.erb_spec.rb:20:in `block (2 levels) in ' # -e:1:in `
' # ------------------ # --- Caused by: --- # NoMethodError: # undefined method `action_links' for # # ./app/views/time_tables/show.html.slim:13:in `block in _app_views_time_tables_show_html_slim__2702796193816469913_70250218141540' 3) /time_tables/show should render a link to remove the time_table Failure/Error: - @time_table.action_links.each do |link| ActionView::Template::Error: undefined method `action_links' for # # ./app/views/time_tables/show.html.slim:13:in `block in _app_views_time_tables_show_html_slim__2702796193816469913_70250218141540' # ./app/helpers/newapplication_helper.rb:246:in `block (2 levels) in pageheader' # ./app/helpers/newapplication_helper.rb:244:in `block in pageheader' # ./app/helpers/newapplication_helper.rb:243:in `pageheader' # ./app/views/time_tables/show.html.slim:4:in `_app_views_time_tables_show_html_slim__2702796193816469913_70250218141540' # ./spec/views/time_tables/show.html.erb_spec.rb:25:in `block (2 levels) in ' # -e:1:in `
' # ------------------ # --- Caused by: --- # NoMethodError: # undefined method `action_links' for # # ./app/views/time_tables/show.html.slim:13:in `block in _app_views_time_tables_show_html_slim__2702796193816469913_70250218141540' Finished in 2.44 seconds (files took 0.82818 seconds to load) 3 examples, 3 failures Failed examples: rspec ./spec/views/time_tables/show.html.erb_spec.rb:14 # /time_tables/show should render h2 with the time_table comment rspec ./spec/views/time_tables/show.html.erb_spec.rb:19 # /time_tables/show should render a link to edit the time_table rspec ./spec/views/time_tables/show.html.erb_spec.rb:24 # /time_tables/show should render a link to remove the time_table To match the controller, we need to decorate the `time_table` objects used in these tests as well, because the view code has changed. Decoration is necessary to give us the `#action_links` method that we use to render the header links on the page. Refs #3479 --- spec/views/time_tables/show.html.erb_spec.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/spec/views/time_tables/show.html.erb_spec.rb b/spec/views/time_tables/show.html.erb_spec.rb index f429f9dec..edfb3f3cc 100644 --- a/spec/views/time_tables/show.html.erb_spec.rb +++ b/spec/views/time_tables/show.html.erb_spec.rb @@ -3,7 +3,14 @@ require 'spec_helper' describe "/time_tables/show", :type => :view do assign_referential - let!(:time_table) { assign(:time_table, create(:time_table)) } + let!(:time_table) do + assign( + :time_table, + create(:time_table).decorate(context: { + referential: referential + }) + ) + end let!(:year) { assign(:year, Date.today.cwyear) } let!(:time_table_combination) {assign(:time_table_combination, TimeTableCombination.new)} -- cgit v1.2.3 From 150e71e3110292c0c27fe35a63e8a2d0e26c5259 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 19 Jun 2017 18:47:32 +0200 Subject: referentials_permissions_spec.rb: Fix failure from `#destroy_link_content` Now, destroy ("Supprimer") buttons in the header get rendered without a `` tag surrounding the text inside the button. This is because I consolidated the HTML structure of the delete buttons inside table gear menus and the header. The ones in the gear menus had no span, and these in the header do. In both places, we're now using `LinksHelper#destroy_link_content` to generate the contents of the button, making the contents uniform in both contexts. This means, however, that the markup is not the same for header buttons, which now have their inner `` tag removed. Update the test to reflect this. Also, the styling breaks when I updated the markup, but Jean-Paul offered to fix that. Refs #3479 --- spec/features/referentials_permissions_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/referentials_permissions_spec.rb b/spec/features/referentials_permissions_spec.rb index 0216eeeb0..c37dff5b9 100644 --- a/spec/features/referentials_permissions_spec.rb +++ b/spec/features/referentials_permissions_spec.rb @@ -31,7 +31,7 @@ describe "Referentials", :type => :feature do end it 'shows the delete button' do expected_href = referential_path(referential) - expect( page ).to have_css(%{a[href=#{expected_href.inspect}] span}, text: destroy_link_text) + expect( page ).to have_css(%{a[href=#{expected_href.inspect}]}, text: destroy_link_text) end end -- cgit v1.2.3