From a29782d89ec15beb5e9721085c85d2f9d384bc84 Mon Sep 17 00:00:00 2001 From: Mark Cottman-Fields Date: Sat, 26 Mar 2016 22:18:49 +1000 Subject: [PATCH] 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 | 4 +-- app/models/site.rb | 11 ++++-- app/models/user.rb | 5 +++ app/views/devise/registrations/edit.html.haml | 2 +- .../shared/_time_zone_select_custom.html.haml | 10 ++++-- app/views/sites/_form.html.haml | 2 +- app/views/user_accounts/edit.html.haml | 2 +- lib/modules/custom_render.rb | 9 +++++ lib/modules/time_zone_helper.rb | 34 ++++++++++++++++--- spec/acceptance/sites_spec.rb | 22 +++++++++++- spec/models/site_spec.rb | 10 ++++++ spec/models/user_spec.rb | 12 +++++++ 12 files changed, 108 insertions(+), 15 deletions(-) create mode 100644 lib/modules/custom_render.rb diff --git a/app/models/project.rb b/app/models/project.rb index 7d37558d..a54e1ab0 100644 --- a/app/models/project.rb +++ b/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 diff --git a/app/models/site.rb b/app/models/site.rb index 2d89ce91..a004b52e 100644 --- a/app/models/site.rb +++ b/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 \ No newline at end of file diff --git a/app/models/user.rb b/app/models/user.rb index 047d82b0..b99f6c35 100644 --- a/app/models/user.rb +++ b/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 \ No newline at end of file diff --git a/app/views/devise/registrations/edit.html.haml b/app/views/devise/registrations/edit.html.haml index b8d0864e..03c09246 100644 --- a/app/views/devise/registrations/edit.html.haml +++ b/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 diff --git a/app/views/shared/_time_zone_select_custom.html.haml b/app/views/shared/_time_zone_select_custom.html.haml index da812e2d..99be2a38 100644 --- a/app/views/shared/_time_zone_select_custom.html.haml +++ b/app/views/shared/_time_zone_select_custom.html.haml @@ -1,10 +1,13 @@ --# 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 @@ -12,6 +15,9 @@ %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} diff --git a/app/views/sites/_form.html.haml b/app/views/sites/_form.html.haml index f367a8c5..cf90e5d7 100644 --- a/app/views/sites/_form.html.haml +++ b/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' \ No newline at end of file diff --git a/app/views/user_accounts/edit.html.haml b/app/views/user_accounts/edit.html.haml index 76447888..66ee8c54 100644 --- a/app/views/user_accounts/edit.html.haml +++ b/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'} diff --git a/lib/modules/custom_render.rb b/lib/modules/custom_render.rb new file mode 100644 index 00000000..40418395 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/lib/modules/time_zone_helper.rb b/lib/modules/time_zone_helper.rb index 9ff5f4f6..f3796942 100644 --- a/lib/modules/time_zone_helper.rb +++ b/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] + 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 diff --git a/spec/acceptance/sites_spec.rb b/spec/acceptance/sites_spec.rb index f947e7ff..3fa57d18 100644 --- a/spec/acceptance/sites_spec.rb +++ b/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 \ No newline at end of file diff --git a/spec/models/site_spec.rb b/spec/models/site_spec.rb index 97052469..3eb6972e 100644 --- a/spec/models/site_spec.rb +++ b/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'). diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index cb72f843..641c32a3 100644 --- a/spec/models/user_spec.rb +++ b/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.