Merge pull request #1 from finnlabs/card-groups

Card groups

README.md

@@ -68,36 +68,59 @@ Rows - A YAML text block which defines in detail what should appear in each row
 The following sample YAML shows the required form and all of the available configuration options:
 
 <pre>
-rows:
-  row1:
-    has_border: false
-    columns:
-      id:
-        has_label: false
-        font_size: 20
-        font_style: bold
-        priority: 1
-        minimum_lines: 2
-        render_if_empty: false
-        width: 30%
-      due_date:
-        has_label: false
-        font_size: 15
-        font_style: italic
-        priority: 1
-        minimum_lines: 2
-        render_if_empty: false
-        width: 70%
-  row2:
-    has_border: false
-    columns:
-      status:
-        has_label: true
-        font_size: 15
-        font_style: normal
-        priority: 2
-        minimum_lines: 1
-        render_if_empty: true
+group1:
+  has_border: false
+  rows:
+    row1:
+      priority: 1
+      columns:
+        id:
+          has_label: false
+          min_font_size: 10
+          max_font_size: 20
+          font_size: 20
+          font_style: bold
+          text_align: left
+          minimum_lines: 2
+          render_if_empty: false
+          width: 30%
+        due_date:
+          has_label: false
+          font_size: 15
+          font_style: italic
+          minimum_lines: 2
+          render_if_empty: false
+          width: 70%
+    row2:
+      priority: 2
+      columns:
+        status:
+          has_label: true
+          font_size: 15
+          font_style: normal
+          minimum_lines: 1
+          render_if_empty: true
+group2:
+  has_border: true
+  rows:
+    row1:
+      priority: 2
+      columns:
+        description:
+          has_label: true
+          font_size: 15
+          font_style: normal
+          minimum_lines: 1
+          render_if_empty: true
+    row2:
+      priority: 2
+      columns:
+        status:
+          has_label: true
+          font_size: 15
+          font_style: normal
+          minimum_lines: 1
+          render_if_empty: true
 </pre>
 
 Any number of rows can be defined. The font_size and minimum_lines properties define how much height on the card is given to the row. The plugin will attempt to assign enough space to each of the rows, however space will be assigned based on the priorities of the the rows, with rows with lower priority (higher numbers) being reduced and removed first if there is not enough for all the data.

app/models/export_card_configuration.rb

@@ -2,10 +2,47 @@
 class ExportCardConfiguration < ActiveRecord::Base
 
   class RowsYamlValidator < ActiveModel::Validator
-    # Note: For now this is just checking to see if it's valid YAML
+    REQUIRED_GROUP_KEYS = ["rows"]
+    VALID_GROUP_KEYS = ["rows", "has_border"]
+    REQUIRED_ROW_KEYS = ["columns"]
+    VALID_ROW_KEYS = ["columns", "height", "priority"]
+    # TODO: Security Consideration
+    # Should we define which model properties are visible and if so how?
+    # VALID_MODEL_PROPERTIES = [""]
+    REQUIRED_COLUMN_KEYS = []
+    VALID_COLUMN_KEYS = ["has_label", "min_font_size", "max_font_size",
+      "font_size", "font_style", "text_align", "minimum_lines", "render_if_empty",
+      "width"]
+
+    def assert_required_keys(hash, valid_keys, required_keys)
+      hash.assert_valid_keys valid_keys
+      pending_keys = required_keys - hash.keys
+      raise(ArgumentError, "Required key(s) not present: #{pending_keys.join(", ")}") unless pending_keys.empty?
+    end
+
     def validate(record)
       if record.rows.nil? || !(YAML::load(record.rows)).is_a?(Hash)
-        record.errors[:rows] << "Rows YAML is badly formed."
+        record.errors[:rows] << "YAML is badly formed."
+        return false
+      end
+
+      begin
+        groups = YAML::load(record.rows)
+        groups.each do |gk, gv|
+          assert_required_keys(gv, VALID_GROUP_KEYS, REQUIRED_GROUP_KEYS)
+          if gv.has_key?("rows") && gv["rows"].is_a?(Hash)
+            gv["rows"].each do |rk, rv|
+              assert_required_keys(rv, VALID_ROW_KEYS, REQUIRED_ROW_KEYS)
+              if rv.has_key?("columns") && rv["columns"].is_a?(Hash)
+                rv["columns"].each do |ck, cv|
+                  assert_required_keys(cv, VALID_COLUMN_KEYS, REQUIRED_COLUMN_KEYS)
+                end
+              end
+            end
+          end
+        end
+      rescue ArgumentError => e
+        record.errors[:rows] << "YAML error: #{e.message}"
       end
     end
   end

