Revert "uk polygons take II"

This reverts commit 7f37921.

Author: starswan

.simplecov

@@ -54,8 +54,8 @@ if ENV.fetch("COVERAGE", 0).to_i.positive?
     # .25% lower (branch) and .1% lower (line) than the test run for now
     #
     # possibly the tests are stable now?
-    # SD 27/01/26 branch coverage still appears to be ~ .1% unstable
-    # 97.47% (12400 / 12722) -> 322  87.11% (2785 / 3197) -> 197 + 215 = 412
-    minimum_coverage line: 97.45, branch: 87.1
+    # SD 16/01/26 branch coverage still appears to be ~ .1% unstable
+    # 97.48% (12352 / 12673) -> 322  86.37% (2776 / 3214) -> 185 + 225 = 410
+    minimum_coverage line: 97.44, branch: 87.12
   end
 end

app/helpers/maps_helper.rb

@@ -1,30 +1,26 @@
 module MapsHelper
   def vacancy_organisations_map_markers(vacancy)
-    vacancy.organisations.filter_map do |organisation|
-      if organisation.uk_geopoint?
-        {
-          id: vacancy.id,
-          parent_id: organisation.id,
-          geopoint: RGeo::GeoJSON.encode(GeoFactories.convert_sr27700_to_wgs84(organisation.uk_geopoint)).to_json,
-        }
-      end
+    vacancy.organisations.map do |organisation|
+      {
+        id: vacancy.id,
+        parent_id: organisation.id,
+        geopoint: RGeo::GeoJSON.encode(organisation.geopoint)&.to_json,
+      }
     end
   end
 
   def organisation_map_marker(organisation)
     [
       {
         parent_id: organisation.id,
-        geopoint: RGeo::GeoJSON.encode(GeoFactories.convert_sr27700_to_wgs84(organisation.uk_geopoint)).to_json,
+        geopoint: RGeo::GeoJSON.encode(organisation.geopoint)&.to_json,
       },
     ]
   end
 
   def organisation_map_can_be_displayed?(vacancy)
     return true unless vacancy.central_office?
 
-    # :nocov:
-    vacancy.organisation.uk_geopoint.present?
-    # :nocov:
+    vacancy.organisation.geopoint.present?
   end
 end

app/models/job_preferences/location.rb

@@ -6,27 +6,28 @@ class Location < ApplicationRecord
 
     self.table_name = "job_preferences_locations"
     belongs_to :job_preferences
-    self.ignored_columns += %i[area]
 
     validates :name, presence: true
     validates :radius, presence: true
     validates :uk_area, presence: true
 
     before_validation :set_area
 
-    scope :containing, ->(point) { where(arel_table[:uk_area].st_contains(point)) }
+    scope :containing, ->(point) { where("ST_Within(ST_GeomFromEWKT(?), area::geometry)", "SRID=4326;#{point.as_text}") }
 
     private
 
     def set_area
       if LocationPolygon.contain?(name)
-        # :nocov:
-        self.uk_area = LocationPolygon.buffered(radius).with_name(name).uk_area
-        # :nocov:
+        polygon = LocationPolygon.buffered(radius).with_name(name)
+        self.area = polygon.area
+        self.uk_area = polygon.uk_area
       else
         lat, long = Geocoding.new(name).coordinates.map(&:to_s)
         radius_meters = convert_miles_to_metres(Search::RadiusBuilder.new(name, radius).radius)
-        self.uk_area = GeoFactories.convert_wgs84_to_sr27700(GeoFactories::FACTORY_4326.point(long, lat).buffer(radius_meters))
+        geopoint = GeoFactories::FACTORY_4326.point(long, lat)
+        self.area = geopoint.buffer(radius_meters)
+        self.uk_area = GeoFactories.convert_wgs84_to_sr27700(geopoint).buffer(radius_meters)
       end
     end
   end

app/models/location_polygon.rb

