OCC-1171 refactor

app/models/service.rb

@@ -35,7 +35,7 @@ def consolidate
   # Only add this association after the db is loaded so we can check config
   # Changes to this config will require a serer restart... not ideal, maybe move it into a custom class method?
   if ActiveRecord::Base.connection.table_exists?(:configs) && Config.dashboard_mode == "travel_patterns"
-    has_many :purposes, through: :travel_patterns
+    has_many :purposes, -> { distinct }, through: :travel_patterns
   else
     has_and_belongs_to_many :purposes
   end
@@ -51,7 +51,9 @@ def consolidate
   accepts_nested_attributes_for :travel_pattern_services, allow_destroy: true, reject_if: :all_blank
 
   ### VALIDATIONS & CALLBACKS ###
-  validates_presence_of :name, :type, :agency
+  validates_presence_of :name, :type, :agency, :max_age, :min_age
+  validates_uniqueness_of :gtfs_agency_id, conditions: -> { where.not(archived: true, gtfs_agency_id: nil) }
+  validates :max_age, :min_age, :eligible_max_age, :eligible_min_age, numericality: { only_integer: true, greater_than_or_equal_to: 0 }
   validates_with FareValidator # For validating fare_structure and fare_details
   contact_fields phone: :phone, email: :email, url: :url
   validate :valid_booking_profile
@@ -168,20 +170,31 @@ def self.available_by_filter_for(filter, trip)
 
   ## Secondary Availability Scopes ##
 
+  # Allowing the schedules' id to be nil includes services with no schedules set
   scope :available_by_schedule_for, -> (trip) do
-    # Either no schedules are set, or there is a schedule that includes the trip time
-    where( id: no_schedules | with_matching_schedule(trip) )
+    where(schedules: {id: nil})
+      .or(where(schedules: {
+        day: trip.wday,
+        start_time: 0..trip.secs,
+        end_time: trip.secs..DAY_LENGTH
+      }))
+      .left_joins(:schedules)
+      .distinct
   end
 
   scope :available_by_geography_for, -> (trip) do
     available_by_start_area_for(trip)
-    .available_by_end_area_for(trip)
-    .available_by_start_or_end_area_for(trip)
-    .available_by_trip_within_area_for(trip)
+      .available_by_end_area_for(trip)
+      .available_by_start_or_end_area_for(trip)
+      .available_by_trip_within_area_for(trip)
   end
 
+  # Allowing the purposes' id to be nil includes services with no purposes selected
   scope :available_by_purpose_for, -> (trip) do
-    trip.purpose ? available_by_purpose(trip.purpose) : all
+    return self.all if trip.purpose_id.nil?
+    where(purposes: { id: [nil, trip.purpose_id] })
+      .left_joins(:purposes)
+      .distinct
   end
 
   scope :available_by_eligibility_for, -> (trip) do
@@ -202,16 +215,12 @@ def self.available_by_filter_for(filter, trip)
     end
   end
 
-  # Includes service if either it has no eligibility requirements, or the user
-  # meets at least one of its eligibility requirements
+  # Either no eligibilities are set, or the user meets an eligibility requirement
   scope :accepts_eligibility_of, -> (user) do
-    # Either no eligibilities are set, or the user meets an eligibility requirement
-    where( id: no_eligibilities | with_met_eligibilities(user) )
-  end
-
-  # find services available by a trips purpose
-  scope :available_by_purpose, -> (purpose) do
-    where(id: no_purposes | with_matching_purpose(purpose))
+    where(eligibilities: { 
+      id: [nil, *user.confirmed_user_eligibilities.pluck(:eligibility_id)] 
+    }).left_joins(:eligibilities)
+      .distinct
   end
 
   # Builds instance methods for determining if record falls within given scope
@@ -222,18 +231,19 @@ def self.available_by_filter_for(filter, trip)
       :available_by_accommodation_for,
       :available_by_eligibility_for,
       :accommodates,
-      :accepts_eligibility_of,
-      :available_by_purpose
+      :accepts_eligibility_of
 
   ## Other Scopes ##
 
   # Accommodation, Eligibility, and Purpose scopes filter by services that have one or more of the passed ids
   scope :with_accommodations, -> (accommodation_ids) do
     where(id: joins(:accommodations).where(accommodations: {id: accommodation_ids}).pluck(:id).uniq)
   end
+
   scope :with_eligibilities, -> (eligibility_ids) do
     where(id: joins(:eligibilities).where(eligibilities: {id: eligibility_ids}).pluck(:id).uniq)
   end
+
   scope :with_purposes, -> (purpose_ids) do
     where(id: joins(:purposes).where(purposes: {id: purpose_ids}).pluck(:id).uniq)
   end
@@ -392,16 +402,6 @@ def booking_detail_to_array(key)
     where( id: no_region(:trip_within_area) | with_containing_trip_within_area(trip) )
   end
 