features/export_card_configurations_admin.feature

@@ -22,7 +22,7 @@ Feature: export card configurations Admin
     And I fill in "Config 1" for "export_card_configuration_name"
     And I fill in "5" for "export_card_configuration_per_page"
     And I select "landscape" from "export_card_configuration_orientation"
-    And I fill in "rows:" for "export_card_configuration_rows"
+    And I fill in valid YAML for export config rows
     And I submit the form by the "Create" button
     Then I should see "Successful creation." within ".flash.notice"
 
@@ -34,7 +34,7 @@ Feature: export card configurations Admin
     When I follow first "Custom 2"
     And I fill in "5" for "export_card_configuration_per_page"
     And I select "portrait" from "export_card_configuration_orientation"
-    And I fill in "rows:" for "export_card_configuration_rows"
+    And I fill in valid YAML for export config rows
     And I submit the form by the "Save" button
     Then I should see "Successful update." within ".flash.notice"
 

features/step_definitions/given.rb

@@ -5,7 +5,7 @@ Given /^there are multiple export card configurations$/ do
     per_page: 1,
     page_size: "A4",
     orientation: "landscape",
-    rows: "rows:\n    row1:\n      has_border: false\n      columns:\n        id:\n          has_label: false\n          font_size: 15"
+    rows: "group1:\n  has_border: false\n  rows:\n    row1:\n      height: 50\n      priority: 1\n      columns:\n        id:\n          has_label: false"
   })
   config2 = ExportCardConfiguration.create!({
     name: "Custom",
@@ -13,7 +13,7 @@ Given /^there are multiple export card configurations$/ do
     per_page: 1,
     page_size: "A4",
     orientation: "landscape",
-    rows: "rows:\n    row1:\n      has_border: false\n      columns:\n        id:\n          has_label: false\n          font_size: 15"
+    rows: "group1:\n  has_border: false\n  rows:\n    row1:\n      height: 50\n      priority: 1\n      columns:\n        id:\n          has_label: false"
   })
   config3 = ExportCardConfiguration.create!({
     name: "Custom 2",
@@ -21,7 +21,7 @@ Given /^there are multiple export card configurations$/ do
     per_page: 1,
     page_size: "A4",
     orientation: "landscape",
-    rows: "rows:\n    row1:\n      has_border: false\n      columns:\n        id:\n          has_label: false\n          font_size: 15"
+    rows: "group1:\n  has_border: false\n  rows:\n    row1:\n      height: 50\n      priority: 1\n      columns:\n        id:\n          has_label: false"
   })
   config4 = ExportCardConfiguration.create!({
     name: "Custom Inactive",
@@ -29,7 +29,7 @@ Given /^there are multiple export card configurations$/ do
     per_page: 1,
     page_size: "A4",
     orientation: "landscape",
-    rows: "rows:\n    row1:\n      has_border: false\n      columns:\n        id:\n          has_label: false\n          font_size: 15"
+    rows: "group1:\n  has_border: false\n  rows:\n    row1:\n      height: 50\n      priority: 1\n      columns:\n        id:\n          has_label: false"
   })
   [config1, config2, config3, config4]
 end
@@ -41,7 +41,12 @@ Given /^there is the default export card configuration$/ do
     per_page: 1,
     page_size: "A4",
     orientation: "landscape",
-    rows: "rows:\n    row1:\n      has_border: false\n      columns:\n        id:\n          has_label: false\n          font_size: 15"
+    rows: "group1:\n  has_border: false\n  rows:\n    row1:\n      height: 50\n      priority: 1\n      columns:\n        id:\n          has_label: false"
   })
   [config1]
+end
+
+Given /^I fill in valid YAML for export config rows$/ do
+  valid_yaml = "groups:\n  rows:\n    row1:\n      columns:\n        id:\n          has_label: false"
+  fill_in("export_card_configuration_rows", :with => valid_yaml)
 end

lib/open_project/pdf_export/export_card/card_element.rb