@@ -1,13 +1,19 @@
 class LocationPolygon < ApplicationRecord
   extend DistanceHelper
 
+  # British National Grid SRID (EPSG:27700) is a projected coordinate system used for mapping in Great Britain.
+  # It provides coordinates in meters, which is useful for distance calculations, which we need
+  # for radius-based searches.
+  # It is significantly more accurate for distance calculations in Great Britain that EPSG:3857 (Web Mercator).
+  # EPSG:3857 distort distances and areas, especially as you move away from the equator. What would cause a multiplier
+  # between 1.5x and 1.7x for radius/buffer distances in our case to get the matches we would expect.
+  BRITISH_NATIONAL_GRID_SRID = 27700 # rubocop:disable Style/NumericLiterals
+
   validates :name, presence: true
 
   # Scope that expands any polygons returned by subsequent scopes by the provided radius
   # by overriding the `area` attribute with a buffered version of itself
-  scope :buffered, ->(radius_in_miles) { select("*, ST_Buffer(uk_area, #{convert_miles_to_metres(radius_in_miles || 0)}) AS uk_area") }
-
-  self.ignored_columns += %i[area centroid]
+  scope :buffered, ->(radius_in_miles) { select("*, ST_Buffer(area, #{convert_miles_to_metres(radius_in_miles || 0)}) AS area") }
 
   def self.with_name(location)
     find_by(name: mapped_name(location))
@@ -23,21 +29,48 @@ def self.mapped_name(location)
 
   def self.find_valid_for_location(location)
     polygon = with_name(location)
-    if polygon.present? && polygon.uk_area.invalid_reason.nil?
+    if polygon.present? && polygon.area.invalid_reason.nil?
       polygon
     end
   rescue RGeo::Error::InvalidGeometry
     nil
   end
 
   # Buffers the polygon's area by the given radius in metres and returns the resulting expanded area in geometry.
+  # Transformations are needed since the original area is a geographic type.
+  #
   # The "where & pick" approach is more efficient to retrieve a single computed value from the DB than loading the whole object.
+  #
+  # Why buffering in British National Grid SRID (27700) to then store it as SRID 4326?
+  # Buffering is best done in a projected coordinate system as it buffers in metres instead of degrees).
+  # After buffering, we transform to SRID: 4326 (lat/lon data). As we're doing spatial queries on this data, 4326 is more appropriate.
   def buffered_geometry_area(radius_in_metres)
+    wkb = self.class.where(id: id).pick(
+      Arel.sql(
+        "ST_AsBinary(
+            ST_Transform(
+              ST_Buffer(
+                ST_Transform(area::geometry, #{BRITISH_NATIONAL_GRID_SRID}),
+                #{radius_in_metres}
+              ),
+            4326)
+          )",
+      ),
+    )
+    return nil unless wkb
+
+    # Has to use this or the returned geometry gets a SRID: 0 even when set in the DB as 4326
+    RGeo::Cartesian.factory(srid: 4326).parse_wkb(wkb)
+  rescue RGeo::Error::InvalidGeometry
+    nil
+  end
+
+  # Buffers the polygon's area by the given radius in metres and returns the resulting expanded area in geometry.
+  # The "where & pick" approach is more efficient to retrieve a single computed value from the DB than loading the whole object.
+  def buffered_geometry_uk_area(radius_in_metres)
     wkb = self.class.where(id: id)
-        .pick(Arel.sql("ST_AsBinary(ST_Buffer(uk_area::geometry,#{radius_in_metres}))"))
+              .pick(Arel.sql("ST_AsBinary(ST_Buffer(uk_area::geometry,#{radius_in_metres}))"))
     # Has to use this or the returned geometry gets a SRID: 0 even when set in the DB as 27700
     GeoFactories::FACTORY_27700.parse_wkb(wkb) if wkb
-  rescue RGeo::Error::InvalidGeometry
-    nil
   end
 end

app/models/organisation.rb

