Fix using wrong tzinfo identifier and improve markdown rendering

Convert to tzinfo identifier when rendering site and user json, closes #270
DRY up markdown rendering, see #264

app/models/project.rb

@@ -37,11 +37,11 @@ class Project < ActiveRecord::Base
   validates_attachment_content_type :image, content_type: /\Aimage\/(jpg|jpeg|pjpeg|png|x-png|gif)\z/, message: 'file type %{value} is not allowed (only jpeg/png/gif images)'
 
   def description_html
-    ApplicationController.helpers.sanitize Kramdown::Document.new(description).to_html
+    CustomRender.render_markdown(self, :description)
   end
 
   def notes_html
-    ApplicationController.helpers.sanitize Kramdown::Document.new(notes).to_html
+    CustomRender.render_markdown(self, :notes)
   end
 
   # Define filter api settings

app/models/site.rb

@@ -45,6 +45,7 @@ class Site < ActiveRecord::Base
 
   validates_attachment_content_type :image, content_type: /^image\/(jpg|jpeg|pjpeg|png|x-png|gif)$/, message: 'file type %{value} is not allowed (only jpeg/png/gif images)'
 
+  validate :check_tz
   before_save :set_rails_tz, if: Proc.new { |site| site.tzinfo_tz_changed? }
 
   # commonly used queries
@@ -108,13 +109,13 @@ def update_location_obfuscated(current_user)
     @location_obfuscated = !is_owner
   end
 
+  # I don't know why Rubymine complains about Kramdown where ever I use it...
   def description_html
-    # I don't know why Rubymine complains about Kramdown where ever I use it...
-    ApplicationController.helpers.sanitize Kramdown::Document.new(description).to_html
+    CustomRender.render_markdown(self, :description)
   end
 
   def notes_html
-    ApplicationController.helpers.sanitize Kramdown::Document.new(notes).to_html
+    CustomRender.render_markdown(self, :notes)
   end
 
   def self.add_location_jitter(value, min, max)
@@ -241,4 +242,8 @@ def set_rails_tz
     TimeZoneHelper.set_rails_tz(self)
   end
 
+  def check_tz
+    TimeZoneHelper.validate_tzinfo_tz(self)
+  end
+
 end

app/models/user.rb

@@ -129,6 +129,7 @@ def excluded_login
 
   after_create :special_after_create_actions
 
+  validate :check_tz
   before_save :set_rails_tz, if: Proc.new { |user| user.tzinfo_tz_changed? }
 
   # Get the last time this user was seen.
@@ -286,4 +287,8 @@ def set_rails_tz
     TimeZoneHelper.set_rails_tz(self)
   end
 
+  def check_tz
+    TimeZoneHelper.validate_tzinfo_tz(self)
+  end
+
 end

app/views/devise/registrations/edit.html.haml

@@ -8,7 +8,7 @@
     = f.error_notification
     = f.input :user_name, required: true, autofocus: true
     = f.input :email, required: true, placeholder: 'Email'
-    = render partial: 'shared/time_zone_select_custom', locals: { f: f, attribute_name: :tzinfo_tz, model_name: resource_name }
+    = render partial: 'shared/time_zone_select_custom', locals: { f: f, attribute_name: :tzinfo_tz, model_name: resource_name, model: resource }
     - if devise_mapping.confirmable? && resource.pending_reconfirmation?
       %p= t('devise.registrations.edit.currently_waiting_confirmation_for_email', email: resource.unconfirmed_email)
     = f.input :password, autocomplete: 'off', hint: t('devise.registrations.edit.leave_blank_if_you_don_t_want_to_change_it'), required: false

app/views/shared/_time_zone_select_custom.html.haml

@@ -1,17 +1,23 @@
--# locals: f, model_name, attribute_name, full_width (defaults to false)
+-# locals: f, model_name, attribute_name, full_width (defaults to false), model
+- has_error = model.errors.any?
 - control_id = "#{model_name}_#{attribute_name}"
+- form_group_class = "#{control_id} #{has_error ? 'has-error' : ''}"
 - span_id = "#{control_id}_appended_info"
 - help_id = "#{control_id}_appended_help"