@@ -2,51 +2,67 @@ module OpenProject::PdfExport::ExportCard
   class CardElement
     include OpenProject::PdfExport::Exceptions
 
-    def initialize(pdf, orientation, rows_config, work_package)
+    def initialize(pdf, orientation, groups_config, work_package)
       @pdf = pdf
       @orientation = orientation
-      @rows_config = rows_config
+      @groups_config = groups_config
       @work_package = work_package
-      @row_elements = []
-      # TODO: This is redundant, the has should just be the rows
-      #       OR if we're going to have boxed groups then this is where they'd be
-      @rows = @rows_config["rows"]
+      @group_elements = []
 
-      raise BadlyFormedExportCardConfigurationError.new("Badly formed YAML") if @rows.nil?
+      # raise BadlyFormedExportCardConfigurationError.new("Badly formed YAML") if @rows.nil?
 
       # Simpler to remove empty rows before calculating the row sizes
-      RowElement.prune_empty_rows(@rows, work_package)
+      RowElement.prune_empty_groups(@groups_config, work_package)
+
+      # Get an array of all the row hashes
+      rows = []
+      @groups_config.each do |gk, gv|
+        gv["rows"].each do |rk, rv|
+          rows << rv
+        end
+      end
+
+      # Assign the row height, ignoring groups
+      heights = assign_row_heights(rows)
 
-      heights = assign_row_heights
       text_padding = @orientation[:text_padding]
+      group_padding = @orientation[:group_padding]
+      current_row = 0
       current_y_offset = text_padding
 
-      @rows.each_with_index do |(key, value), i|
-        current_y_offset += (heights[i - 1]) if i > 0
-        row_orientation = {
+      # Initialize groups
+      @groups_config.each_with_index do |(g_key, g_value), i|
+        row_count = g_value["rows"].count
+        row_heights = heights.slice(current_row, row_count)
+        group_height = row_heights.sum
+        group_orientation = {
           y_offset: @orientation[:height] - current_y_offset,
           x_offset: 0,
           width: @orientation[:width],
-          height: heights[i],
-          text_padding: text_padding
+          height: group_height,
+          row_heights: row_heights,
+          text_padding: text_padding,
+          group_padding: group_padding
         }
+        @group_elements << GroupElement.new(@pdf, group_orientation, g_value, @work_package)
 
-        @row_elements << RowElement.new(@pdf, row_orientation, value, @work_package)
+        current_y_offset += group_height
+        current_row += row_count
       end
     end
 
-    def assign_row_heights
-      # Assign initial heights
+    def assign_row_heights(rows)
+      # Assign initial heights for rows in all groups
       available = @orientation[:height] - @orientation[:text_padding]
-      c = @rows.count
+      c = rows.count
       assigned_heights = Array.new(c){ available / c }
 
-      min_heights = min_row_heights(c)
+      min_heights = min_row_heights(rows)
       diffs = assigned_heights.zip(min_heights).map {|a, m| a - m}
       diffs.each_with_index do |diff, i|
         if diff < 0
           # Need to grab some pixels from a low priority row and add them to current one
-          reduce_low_priority_rows(assigned_heights, diffs, i)
+          reduce_low_priority_rows(rows, assigned_heights, diffs, i)
         end
       end
 
@@ -54,10 +70,10 @@ module OpenProject::PdfExport::ExportCard
       assigned_heights
     end
 
-    def reduce_low_priority_rows(assigned_heights, diffs, conflicted_i)
+    def reduce_low_priority_rows(rows, assigned_heights, diffs, conflicted_i)
       # Get an array of row indexes sorted by inverse priority
-      priorities = *(0..@rows_config["rows"].count - 1)
-        .zip(@rows_config["rows"].map { |k, v| first_column_property(v, "priority") or 10 })
+      priorities = *(0..rows.count - 1)
+        .zip(rows.map { |row| row["priority"] or 10 })
         .sort {|x,y| y[1] <=> x[1]}
         .map {|x| x[0]}
 
@@ -77,11 +93,6 @@ module OpenProject::PdfExport::ExportCard
       return false
     end
 
-    def first_column_property(row_hash, property)
-      k, v = row_hash["columns"].first
-      v[property]
-    end
-
     def exchange(heights, diffs, a, b, v)
       heights[a] -= v
       heights[b] += v
@@ -89,21 +100,27 @@ module OpenProject::PdfExport::ExportCard
       diffs[b] += v
     end
 
-    def min_row_heights(c)
+    def min_row_heights(rows)
       # Calculate minimum user assigned heights...
-      min_heights = Array.new(c)
-      @rows_config["rows"].each_with_index do |(key, value), i|
-        # min lines * font height (first col) # TODO: get the biggest one
-        k, v = value["columns"].first
-        min_lines = v["minimum_lines"]
-        min_lines ||= 1
-        font_size = v["font_size"]
-        font_size ||= 10
-        min_heights[i] = (@pdf.font.height_at(font_size) * min_lines).floor
+      min_heights = Array.new(rows.count)
+      rows.each_with_index do |row, i|
+        min_heights[i] = min_row_height(row)
       end
       min_heights
     end
 
+    def min_row_height(row)
+      # Look through each of the row's columns for the column with the largest minimum height
+      largest = 0
+      row["columns"].each do |rk, rv|
+        min_lines = rv["minimum_lines"] || 1
+        font_size = rv["min_font_size"] || rv["font_size"] || 10
+        min_col_height = (@pdf.font.height_at(font_size) * min_lines).floor
+        largest = min_col_height if min_col_height > largest
+      end
+      largest
+    end
+
     def draw
       top_left = [@orientation[:x_offset], @orientation[:y_offset]]
       bounds = @orientation.slice(:width, :height)
@@ -112,8 +129,8 @@ module OpenProject::PdfExport::ExportCard
         @pdf.stroke_color '000000'
 
         # Draw rows
-        @row_elements.each do |row|
-          row.draw
+        @group_elements.each do |group|
+          group.draw
         end
 
         @pdf.stroke_bounds

lib/open_project/pdf_export/export_card/document_generator.rb

@@ -38,6 +38,7 @@ module OpenProject::PdfExport::ExportCard
 
     def render_pages
       card_padding = 10
+      group_padding = 5
       text_padding = 5
       card_width = pdf.bounds.width - (card_padding * 2)
       card_height = ((pdf.bounds.height - (card_padding * config.per_page )) / config.per_page) - (card_padding / config.per_page)
@@ -50,6 +51,7 @@ module OpenProject::PdfExport::ExportCard
           width: card_width,
           height: card_height,
           card_padding: card_padding,
+          group_padding: group_padding,
           text_padding: text_padding
         }
 