-  # Returns IDs of Services with no eligibility requirements
-  def self.no_eligibilities
-    includes(:eligibilities).where(eligibilities: {id: nil}).pluck(:id)
-  end
-
-  # Returns IDs of Services with at least one eligibility requirement met by user
-  def self.with_met_eligibilities(user)
-    joins(:eligibilities).where(eligibilities: {id: user.confirmed_eligibilities.pluck(:id)}).pluck(:id)
-  end
-
   # Returns all services that provide a given accommodation
   scope :accommodates_by_code, -> (code) { joins(:accommodations).where(accommodations: {code: code}) }
   scope :accommodates_accommodation, -> (accommodation) { joins(:accommodations).where(accommodations: {id: accommodation.id})}
@@ -411,30 +411,6 @@ def self.accommodates_all_needs(user)
     user.accommodations.map {|acc| Service.accommodates_accommodation(acc).pluck(:id)}.reduce(&:&)
   end
 
-  # Returns IDs of Services with no schedules set
-  def self.no_schedules
-    includes(:schedules).where(schedules: {id: nil}).pluck(:id)
-  end
-
-  # Returns IDs of Services with no purposes set
-  def self.no_purposes
-    includes(:purposes).where(purposes: {id: nil}).pluck(:id)
-  end
-
-  # Returns IDs of Services with a schedule that includes the trip time
-  def self.with_matching_schedule(trip)
-    joins(:schedules).where(schedules: {
-      day: trip.wday,
-      start_time: 0..trip.secs,
-      end_time: trip.secs..DAY_LENGTH
-    }).pluck(:id)
-  end
-
-  # Returns IDs of Services with a purpose that includes the trip's purpose
-  def self.with_matching_purpose(purpose)
-    joins(:purposes).where(purposes: {id: purpose.id}) #Surely we can do this without comparing codes
-  end
-
   # Returns IDs of Services with no region of given association type
   def self.no_region(region_type)
     includes(region_type).where(regions: { id: nil }).pluck(:id)

app/services/external_api_ambassadors/otp.rb

@@ -312,11 +312,9 @@ def each &block
       @legs.each &block
     end
 
-    # Returns first instance of an attribute from the legs, or the first leg if
-    # no attribute is passed
-    def first(attribute=nil)
-      return self.pluck(attribute).first if attribute
-      @legs.first || {}
+    # Returns first instance of an attribute from the legs
+    def detect &block
+      @legs.detect &block
     end
 
     # Returns an array of all non-nil instances of the given value in the legs

app/services/external_api_ambassadors/otp_ambassador.rb

@@ -32,7 +32,7 @@ def initialize(
       trip, 
       trip_types=TRIP_TYPE_DICTIONARY.keys, 
       http_request_bundler=HTTPRequestBundler.new, 
-      services=Service.published.available_for(trip, {only_filters: [:eligibility, :accommodation]})
+      services=Service.published
     )
 
     @trip = trip
@@ -142,30 +142,27 @@ def ensure_response(trip_type)
   # Converts an OTP itinerary hash into a set of 1-Click itinerary attributes
   def convert_itinerary(otp_itin, trip_type)
     associate_legs_with_services(otp_itin)
-    service_id = otp_itin.legs.first('serviceId')
-
-    # Reject paratransit itineraries unless we recognize the service it belongs to.
-    # This is because unknown paratransit services may have eligibility requirements or lack accommodations.
-    if service_id || trip_type.to_sym != :paratransit
-      return {
-        start_time: Time.at(otp_itin["startTime"].to_i/1000).in_time_zone,
-        end_time: Time.at(otp_itin["endTime"].to_i/1000).in_time_zone,
-        transit_time: get_transit_time(otp_itin, trip_type),
-        walk_time: get_walk_time(otp_itin, trip_type),
-        wait_time: get_wait_time(otp_itin),
-        walk_distance: get_walk_distance(otp_itin),
-        cost: extract_cost(otp_itin, trip_type),
-        legs: otp_itin.legs.to_a,
-        trip_type: trip_type, #TODO: Make this smarter
-        service_id: service_id
-      }
-    else
-      return nil
-    end
+    service_id = otp_itin.legs
+                          .detect{|leg| leg['serviceId'].present? }
+                          &.fetch('serviceId', nil)
+
+    return {
+      start_time: Time.at(otp_itin["startTime"].to_i/1000).in_time_zone,
+      end_time: Time.at(otp_itin["endTime"].to_i/1000).in_time_zone,
+      transit_time: get_transit_time(otp_itin, trip_type),
+      walk_time: get_walk_time(otp_itin, trip_type),
+      wait_time: get_wait_time(otp_itin),
+      walk_distance: get_walk_distance(otp_itin),
+      cost: extract_cost(otp_itin, trip_type),
+      legs: otp_itin.legs.to_a,
+      trip_type: trip_type, #TODO: Make this smarter
+      service_id: service_id
+    }
   end
 
   # Modifies OTP Itin's legs, inserting information about 1-Click services
   def associate_legs_with_services(otp_itin)
+    otp_itin.legs ||= []
     otp_itin.legs = otp_itin.legs.map do |leg|
       svc = get_associated_service_for(leg)
 
@@ -190,6 +187,7 @@ def associate_legs_with_services(otp_itin)
 
   def get_associated_service_for(leg)
     svc = nil