+- error_id = "#{control_id}_appended_error"
 - unless defined?(full_width)
   - full_width = false
-.form-group.string.optional{class: control_id }
+.form-group.string.optional{class: form_group_class }
   = f.label attribute_name, 'Time Zone', class: 'string optional control-label col-sm-3', for: control_id
   %div{class: (full_width ? 'col-sm-9' : 'col-sm-5')}
     .input-group
       = f.input_field attribute_name, type: 'text', autocomplete: 'off', class: 'form-control'
       %span.input-group-addon{id: span_id, title: 'Time zone abbreviation and current UTC offset'}
         (no match)
     %p.help-block{id: help_id }
+    - if has_error
+      %p.help-block{id: error_id}
+        = model.errors[attribute_name].join(', ')
 
 :javascript
   var mapping = #{TimeZoneHelper.mapping_zone_to_offset.to_json}

app/views/sites/_form.html.haml

@@ -9,7 +9,7 @@
         = f.input :latitude, input_html: { max: 90, min: -90 }
         = f.input :longitude, input_html: {  max: 180, min: -180 }
         = render partial: 'shared/image_upload', locals: { f: f,  model_instance: site, model_name: 'site' }
-        = render partial: '/shared/time_zone_select_custom', locals: { f: f, attribute_name: :tzinfo_tz, model_name: :site, full_width: true }
+        = render partial: '/shared/time_zone_select_custom', locals: { f: f, attribute_name: :tzinfo_tz, model_name: :site, full_width: true, model: site }
       .col-sm-5
         = render partial: 'shared/google_maps', locals: {markers: []}
     = f.button :submit_cancel, 'Submit', class: 'btn-default'

app/views/user_accounts/edit.html.haml

@@ -7,7 +7,7 @@
     = f.error_notification
     = f.input :user_name, required: true, autofocus: true, placeholder: 'User name'
     = f.input :email, required: true, placeholder: 'Email'
-    = render partial: 'shared/time_zone_select_custom', locals: { f: f, attribute_name: :tzinfo_tz, model_name: 'user' }
+    = render partial: 'shared/time_zone_select_custom', locals: { f: f, attribute_name: :tzinfo_tz, model_name: :user, model: @user }
     = f.input :password, autocomplete: 'off', required: false, placeholder: 'Password'
     .form-group.optional.pending-reconfirmation
       %label.optional.pending-reconfirmation.control-label.col-sm-3{for: 'pending-reconfirmation'}

lib/modules/custom_render.rb

@@ -0,0 +1,9 @@
+class CustomRender
+  class << self
+    def render_markdown(model, attribute)
+      value = model[attribute]
+      is_blank = value.blank?
+      is_blank ? nil : ApplicationController.helpers.sanitize(Kramdown::Document.new(value).to_html)
+    end
+  end
+end

lib/modules/time_zone_helper.rb

@@ -54,10 +54,10 @@ def ruby_tz_class(ruby_tz_name)
     end
 
     # Get the TZInfo Timezone class for the name.
-    # @param [string] tzinfo_tz_name
+    # @param [string] tzinfo_tz_identifier
     # @return [TZInfo::Timezone]
-    def tzinfo_class(tzinfo_tz_name)
-      tzinfo_tz_name.blank? ? nil : TZInfo::Timezone.get(tzinfo_tz_name)
+    def tzinfo_class(tzinfo_tz_identifier)
+      tzinfo_tz_identifier.blank? ? nil : TZInfo::Timezone.get(tzinfo_tz_identifier)
     end
 
     # Get the TZInfo Timezone equivalent to the Ruby TimeZone.
@@ -74,6 +74,31 @@ def tzinfo_to_ruby(tzinfo_tz_name)
       ActiveSupport::TimeZone::MAPPING.invert[tzinfo_tz_name]
     end
 