@@ -24,8 +24,6 @@ class Organisation < ApplicationRecord
   has_many :jobseeker_profile_exclusions, class_name: "JobseekerProfileExcludedOrganisation"
   has_many :hidden_jobseeker_profiles, through: :jobseeker_profile_exclusions, source: :jobseeker_profile
 
-  self.ignored_columns += %i[geopoint]
-
   scope :not_closed, -> { where.not(establishment_status: "Closed") }
   scope :schools, -> { where(type: "School") }
   scope :school_groups, -> { where(type: "SchoolGroup") }

app/models/subscription.rb

@@ -20,8 +20,6 @@ class Subscription < ApplicationRecord
   # support_job_roles used to be called teaching_support_job_roles and non_teaching_support_job_roles in the past, and there are still active subscriptions with this name
   JOB_ROLE_ALIASES = %i[teaching_job_roles support_job_roles teaching_support_job_roles non_teaching_support_job_roles].freeze
 
-  self.ignored_columns += %i[area geopoint]
-
   def self.encryptor(serializer: :json_allow_marshal)
     key_generator_secret = SUBSCRIPTION_KEY_GENERATOR_SECRET
     key_generator_salt = SUBSCRIPTION_KEY_GENERATOR_SALT
@@ -98,16 +96,25 @@ def set_location_data!
 
   # A subscription with location area has a polygon area seat buffered by radius, no geopoint.
   def set_location_from_polygon(polygon, radius)
-    self.uk_area = polygon.buffered_geometry_area(self.class.convert_miles_to_metres(radius))
+    # Cast polygon area from geography to geometry and buffer by radius before storing
+    self.area = polygon.buffered_geometry_area(self.class.convert_miles_to_metres(radius))
+    self.geopoint = nil
+
+    self.uk_area = polygon.buffered_geometry_uk_area(self.class.convert_miles_to_metres(radius))
     self.uk_geopoint = nil
+
     self.radius_in_metres = self.class.convert_miles_to_metres(radius)
   end
 
   # A subscription with location coordinates has a geopoint, no area.
   def set_location_from_coordinates(coordinates, radius)
+    self.geopoint = RGeo::Cartesian.factory(srid: 4326).point(coordinates.second, coordinates.first)
+    self.area = nil
+
     geopoint = GeoFactories::FACTORY_4326.point(coordinates.second, coordinates.first)
     self.uk_geopoint = GeoFactories.convert_wgs84_to_sr27700 geopoint
     self.uk_area = nil
+
     self.radius_in_metres = self.class.convert_miles_to_metres(radius)
   end
 end

app/models/vacancy.rb

@@ -157,8 +157,6 @@ class Vacancy < ApplicationRecord
 
   after_save :update_conversation_searchable_content, if: -> { saved_change_to_job_title? }
 
-  self.ignored_columns += %i[geolocation]
-
   EQUAL_OPPORTUNITIES_PUBLICATION_THRESHOLD = 5
   EXPIRY_TIME_OPTIONS = %w[8:00 9:00 12:00 15:00 23:59].freeze
 
@@ -270,11 +268,11 @@ def salary_types
   end
 
   def distance_in_miles_to(search_coordinates)
-    if uk_geolocation.is_a? RGeo::Cartesian::MultiPointImpl
+    if geolocation.is_a? RGeo::Geographic::SphericalMultiPointImpl
       # if there are multiple geolocations then return the distance to the nearest one to the given search location
-      uk_geolocation.map { |geolocation| self.class.calculate_distance(search_coordinates, geolocation) }.min
+      geolocation.map { |geolocation| calculate_distance(search_coordinates, geolocation) }.min
     else
-      self.class.calculate_distance(search_coordinates, uk_geolocation)
+      calculate_distance(search_coordinates, geolocation)
     end
   end
 
@@ -305,12 +303,8 @@ def update_conversation_searchable_content
                 .find_each(&:update_searchable_content)
   end
 