+    leg ||= {}
     gtfs_agency_id = leg['agencyId']
     gtfs_agency_name = leg['agencyName']
 

app/services/trip_planner.rb

@@ -66,7 +66,7 @@ def plan
   # Set up external API ambassadors
   def prepare_ambassadors
     # Set up external API ambassadors for route finding and fare calculation
-    @router ||= OTPAmbassador.new(@trip, @trip_types, @http_request_bundler, @available_services[:transit])
+    @router ||= OTPAmbassador.new(@trip, @trip_types, @http_request_bundler, @available_services[:transit].or(@available_services[:paratransit]))
     @taxi_ambassador ||= TFFAmbassador.new(@trip, @http_request_bundler, services: @available_services[:taxi])
     @uber_ambassador ||= UberAmbassador.new(@trip, @http_request_bundler)
     @lyft_ambassador ||= LyftAmbassador.new(@trip, @http_request_bundler)
@@ -198,17 +198,21 @@ def build_bicycle_itineraries
 
   # Builds paratransit itineraries for each service, populates transit_time based on OTP response
   def build_paratransit_itineraries
-    return [] unless @available_services[:paratransit] # Return an empty array if no paratransit services are available
+    return [] unless @available_services[:paratransit].present? # Return an empty array if no paratransit services are available
 
     # gtfs flex can load paratransit itineraries but not all otp instances have flex
     router_paratransit_itineraries = []
     if Config.open_trip_planner_version == 'v2'
-      otp_itineraries = build_fixed_itineraries(:paratransit)
+      # Paratransit itineraries must belong to a service
+      # This ensures we respect accomodations and eligibilities
+      otp_itineraries = build_fixed_itineraries(:paratransit).select{ |itin|
+        itin.service_id.present?
+      }
 
       # paratransit itineraries can return just transit since we also look for a mixed
       # filter these out
       # then set itineraries that are a mix of paratransit and transit mixed
-      router_paratransit_itineraries += otp_itineraries.map{|itin|
+      router_paratransit_itineraries += otp_itineraries.map{ |itin|
         no_paratransit = true
         has_transit = false
         itin.legs.each do |leg|

config/initializers/version.rb

@@ -1 +1 @@
-OneclickCore::Application.config.version='v1.18.6'
+OneclickCore::Application.config.version='v1.18.7'

db/migrate/20230322101725_add_uniqueness_indexes_to_join_tables.rb

@@ -0,0 +1,8 @@
+class AddUniquenessIndexesToJoinTables < ActiveRecord::Migration[5.0]
+  def change
+    add_index :accommodations_users, [:user_id, :accommodation_id], unique: true
+    add_index :accommodations_services, [:service_id, :accommodation_id], unique: true, name: 'idx_services_accommodations_on_service_id_and_accommodation_id'
+    add_index :eligibilities_services, [:service_id, :eligibility_id], unique: true
+    add_index :purposes_services, [:service_id, :purpose_id], unique: true
+  end
+end

db/schema.rb

@@ -10,11 +10,12 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20230130085256) do
+ActiveRecord::Schema.define(version: 20230322101725) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
   enable_extension "postgis"
+  enable_extension "pg_stat_statements"
 
   create_table "accommodations", force: :cascade do |t|
     t.string   "code",                     null: false
@@ -28,6 +29,7 @@
     t.integer "service_id",       null: false
     t.integer "accommodation_id", null: false
     t.index ["accommodation_id"], name: "index_accommodations_services_on_accommodation_id", using: :btree
+    t.index ["service_id", "accommodation_id"], name: "idx_services_accommodations_on_service_id_and_accommodation_id", unique: true, using: :btree
     t.index ["service_id"], name: "index_accommodations_services_on_service_id", using: :btree
   end
 
@@ -41,6 +43,7 @@
     t.integer "user_id",          null: false
     t.integer "accommodation_id", null: false
     t.index ["accommodation_id"], name: "index_accommodations_users_on_accommodation_id", using: :btree
+    t.index ["user_id", "accommodation_id"], name: "index_accommodations_users_on_user_id_and_accommodation_id", unique: true, using: :btree
     t.index ["user_id"], name: "index_accommodations_users_on_user_id", using: :btree
   end
 
@@ -193,6 +196,7 @@
     t.integer "service_id",     null: false
     t.integer "eligibility_id", null: false
     t.index ["eligibility_id"], name: "index_eligibilities_services_on_eligibility_id", using: :btree
+    t.index ["service_id", "eligibility_id"], name: "index_eligibilities_services_on_service_id_and_eligibility_id", unique: true, using: :btree
     t.index ["service_id"], name: "index_eligibilities_services_on_service_id", using: :btree
   end
 
@@ -416,6 +420,7 @@
     t.integer "service_id", null: false
     t.integer "purpose_id", null: false
     t.index ["purpose_id"], name: "index_purposes_services_on_purpose_id", using: :btree
+    t.index ["service_id", "purpose_id"], name: "index_purposes_services_on_service_id_and_purpose_id", unique: true, using: :btree
     t.index ["service_id"], name: "index_purposes_services_on_service_id", using: :btree
   end