+    # Check if a tzinfo friendly id is valid.
+    # @param [Site, User] model
+    # @return [Boolean]
+    def validate_tzinfo_tz(model)
+      tzinfo_friendly = model.tzinfo_tz
+      is_invalid = to_identifier(tzinfo_friendly).blank?
+      if !model.tzinfo_tz.blank? && is_invalid
+        suggestions = tzinfo_friendly_did_you_mean(tzinfo_friendly)[0..2]
+        msg1 = "is not a recognised timezone ('#{tzinfo_friendly}'"
+        msg2 = suggestions.any? ? " - did you mean '#{suggestions.join("', '")}'?)" : ')'
+        model.errors[:tzinfo_tz] <<  msg1 + msg2
+      end
+    end
+
+    # Get suggestions for a tzinfo friendly name.
+    # @param [String] tzinfo_friendly
+    # @return [Array<String>]
+    def tzinfo_friendly_did_you_mean(tzinfo_friendly)
+      matches = []
+      TZInfo::Timezone.all.each do |tz|
+        matches << tz.friendly_identifier if tz.friendly_identifier.include?(tzinfo_friendly)
+      end
+      matches
+    end
+
     # Set rails time zone from tzinfo time zone.
     # @param [Site, User] model
     # @return [void]
@@ -96,7 +121,8 @@ def offset_seconds_to_formatted(utc_offset_seconds)
     end
 
     def info_hash(model)
-      tzinfo_tz_string = model.tzinfo_tz
+      # the model stores the tzinfo friendly_identifier
+      tzinfo_tz_string = TimeZoneHelper.to_identifier(model.tzinfo_tz)
       tzinfo_tz = tzinfo_class(tzinfo_tz_string)
 
       rails_tz_string = model.rails_tz

spec/acceptance/sites_spec.rb

@@ -524,7 +524,7 @@ def sites_body_params
 
   post '/sites/filter' do
     let(:authentication_token) { writer_token }
-    let!(:update_site_tz){
+    let!(:update_site_tz) {
       site2 = Creation::Common.create_site(writer_user, project)
       site2.rails_tz = 'Sydney'
       site2.save!
@@ -542,4 +542,24 @@ def sites_body_params
                              })
   end
 
+  post '/sites/filter' do
+    let(:authentication_token) { writer_token }
+    let!(:update_site_tz) {
+      site2 = FactoryGirl.build(:site, creator: writer_user)
+      site2.projects << project
+      site2.tzinfo_tz = 'Australia - Brisbane'
+      site2.save!
+    }
+    let(:raw_post) { {
+        'projection' => {
+            'include' => ['id', 'name']
+        }
+    }.to_json }
+    standard_request_options(:post, 'FILTER (as writer ensuring site timezone is valid)', :ok,
+                             {
+                                 data_item_count: 2,
+                                 response_body_content: ['"timezone_information":{"identifier_alt":"Brisbane","identifier":"Australia/Brisbane","friendly_identifier":"Australia - Brisbane"']
+                             })
+  end
+
 end

spec/models/site_spec.rb

@@ -115,6 +115,16 @@
     expect(projectHtml.notes_html).to eq(html)
   end
 
+  it 'should error on invalid timezone' do
+    expect {
+      FactoryGirl.create(:site, tzinfo_tz: 'blah')
+    }.to raise_error(ActiveRecord::RecordInvalid, "Validation failed: Tzinfo tz is not a recognised timezone ('blah')")
+  end
+
+  it 'should be valid for a valid timezone' do
+    expect(FactoryGirl.create(:site, tzinfo_tz: 'Australia - Brisbane')).to be_valid
+  end
+
   # this should pass, but the paperclip implementation of validate_attachment_content_type is buggy.
   # it { should validate_attachment_content_type(:image).
   #                 allowing('image/gif', 'image/jpeg', 'image/jpg','image/png').

spec/models/user_spec.rb

@@ -1,6 +1,18 @@
 require 'rails_helper'
 
 describe User, :type => :model do
+
+
+  it 'should error on invalid timezone' do
+    expect {
+      FactoryGirl.create(:user, tzinfo_tz: 'blah')
+    }.to raise_error(ActiveRecord::RecordInvalid, "Validation failed: Tzinfo tz is not a recognised timezone ('blah')")
+  end
+
+  it 'should be valid for a valid timezone' do
+    expect(FactoryGirl.create(:user, tzinfo_tz: 'Australia - Brisbane')).to be_valid
+  end
+
   #pending "add some examples to (or delete) #{__FILE__}"
 
   # this should pass, but the paperclip implementation of validate_attachment_content_type is buggy.