-  class << self
-    def calculate_distance(search_coordinates, geolocation)
-      search_location = GeoFactories::FACTORY_4326.point(search_coordinates.second, search_coordinates.first)
-      search_point = GeoFactories.convert_wgs84_to_sr27700 search_location
-      search_point.distance(geolocation) / DistanceHelper::METRES_PER_MILE
-    end
+  def calculate_distance(search_coordinates, geolocation)
+    Geocoder::Calculations.distance_between(search_coordinates, [geolocation.latitude, geolocation.longitude])
   end
 
   def slug_candidates
@@ -323,12 +317,18 @@ def slug_candidates
   # In the former case, it gets an argument, which we don't need and thus ignore
   #
   def refresh_geolocation(_school_added_or_removed = nil)
-    self.uk_geolocation = if organisations.one?
-                            organisation.uk_geopoint
-                          else
-                            uk_points = organisations.filter_map(&:uk_geopoint)
-                            uk_points.presence && uk_points.first.factory.multi_point(uk_points)
-                          end
+    self.geolocation = if organisations.one?
+                         organisation.geopoint
+                       else
+                         points = organisations.filter_map(&:geopoint)
+                         points.presence && points.first.factory.multi_point(points)
+                       end
+    # self.uk_geolocation = if organisations.one?
+    #                         organisation.uk_geopoint
+    #                       else
+    #                         uk_points = organisations.filter_map(&:uk_geopoint)
+    #                         uk_points.presence && uk_points.first.factory.multi_point(uk_points)
+    #                       end
   end
 end
 # rubocop:enable Metrics/ClassLength

app/queries/location_query.rb

