From ca8cd6400b8da768c9364a9d38e6e5f0bcde3287 Mon Sep 17 00:00:00 2001 From: Tim Cowlishaw Date: Mon, 26 Feb 2024 14:42:50 +0100 Subject: [PATCH] harmonize world map and device view Caching will need testing, as well as repercussions of removing lat/lng from user devices --- Gemfile | 2 +- Gemfile.lock | 5 ++- app/controllers/v0/devices_controller.rb | 14 +++--- app/models/device.rb | 54 ++++++++++++------------ app/views/v0/devices/_device.jbuilder | 29 +++++++------ app/views/v0/devices/world_map.jbuilder | 1 + app/views/v0/users/_user.jbuilder | 15 +++---- spec/requests/v0/devices_spec.rb | 2 +- spec/requests/v0/users_spec.rb | 10 ----- 9 files changed, 63 insertions(+), 69 deletions(-) create mode 100644 app/views/v0/devices/world_map.jbuilder diff --git a/Gemfile b/Gemfile index d53e80f7..e89d369b 100644 --- a/Gemfile +++ b/Gemfile @@ -8,7 +8,7 @@ gem 'doorkeeper', '~> 5' # To resize active storage images: # Revise if this is needed after Rails 6.0 gem 'image_processing' - +gem 'actionpack-action_caching' gem 'ancestry' gem 'api-pagination' gem 'api_cache' diff --git a/Gemfile.lock b/Gemfile.lock index a770b0e0..17030c92 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -53,6 +53,8 @@ GEM rack-test (>= 0.6.3) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.0, >= 1.2.0) + actionpack-action_caching (1.2.2) + actionpack (>= 4.0.0) actiontext (6.1.7.3) actionpack (= 6.1.7.3) activerecord (= 6.1.7.3) @@ -531,6 +533,7 @@ PLATFORMS ruby DEPENDENCIES + actionpack-action_caching ancestry api-pagination api_cache @@ -616,4 +619,4 @@ RUBY VERSION ruby 3.0.6p216 BUNDLED WITH - 2.4.13 + 2.5.6 diff --git a/app/controllers/v0/devices_controller.rb b/app/controllers/v0/devices_controller.rb index 514b350b..3f798f4a 100644 --- a/app/controllers/v0/devices_controller.rb +++ b/app/controllers/v0/devices_controller.rb @@ -1,10 +1,13 @@ +require 'actionpack/action_caching' module V0 class DevicesController < ApplicationController - + include ActionController::Caching before_action :check_if_authorized!, only: [:create] after_action :verify_authorized, except: [:index, :world_map, :fresh_world_map] + caches_action :world_map, expires_in: 1.minute + def show @device = Device.includes( :owner, :sensors,:tags).find(params[:id]) @@ -76,15 +79,12 @@ def destroy # debug method, must be refactored def fresh_world_map - render json: Device.for_world_map(current_user&.is_admin?) + @devices = Device.for_world_map + render :world_map end def world_map - unless params[:cachebuster] - expires_in 30.seconds, public: true # CRON cURL every 60 seconds to cache - end - - render json: Device.for_world_map(current_user&.is_admin?) + @devices = Device.for_world_map end private diff --git a/app/models/device.rb b/app/models/device.rb index 3a70821e..4fd7569f 100644 --- a/app/models/device.rb +++ b/app/models/device.rb @@ -69,6 +69,10 @@ class Device < ActiveRecord::Base end end + scope :for_world_map, -> { + where.not(latitude: nil).where.not(data: nil).where(is_test: false).includes(:owner, :tags) + } + def self.ransackable_attributes(auth_object = nil) if auth_object == :admin # admin can ransack on every attribute @@ -228,32 +232,30 @@ def self.geocode_all_without_location end end - def self.for_world_map(authorized=false) - Rails.cache.fetch("world_map", expires_in: 10.seconds) do - where - .not(latitude: nil) - .where.not(data: nil) - .where(is_test: false) - .includes(:owner,:tags) - .map do |device| - { - id: device.id, - name: device.name, - description: (device.description.present? ? device.description : nil), - owner_id: device.owner_id, - owner_username: device.owner_id ? device.owner_username : nil, - latitude: device.latitude, - longitude: device.longitude, - city: device.city, - hardware: device.hardware(authorized), - country_code: device.country_code, - state: device.state, - system_tags: device.system_tags, - user_tags: device.user_tags, - updated_at: device.updated_at, - last_reading_at: (device.last_reading_at.present? ? device.last_reading_at : nil) - } - end + def self.old_for_world_map(authorized=false) + where + .not(latitude: nil) + .where.not(data: nil) + .where(is_test: false) + .includes(:owner,:tags) + .map do |device| + { + id: device.id, + name: device.name, + description: (device.description.present? ? device.description : nil), + owner_id: device.owner_id, + owner_username: device.owner_id ? device.owner_username : nil, + latitude: device.latitude, + longitude: device.longitude, + city: device.city, + hardware: device.hardware(authorized), + country_code: device.country_code, + state: device.state, + system_tags: device.system_tags, + user_tags: device.user_tags, + updated_at: device.updated_at, + last_reading_at: (device.last_reading_at.present? ? device.last_reading_at : nil) + } end end diff --git a/app/views/v0/devices/_device.jbuilder b/app/views/v0/devices/_device.jbuilder index e4461f26..69f1012a 100644 --- a/app/views/v0/devices/_device.jbuilder +++ b/app/views/v0/devices/_device.jbuilder @@ -1,5 +1,8 @@ -with_owner = true unless local_assigns.has_key?(:with_owner) -with_data = true unless local_assigns.has_key?(:with_data) +local_assigns[:with_owner] = true unless local_assigns.has_key?(:with_owner) +local_assigns[:with_data] = true unless local_assigns.has_key?(:with_data) +local_assigns[:with_postprocessing] = true unless local_assigns.has_key?(:with_postprocessing) +local_assigns[:with_location] = true unless local_assigns.has_key?(:with_location) +local_assigns[:slim_owner] = false unless local_assigns.has_key?(:slim_owner) json.( device, @@ -8,7 +11,6 @@ json.( :name, :description, :state, - :postprocessing, :system_tags, :user_tags, :is_private, @@ -26,25 +28,26 @@ if authorized else json.merge! device_token: '[FILTERED]' end - -json.merge!(location: device.formatted_location) +json.merge!(postprocessing: device.postprocessing) if local_assigns[:with_postprocessing] +json.merge!(location: device.formatted_location) if local_assigns[:with_location] json.merge!(hardware: device.hardware(authorized)) -if with_owner && device.owner +if local_assigns[:with_owner] && device.owner json.owner do json.id device.owner.id json.uuid device.owner.uuid json.username device.owner.username - json.avatar device.owner.avatar - - json.profile_picture profile_picture_url(device.owner) - json.url device.owner.url - json.location device.owner.location - json.device_ids device.owner.cached_device_ids + + unless local_assigns[:slim_owner] + json.avatar device.owner.avatar + json.profile_picture profile_picture_url(device.owner) + json.location device.owner.location + json.device_ids device.owner.cached_device_ids + end end end -json.data device.formatted_data if with_data +json.data device.formatted_data if local_assigns[:with_data] diff --git a/app/views/v0/devices/world_map.jbuilder b/app/views/v0/devices/world_map.jbuilder new file mode 100644 index 00000000..9622cff9 --- /dev/null +++ b/app/views/v0/devices/world_map.jbuilder @@ -0,0 +1 @@ +json.array! @devices, partial: 'device', as: :device, local_assigns: { with_data: false, with_postprocessing: false, slim_owner: true } \ No newline at end of file diff --git a/app/views/v0/users/_user.jbuilder b/app/views/v0/users/_user.jbuilder index d0a403c7..ed70c911 100644 --- a/app/views/v0/users/_user.jbuilder +++ b/app/views/v0/users/_user.jbuilder @@ -12,7 +12,9 @@ json.(user, json.profile_picture profile_picture_url(user) -if current_user and (current_user.is_admin? or current_user == user) +authorized = current_user && current_user == user || current_user&.is_admin? + +if authorized json.merge! email: user.email json.merge! legacy_api_key: user.legacy_api_key else @@ -21,14 +23,7 @@ else end json.devices user.devices.filter { |d| - !d.is_private? || current_user == user || current_user&.is_admin? + !d.is_private? || authorized }.map do |device| - json.partial! "devices/device", device: device, with_data: false, with_owner: false - if current_user == user || current_user&.is_admin? - json.merge!( - location: device.location, - latitude: device.latitude, - longitude: device.longitude, - ) - end + json.partial! "devices/device", device: device, with_data: false, with_owner: false, with_location: authorized end diff --git a/spec/requests/v0/devices_spec.rb b/spec/requests/v0/devices_spec.rb index b5090fe5..c3148caa 100644 --- a/spec/requests/v0/devices_spec.rb +++ b/spec/requests/v0/devices_spec.rb @@ -24,7 +24,7 @@ expect(json.length).to eq(2) # expect(json[0]['name']).to eq(first.name) # expect(json[1]['name']).to eq(second.name) - expect(json[0].keys).to eq(%w(id uuid name description state postprocessing system_tags user_tags is_private notify_low_battery notify_stopped_publishing last_reading_at created_at updated_at device_token location hardware owner data)) + expect(json[0].keys).to eq(%w(id uuid name description state system_tags user_tags is_private notify_low_battery notify_stopped_publishing last_reading_at created_at updated_at device_token postprocessing location hardware owner data)) end describe "when not logged in" do diff --git a/spec/requests/v0/users_spec.rb b/spec/requests/v0/users_spec.rb index 65f0f576..8d2610e8 100644 --- a/spec/requests/v0/users_spec.rb +++ b/spec/requests/v0/users_spec.rb @@ -58,8 +58,6 @@ it "does not include the device locations" do j = api_get "users/testguy" expect(j["devices"].map { |d| d["location"]}.compact).to be_empty - expect(j["devices"].map { |d| d["latitude"]}.compact).to be_empty - expect(j["devices"].map { |d| d["longitude"]}.compact).to be_empty end end @@ -73,8 +71,6 @@ it "includes the device locations" do j = api_get "users/testguy?access_token=#{token.token}" expect(j["devices"].map { |d| d["location"]}.compact).not_to be_empty - expect(j["devices"].map { |d| d["latitude"]}.compact).not_to be_empty - expect(j["devices"].map { |d| d["longitude"]}.compact).not_to be_empty end end @@ -95,8 +91,6 @@ it "does not include the device locations" do j = api_get "users/testguy?access_token=#{requesting_token.token}" expect(j["devices"].map { |d| d["location"]}.compact).to be_empty - expect(j["devices"].map { |d| d["latitude"]}.compact).to be_empty - expect(j["devices"].map { |d| d["longitude"]}.compact).to be_empty end end @@ -117,8 +111,6 @@ it "does not include the device locations" do j = api_get "users/testguy?access_token=#{requesting_token.token}" expect(j["devices"].map { |d| d["location"]}.compact).to be_empty - expect(j["devices"].map { |d| d["latitude"]}.compact).to be_empty - expect(j["devices"].map { |d| d["longitude"]}.compact).to be_empty end end @@ -139,8 +131,6 @@ it "includes the device locations" do j = api_get "users/testguy?access_token=#{requesting_token.token}" expect(j["devices"].map { |d| d["location"]}.compact).not_to be_empty - expect(j["devices"].map { |d| d["latitude"]}.compact).not_to be_empty - expect(j["devices"].map { |d| d["longitude"]}.compact).not_to be_empty end end end