diff --git a/Gemfile b/Gemfile index 6f5a99f09..8a21b9fbe 100644 --- a/Gemfile +++ b/Gemfile @@ -12,7 +12,7 @@ gem 'heroku-api' # for Heroku deployment - as described in Ap. A of ELLS book group :development, :test do - gem 'delorean' + gem 'timecop' gem 'metric_fu' gem 'database_cleaner', '1.0.1' gem 'launchy' @@ -87,4 +87,4 @@ gem 'url_validator' gem 'rails_autolink' -gem "paranoia", "~> 2.0" +gem "paranoia", "~> 2.0" \ No newline at end of file diff --git a/Gemfile.lock b/Gemfile.lock index 1ad76e58b..b2be99962 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -115,8 +115,6 @@ GEM database_cleaner (1.0.1) debug_inspector (0.0.2) debugger-linecache (1.2.0) - delorean (2.1.0) - chronic devise (3.4.1) bcrypt (~> 3.0) orm_adapter (~> 0.1) @@ -352,6 +350,7 @@ GEM thor (0.19.1) thread_safe (0.3.4) tilt (1.4.1) + timecop (0.7.3) twitter-bootstrap-rails (2.2.8) actionpack (>= 3.1) execjs @@ -402,7 +401,6 @@ DEPENDENCIES cucumber-rails cucumber-rails-training-wheels database_cleaner (= 1.0.1) - delorean devise (~> 3.4.0) devise_invitable (~> 1.3.0) execjs @@ -433,6 +431,7 @@ DEPENDENCIES simplecov sinatra-base therubyracer + timecop twitter-bootstrap-rails (= 2.2.8) uglifier (= 2.5.3) underscore-rails diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index dc8ebd329..cc69be589 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -2,7 +2,9 @@ class ApplicationController < ActionController::Base protect_from_forgery - before_filter :store_location, :assign_footer_page_links + before_filter :store_location, + :assign_footer_page_links + include CustomErrors # To prevent infinite redirect loops, only requests from white listed diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb index 6648f2265..f7fbc9b54 100644 --- a/app/controllers/organisations_controller.rb +++ b/app/controllers/organisations_controller.rb @@ -136,8 +136,8 @@ def build_map_markers(organisations) partial: 'shared/custom_marker', locals: { attrs: org.gmaps4rails_marker_attrs } ), - index: org.not_updated_recently_or_has_no_owner? ? -1 : 1, - type: org.not_updated_recently_or_has_no_owner? ? 'small_org' : 'large_org' + index: org.recently_updated_and_has_owner ? 1 : -1, + type: org.recently_updated_and_has_owner ? 'large_org' : 'small_org' ) end end diff --git a/app/models/organisation.rb b/app/models/organisation.rb index 01709fe8a..8c22b4a21 100644 --- a/app/models/organisation.rb +++ b/app/models/organisation.rb @@ -35,7 +35,7 @@ class Organisation < ActiveRecord::Base after_save :uninvite_users, if: ->{ email_changed? } def not_updated_recently? - updated_at < 365.day.ago + updated_at < 1.year.ago end def uninvite_users @@ -89,14 +89,14 @@ def self.filter_by_categories(category_ids) end def gmaps4rails_marker_attrs - if not_updated_recently_or_has_no_owner? - ['https://maps.gstatic.com/intl/en_ALL/mapfiles/markers2/measle.png', - 'data-id' => id, - class: 'measle'] - else + if recently_updated_and_has_owner ['http://mt.googleapis.com/vt/icon/name=icons/spotlight/spotlight-poi.png', 'data-id' => id, class: 'marker'] + else + ['https://maps.gstatic.com/intl/en_ALL/mapfiles/markers2/measle.png', + 'data-id' => id, + class: 'measle'] end end diff --git a/app/services/map_marker_json.rb b/app/services/map_marker_json.rb index 2de808c22..be980308a 100644 --- a/app/services/map_marker_json.rb +++ b/app/services/map_marker_json.rb @@ -1,6 +1,6 @@ module MapMarkerJson def self.build(organisations) - Gmaps4rails.build_markers(organisations) do |org, marker| + Gmaps4rails.build_markers(Queries::Organisations.add_recently_updated_and_has_owner(organisations)) do |org, marker| yield org, marker end.select do |marker| marker[:lat].present? && marker[:lng].present? diff --git a/app/services/queries/organisations.rb b/app/services/queries/organisations.rb index 646fb0839..7f82623d9 100644 --- a/app/services/queries/organisations.rb +++ b/app/services/queries/organisations.rb @@ -20,5 +20,32 @@ def self.search_by_keyword_and_category(parsed_params) end end + FORMAT = '%Y-%m-%d %H:%M:%S.%N' + + def self.add_recently_updated_and_has_owner(organisations) + if organisations.method(:select).arity == 0 + return add_recently_update_and_has_owner_to_enumerable(organisations) + end + one_year_ago = Time.current.advance(years: -1) + recently_updated = "organisations.updated_at > '#{one_year_ago.strftime(FORMAT)}'" + # recently_updated = "organisations.updated_at > #{one_year_ago.strftime}" + has_owner = "organisations.id IN (SELECT users.organisation_id FROM users)" + condition = + "CASE WHEN #{recently_updated} AND #{has_owner} THEN TRUE ELSE FALSE END" + organisations + .select("organisations.*, (#{condition}) as recently_updated_and_has_owner") + end + + private + def self.add_recently_update_and_has_owner_to_enumerable(organisations) + organisations.each do |o| + o.instance_eval %{ + def recently_updated_and_has_owner + !not_updated_recently? && !self.users.empty? + end + } + end + return organisations + end end end diff --git a/features/home_page/map.feature b/features/home_page/map.feature index 287b29448..c1d4b5a77 100644 --- a/features/home_page/map.feature +++ b/features/home_page/map.feature @@ -31,18 +31,17 @@ Feature: Map of local charities @time_travel @javascript Scenario Outline: Organisation map has small icon for organisations updated more than 365 days ago - Given I travel "" days into the future + Given I travel a year plus "" days into the future And I visit the home page Then the organisation "Youth UK" should have a icon - Examples: + Examples: |days | size | - | 2 | large| - |100 | large| - |200 | large| - |364 | large| - |365 | small| - |366 | small| - |500 | small| + | -10 | large| + | -1 | large| + | 0 | small| + | 1 | small| + | 10 | small| + |100 | small| @javascript Scenario: Organisation map has small icon for organisation with no users diff --git a/features/individual_charity_pages/remind_charity_admin_to_update_page.feature b/features/individual_charity_pages/remind_charity_admin_to_update_page.feature index 722970565..4a6e9b9bb 100644 --- a/features/individual_charity_pages/remind_charity_admin_to_update_page.feature +++ b/features/individual_charity_pages/remind_charity_admin_to_update_page.feature @@ -17,7 +17,7 @@ Feature: Charity worker is reminded annually to edit own charity profile @javascript @time_travel Scenario: Org owner is reminded to update details after a year - Given I travel "365" days into the future + Given I travel a year plus "0" days into the future And I visit the home page And I click "Login" When I sign in as "superadmin@friendly.org" with password "pppppppp" with javascript @@ -25,7 +25,7 @@ Feature: Charity worker is reminded annually to edit own charity profile @javascript @time_travel Scenario: Org owner is not reminded to update details prior to a year - Given I travel "364" days into the future + Given I travel a year plus "-1" days into the future And I visit the home page And I click "Login" When I sign in as "superadmin@friendly.org" with password "pppppppp" with javascript diff --git a/features/step_definitions/basic_steps.rb b/features/step_definitions/basic_steps.rb index d435b3b9c..fd9bb4e5f 100644 --- a/features/step_definitions/basic_steps.rb +++ b/features/step_definitions/basic_steps.rb @@ -2,8 +2,10 @@ require 'uri-handler' include ApplicationHelper -Then(/^I travel "(.*?)" days into the future$/) do |days| - Delorean.time_travel_to "#{days} days from now" +Then(/^I travel a year plus "(.*?)" days into the future$/) do |days| + Timecop.travel( + Time.current.advance(years: 1, days: days.to_i) + ) end Then(/^I should see the "(.*?)" image linked to "(.*?)"$/) do |image_alt, link| within("a[href='#{link}']") do diff --git a/features/support/env.rb b/features/support/env.rb index 9da48836b..ebf872001 100644 --- a/features/support/env.rb +++ b/features/support/env.rb @@ -13,7 +13,7 @@ require 'rack_session_access/capybara' require 'factory_girl_rails' require 'aruba/cucumber' -require 'delorean' +require 'timecop' Dir['../../spec/factories/*.rb'].each {|file| require_relative file } # https://github.com/jnicklas/capybara/commit/4f805d5a1c42da29ed32ab0371e24add2dc08af1 @@ -25,7 +25,7 @@ # prefer to use XPath just remove this line and adjust any selectors in your # steps to use the XPath syntax. Capybara.default_selector = :css -Capybara.default_wait_time = 20 +Capybara.default_wait_time = 3 Capybara.javascript_driver = :webkit @@ -94,5 +94,5 @@ end After('@time_travel') do - Delorean.back_to_1985 + Timecop.return end diff --git a/spec/controllers/organisations_controller_spec.rb b/spec/controllers/organisations_controller_spec.rb index d64b8abf4..5aa3511d8 100644 --- a/spec/controllers/organisations_controller_spec.rb +++ b/spec/controllers/organisations_controller_spec.rb @@ -15,8 +15,9 @@ def double_organisation(stubs={}) describe "#build_map_markers" do render_views - let(:org) { create :organisation } - subject { JSON.parse(controller.send(:build_map_markers, org)).first } + let!(:org) { create :organisation } + let(:org_relation){Organisation.all} + subject { JSON.parse(controller.send(:build_map_markers, org_relation)).first } it { expect(subject['lat']).to eq org.latitude } it { expect(subject['lng']).to eq org.longitude } it { expect(subject['infowindow']).to include org.id.to_s } @@ -24,7 +25,7 @@ def double_organisation(stubs={}) it { expect(subject['infowindow']).to include org.description } context 'markers without coords omitted' do let!(:org) { create :organisation, address: '150 pinner rd', latitude: nil, longitude: nil } - it { expect(JSON.parse(controller.send(:build_map_markers, org))).to be_empty } + it { expect(JSON.parse(controller.send(:build_map_markers, org_relation))).to be_empty } end end @@ -138,7 +139,6 @@ def double_organisation(stubs={}) before(:each) do @user = double("User") allow(@user).to receive(:pending_org_admin?) - allow(Organisation).to receive(:find).with('37') { real_org} allow(@user).to receive(:can_edit?) allow(@user).to receive(:can_delete?) allow(@user).to receive(:can_create_volunteer_ops?) @@ -147,7 +147,7 @@ def double_organisation(stubs={}) end it 'should use a two_column layout' do - get :show, :id => '37' + get :show, :id => real_org.id.to_s expect(response).to render_template 'layouts/two_columns' end @@ -155,28 +155,27 @@ def double_organisation(stubs={}) markers='my markers' @org = real_org expect(controller).to receive(:build_map_markers).and_return(markers) - expect(Organisation).to receive(:find).with('37') { @org } - get :show, :id => '37' - expect(assigns(:organisation)).to be(real_org) + get :show, :id => real_org.id.to_s + expect(assigns(:organisation)).to eq(real_org) expect(assigns(:markers)).to eq(markers) end context "editable flag is assigned to match user permission" do it "user with permission leads to editable flag true" do expect(@user).to receive(:can_edit?).with(real_org).and_return(true) - get :show, :id => 37 + get :show, id: real_org.id.to_s expect(assigns(:editable)).to be(true) end it "user without permission leads to editable flag false" do expect(@user).to receive(:can_edit?).with(real_org).and_return(true) - get :show, :id => 37 + get :show, id: real_org.id.to_s expect(assigns(:editable)).to be(true) end it 'when not signed in editable flag is nil' do allow(controller).to receive(:current_user).and_return(nil) - get :show, :id => 37 + get :show, id: real_org.id.to_s expect(assigns(:editable)).to be_nil end end @@ -186,19 +185,19 @@ def double_organisation(stubs={}) allow(@user).to receive(:can_edit?) expect(@user).to receive(:can_request_org_admin?).with(real_org).and_return(true) allow(controller).to receive(:current_user).and_return(@user) - get :show, :id => 37 + get :show, :id => real_org.id.to_s expect(assigns(:grabbable)).to be(true) end it 'assigns grabbable to false when user cannot request org superadmin status' do allow(@user).to receive(:can_edit?) expect(@user).to receive(:can_request_org_admin?).with(real_org).and_return(false) allow(controller).to receive(:current_user).and_return(@user) - get :show, :id => 37 + get :show, :id => real_org.id.to_s expect(assigns(:grabbable)).to be(false) end it 'when not signed in grabbable flag is true' do allow(controller).to receive(:current_user).and_return(nil) - get :show, :id => 37 + get :show, :id => real_org.id.to_s expect(assigns(:grabbable)).to be true end end @@ -207,13 +206,13 @@ def double_organisation(stubs={}) context 'depends on can_create_volunteer_ops?' do it 'true' do expect(@user).to receive(:can_create_volunteer_ops?) { true } - get :show, :id => 37 + get :show, :id => real_org.id.to_s expect(assigns(:can_create_volunteer_op)).to be true end it 'false' do expect(@user).to receive(:can_create_volunteer_ops?) { false } - get :show, :id => 37 + get :show, :id => real_org.id.to_s expect(assigns(:can_create_volunteer_op)).to be false end end @@ -221,7 +220,7 @@ def double_organisation(stubs={}) it 'will not be called when current user is nil' do allow(controller).to receive_messages current_user: nil expect(@user).not_to receive :can_create_volunteer_ops? - get :show, :id => 37 + get :show, :id => real_org.id.to_s expect(assigns(:can_create_volunteer_op)).to be nil end end diff --git a/spec/controllers/volunteer_ops_controller_spec.rb b/spec/controllers/volunteer_ops_controller_spec.rb index d0847a759..3b9112b25 100644 --- a/spec/controllers/volunteer_ops_controller_spec.rb +++ b/spec/controllers/volunteer_ops_controller_spec.rb @@ -29,7 +29,7 @@ render_views let(:org) { create :organisation } let!(:op) { create :volunteer_op, organisation: org } - subject { JSON.parse(controller.send(:build_map_markers, org)).first } + subject { JSON.parse(controller.send(:build_map_markers, [org])).first } it { expect(subject['lat']).to eq org.latitude } it { expect(subject['lng']).to eq org.longitude } it { expect(subject['infowindow']).to include org.id.to_s } @@ -39,7 +39,7 @@ it { expect(subject['infowindow']).to include op.description } context 'markers without coords omitted' do let(:org) { create :organisation, address: "0 pinnner road", latitude: nil, longitude: nil } - it { expect(JSON.parse(controller.send(:build_map_markers, org))).to be_empty } + it { expect(JSON.parse(controller.send(:build_map_markers, [org]))).to be_empty } end end diff --git a/spec/models/organisation_spec.rb b/spec/models/organisation_spec.rb index ee65e22e9..98ef8e4c0 100644 --- a/spec/models/organisation_spec.rb +++ b/spec/models/organisation_spec.rb @@ -59,9 +59,15 @@ end describe "#gmaps4rails_marker_attrs" do + + def build_org_with_computed_fields_and_updated_at org, updated_at = nil + org.update_attributes(updated_at: updated_at) unless updated_at.nil? + Queries::Organisations.add_recently_updated_and_has_owner(Organisation.where(id: org.id)).first + end context 'no user' do it 'returns small icon when no associated user' do - expect(@org1.gmaps4rails_marker_attrs).to eq(["https://maps.gstatic.com/intl/en_ALL/mapfiles/markers2/measle.png", {"data-id"=>@org1.id, :class=>"measle"}]) + expect(build_org_with_computed_fields_and_updated_at(@org1).gmaps4rails_marker_attrs).to eq(["https://maps.gstatic.com/intl/en_ALL/mapfiles/markers2/measle.png", + {"data-id"=>@org1.id, :class=>"measle"}]) end end @@ -72,25 +78,30 @@ @org1.users << [usr] @org1.save! end - after(:each) do - allow(Time).to receive(:now).and_call_original - end it 'returns large icon when there is an associated user' do - expect(@org1.gmaps4rails_marker_attrs).to eq( ["http://mt.googleapis.com/vt/icon/name=icons/spotlight/spotlight-poi.png", {"data-id"=>@org1.id,:class=>"marker"}]) + expect(build_org_with_computed_fields_and_updated_at(@org1).gmaps4rails_marker_attrs).to eq( ["http://mt.googleapis.com/vt/icon/name=icons/spotlight/spotlight-poi.png", + {"data-id"=>@org1.id,:class=>"marker"}]) end - [365, 366, 500].each do |days| + [ 365, 366, 500 ].each do |days| it "returns small icon when update is #{days} days old" do - future_time = Time.at(Time.now + days.day) - allow(Time).to receive(:now){future_time} - expect(@org1.gmaps4rails_marker_attrs).to eq(["https://maps.gstatic.com/intl/en_ALL/mapfiles/markers2/measle.png", {"data-id"=>@org1.id, :class=>"measle"}]) + # adds generous 5 second pad for query to run + past_time = Time.current.advance(days: -days).advance(seconds: -5) + expect( + build_org_with_computed_fields_and_updated_at( + @org1, past_time + ).gmaps4rails_marker_attrs + ).to eq([ + "https://maps.gstatic.com/intl/en_ALL/mapfiles/markers2/measle.png", + {"data-id"=>@org1.id, :class=>"measle"} + ]) end end [ 2, 100, 200, 364].each do |days| it "returns large icon when update is only #{days} days old" do - future_time = Time.at(Time.now + days.day) - allow(Time).to receive(:now){future_time} - expect(@org1.gmaps4rails_marker_attrs).to eq( ["http://mt.googleapis.com/vt/icon/name=icons/spotlight/spotlight-poi.png", {"data-id"=>@org1.id, :class=>"marker"} ]) + past_time = Time.at(Time.now - days.day) + expect(build_org_with_computed_fields_and_updated_at(@org1, past_time).gmaps4rails_marker_attrs).to eq( ["http://mt.googleapis.com/vt/icon/name=icons/spotlight/spotlight-poi.png", + {"data-id"=>@org1.id, :class=>"marker"} ]) end end end diff --git a/spec/services/queries/organisations_spec.rb b/spec/services/queries/organisations_spec.rb index 82b791c5c..38c466ba8 100644 --- a/spec/services/queries/organisations_spec.rb +++ b/spec/services/queries/organisations_spec.rb @@ -6,6 +6,8 @@ # not initialized! let(:category3) { create(:category, :charity_commission_id => 307) } + + let!(:org1) do create( :organisation, @@ -123,4 +125,32 @@ def run_spec it { expect(run_spec).to include org1, org2, org3 } end + context '#add_computed_recently_updated_and_has_owner' do + let(:user1){create(:user)} + let(:user2){create(:user, email: "blah@blah.org")} + before{org1.update_attributes(updated_at: 2.year.ago); org1.users << user1; org2.users << user2; } + context 'computes correctly when array passed in' do + it do + orgs = described_class.add_recently_updated_and_has_owner([org1, org2, org3]) + expect(orgs[0].recently_updated_and_has_owner).to be false + expect(orgs[1].recently_updated_and_has_owner).to be true + expect(orgs[2].recently_updated_and_has_owner).to be false + end + end + context 'computes correctly when scope passed in' do + it do + orgs = described_class.add_recently_updated_and_has_owner(Organisation.all) + orgs.each do |o| + case o.name + when org1.name + expect(o.recently_updated_and_has_owner).to be false + when org2.name + expect(o.recently_updated_and_has_owner).to be true + when org3.name + expect(o.recently_updated_and_has_owner).to be false + end + end + end + end + end end