@@ -32,7 +32,7 @@ def nationwide_location?(query)
   def handle_polygon_location(field_name, polygon, radius, sort_by_distance)
     @scope = scope.joins("
       INNER JOIN location_polygons
-      ON ST_DWithin(#{field_name}, location_polygons.uk_area, #{radius})
+      ON ST_DWithin(#{field_name}, location_polygons.area, #{radius}, false)
     ").where("location_polygons.id = ?", polygon.id)
 
     sort_by_polygon_distance(field_name) if sort_by_distance
@@ -47,24 +47,23 @@ def handle_coordinates(field_name, query, radius, sort_by_distance)
     # suitable error instead. Refactor later!
     return scope.none if coordinates == [0, 0]
 
-    earth_point = GeoFactories::FACTORY_4326.point(coordinates.second, coordinates.first)
-    point = GeoFactories.convert_wgs84_to_sr27700 earth_point
-    @scope = scope.where("ST_DWithin(#{field_name}, ?, ?)", "SRID=#{point.srid};#{point}", radius)
+    point = "POINT(#{coordinates.second} #{coordinates.first})"
+    @scope = scope.where("ST_DWithin(#{field_name}, ?, ?, false)", point, radius)
 
     sort_by_coordinates_distance(field_name, point) if sort_by_distance
 
     scope
   end
 
   def sort_by_polygon_distance(field_name)
-    @scope = scope.select("vacancies.*, ST_Distance(#{field_name}, location_polygons.uk_centroid) AS distance")
+    @scope = scope.select("vacancies.*, ST_Distance(#{field_name}, location_polygons.centroid, false) AS distance")
                   # why not using 'distance' alias? is not defined when calling this query with a 'pluck'
-                  .order(Arel.sql("ST_Distance(#{field_name}, location_polygons.uk_centroid)"))
+                  .order(Arel.sql("ST_Distance(#{field_name}, location_polygons.centroid, false)"))
   end
 
   def sort_by_coordinates_distance(field_name, point)
-    @scope = scope.select("vacancies.*, ST_Distance(#{field_name}, 'SRID=#{point.srid};#{point}') AS distance")
+    @scope = scope.select("vacancies.*, ST_Distance(#{field_name}, '#{point}', false) AS distance")
                   # why not using 'distance' alias? is not defined when calling this query with a 'pluck'
-                  .order(Arel.sql("ST_Distance(#{field_name}, 'SRID=#{point.srid};#{point}')"))
+                  .order(Arel.sql("ST_Distance(#{field_name}, '#{point}', false)"))
   end
 end

app/queries/organisation_location_query.rb

@@ -4,6 +4,6 @@ def initialize(scope = Organisation.all)
   end
 
   def call(...)
-    super("organisations.uk_geopoint", ...)
+    super("organisations.geopoint", ...)
   end
 end

app/queries/subscription_vacancies_matching_query.rb

@@ -1,6 +1,14 @@
 class SubscriptionVacanciesMatchingQuery
   attr_reader :scope
 
+  # British National Grid SRID (EPSG:27700) is a projected coordinate system used for mapping in Great Britain.
+  # It provides coordinates in meters, which is useful for distance calculations, which we need
+  # for radius-based searches.
+  # It is significantly more accurate for distance calculations in Great Britain that EPSG:3857 (Web Mercator).
+  # EPSG:3857 distort distances and areas, especially as you move away from the equator. What would cause a multiplier
+  # between 1.5x and 1.7x for radius/buffer distances in our case to get the matches we would expect.
+  BRITISH_NATIONAL_GRID_SRID = 27700 # rubocop:disable Style/NumericLiterals
+
   # Builds the query to find the IDs for the vacancies matching the subscription criteria.
   # The subquery is needed to be able to combine our requirements into a valid single SQL statement:
   # - retrieve unique DISTINCT ON vacancy ids: As the location filter may return the same vacancy multiple times (vacancy for multiple orgs).
@@ -169,9 +177,9 @@ def location_filter(scope, subscription_location, subscription)
     # 'area_before_type_cast' and 'geopoint_before_type_cast' are used to avoid casting the fields into RGeo objects.
     # This reduces memory usage and speeds up the query. As we don't need to use the actual objects in Ruby code,
     # just need to know if they are present in DB.
-    if subscription.uk_area_before_type_cast.present?
+    if subscription.area_before_type_cast.present?
       location_by_area_filter(scope, subscription)
-    elsif subscription.uk_geopoint_before_type_cast.present? && subscription.radius_in_metres.present?
+    elsif subscription.geopoint_before_type_cast.present? && subscription.radius_in_metres.present?
       location_by_geopoint_filter(scope, subscription)
     else
       scope.none # Invalid location filter (having no area or geopoint) returns no matches
@@ -189,11 +197,9 @@ def location_filter(scope, subscription_location, subscription)
   # Including 'publish_on' in the distinct: The provided scope may be ordered by publish on date. When using distinct
   # with ordering, Postgres requires all selected columns to be included in the distinct clause or will fail during execution.
   def location_by_area_filter(scope, subscription)
-    subscriptions = Subscription.arel_table
-    organisations = Organisation.arel_table
     scope.joins("INNER JOIN subscriptions ON subscriptions.id = '#{subscription.id}'")
          .joins(:organisations)
-         .where(subscriptions[:uk_area].st_contains(organisations[:uk_geopoint]))
+         .where("ST_Contains(subscriptions.area, organisations.geopoint::geometry)")
   end
 
   # Filter vacancies where their organisations' geopoints fall within the subscription's radius from the subscription's
@@ -208,8 +214,8 @@ def location_by_area_filter(scope, subscription)
   def location_by_geopoint_filter(scope, subscription)
     scope.joins("INNER JOIN subscriptions ON subscriptions.id = '#{subscription.id}'")
          .joins(:organisations)
-         .where("ST_DWithin(organisations.uk_geopoint::geometry,
-                            subscriptions.uk_geopoint,
+         .where("ST_DWithin(ST_Transform(organisations.geopoint::geometry, #{BRITISH_NATIONAL_GRID_SRID}),
+                            ST_Transform(subscriptions.geopoint, #{BRITISH_NATIONAL_GRID_SRID}),
                             subscriptions.radius_in_metres)")
   end
 end