lib/open_project/pdf_export/export_card/group_element.rb

@@ -0,0 +1,52 @@
+
+module OpenProject::PdfExport::ExportCard
+  class GroupElement
+    include OpenProject::PdfExport::Exceptions
+
+    def initialize(pdf, orientation, config, work_package)
+      @pdf = pdf
+      @orientation = orientation
+      @config = config
+      @rows_config = config["rows"]
+      @work_package = work_package
+      @row_elements = []
+
+      current_y_offset = 0
+      row_heights = @orientation[:row_heights]
+
+      @rows_config.each_with_index do |(r_key, r_value), i|
+        current_y_offset += (row_heights[i - 1]) if i > 0
+        row_orientation = {
+          y_offset: @orientation[:height] - current_y_offset,
+          x_offset: 0,
+          width: @orientation[:width] - (@orientation[:group_padding] * 2),
+          height: row_heights[i],
+          text_padding: @orientation[:text_padding]
+        }
+
+        @row_elements << RowElement.new(@pdf, row_orientation, r_value, @work_package)
+      end
+    end
+
+    def draw
+      padding = @orientation[:group_padding]
+      top_left = [@orientation[:x_offset] + padding, @orientation[:y_offset]]
+      bounds = @orientation.slice(:width, :height)
+      bounds[:width] -= padding * 2
+
+      @pdf.bounding_box(top_left, bounds) do
+        @pdf.stroke_color '000000'
+
+        # Draw rows
+        @row_elements.each do |row|
+          row.draw
+        end
+
+        if (@config["has_border"] or false)
+          @pdf.stroke_bounds
+        end
+      end
+
+    end
+  end
+end

lib/open_project/pdf_export/export_card/row_element.rb

@@ -52,8 +52,23 @@ module OpenProject::PdfExport::ExportCard
 
     end
 
+    def self.prune_empty_groups(groups, wp)
+      # Prune rows in groups
+      groups.each do |gk, gv|
+        self.prune_empty_rows(gv["rows"], wp)
+      end
+
+      # Prune empty groups
+      groups.each do |gk, gv|
+        if gv["rows"].count == 0
+          groups.delete(gk)
+        end
+      end
+    end
+
     def self.prune_empty_rows(rows, wp)
-      rows.each_with_index do |(rk, rv), i|
+      rows.each do |rk, rv|
+        # TODO RS: This is still only checking the first column, need to check all
         ck, cv = rv["columns"].first
         if is_empty_column(ck, cv, wp)
           rows.delete(rk)

spec/controllers/export_card_configurations_controller_spec.rb

@@ -11,7 +11,7 @@ describe ExportCardConfigurationsController do
     @inactive_config = FactoryGirl.create(:inactive_export_card_configuration)
 
     @params = {}
-    @valid_rows_yaml = "rows:\n    row1:\n      has_border: false\n      columns:\n        id:\n          has_label: false\n          font_size: \"15\""
+    @valid_rows_yaml = "group1:\n  has_border: false\n  rows:\n    row1:\n      height: 50\n      priority: 1\n      columns:\n        id:\n          has_label: false"
   end
 
   describe 'Create' do

spec/export_card/document_generator_spec.rb

@@ -6,7 +6,7 @@ describe OpenProject::PdfExport::ExportCard::DocumentGenerator do
     per_page: 1,
     page_size: "A4",
     orientation: "landscape",
-    rows: "rows:\n    row1:\n      has_border: false\n      columns:\n        subject:\n          has_label: false\n          font_size: 15\n    row2:\n      has_border: false\n      columns:\n        non_existent:\n          has_label: false\n          font_size: 15\n          has_label: true\n          render_if_empty: true"
+    rows: "group1:\n  has_border: false\n  rows:\n    row1:\n      height: 50\n      priority: 1\n      columns:\n        subject:\n          has_label: false\n          font_size: 15\n    row2:\n      height: 50\n      priority: 1\n      columns:\n        non_existent:\n          has_label: true\n          font_size: 15\n          render_if_empty: true"
   })}
 
   let(:work_package1) { WorkPackage.new({

spec/factories/export_card_configuration_factory.rb

@@ -2,7 +2,7 @@
 FactoryGirl.define do
   factory :export_card_configuration do
     name "Config 1"
-    rows "rows:\n    row1:\n      has_border: false\n      columns:\n        id:\n          has_label: false\n          font_size: \"15\""
+    rows "group1:\n  has_border: false\n  rows:\n    row1:\n      height: 50\n      priority: 1\n      columns:\n        id:\n          has_label: false"
     per_page 5
     page_size "A4"
     orientation "landscape"
@@ -11,7 +11,7 @@ FactoryGirl.define do
   factory :default_export_card_configuration, :class => ExportCardConfiguration do
     name "Default"
     active true
-    rows "rows:\n    row1:\n      has_border: false\n      columns:\n        id:\n          has_label: false\n          font_size: \"15\""
+    rows "group1:\n  has_border: false\n  rows:\n    row1:\n      height: 50\n      priority: 1\n      columns:\n        id:\n          has_label: false"
     per_page 5
     page_size "A4"
     orientation "landscape"
@@ -28,7 +28,7 @@ FactoryGirl.define do
   factory :active_export_card_configuration, :class => ExportCardConfiguration do
     name "Config active"
     active true
-    rows "rows:\n    row1:\n      has_border: false\n      columns:\n        id:\n          has_label: false\n          font_size: \"15\""
+    rows "group1:\n  has_border: false\n  rows:\n    row1:\n      height: 50\n      priority: 1\n      columns:\n        id:\n          has_label: false"
     per_page 5
     page_size "A4"
     orientation "landscape"
@@ -37,7 +37,7 @@ FactoryGirl.define do
   factory :inactive_export_card_configuration, :class => ExportCardConfiguration do
     name "Config inactive"
     active false
-    rows "rows:\n    row1:\n      has_border: false\n      columns:\n        id:\n          has_label: false\n          font_size: \"15\""
+    rows "group1:\n  has_border: false\n  rows:\n    row1:\n      height: 50\n      priority: 1\n      columns:\n        id:\n          has_label: false"
     per_page 5
     page_size "A4"
     orientation "landscape"