diff --git a/lib/suma/lime/sync_trips_from_report.rb b/lib/suma/lime/sync_trips_from_report.rb index c97a52bb2..4cba29ece 100644 --- a/lib/suma/lime/sync_trips_from_report.rb +++ b/lib/suma/lime/sync_trips_from_report.rb @@ -7,6 +7,8 @@ class Suma::Lime::SyncTripsFromReport include Appydays::Loggable + class InvalidReportRow < StandardError; end + DEFAULT_VEHICLE_TYPE = Suma::Mobility::ESCOOTER DEFAULT_CUTOFF = 2.weeks @@ -14,6 +16,7 @@ class Suma::Lime::SyncTripsFromReport START_TIME = "START_TIME" END_TIME = "END_TIME" USER_EMAIL = "USER_EMAIL" + EMAIL_ADDRESS = "EMAIL_ADDRESS" START_LATITUDE = "START_LATITUDE" START_LONGITUDE = "START_LONGITUDE" END_LATITUDE = "END_LATITUDE" @@ -43,13 +46,14 @@ def run b64content = attachment.fetch("Content") content = Base64.decode64(b64content) filename = attachment.fetch("Name", nil) - log_context = { + Suma::Logutil.with_tags( filename:, message_id: row.fetch(:message_id), message_timestamp: row.fetch(:timestamp), subject: row.fetch(:subject), - } - self.run_for_report(content, log_context:) + ) do + self.run_for_report(content) + end end end end @@ -64,7 +68,13 @@ def dataset end # Import the text parsed as a CSV, and return the number of rows imported. - def run_for_report(txt, log_context: {}) + def run_for_report(txt) + Suma::Logutil.with_tags do + self._run_for_report(txt) + end + end + + def _run_for_report(txt) # The trip report generator is adding summary lines to the bottom that have the wrong newline. # I am guessing they are doing this with some special code and don't know CSV format line endings are meaningful. txt = txt.gsub("\n\n", "\r\n") @@ -72,15 +82,17 @@ def run_for_report(txt, log_context: {}) csv = CSV.parse(txt, headers: true) rescue CSV::MalformedCSVError => e fragment = txt[..1000] - self.logger.warn("lime_report_malformed_csv", {text_fragment: fragment, **log_context}, e) - Sentry.capture_message("Lime trip report CSV malformed") do |scope| - scope.set_extras(text_fragment: fragment, error: e, **log_context) + Suma::Logutil.with_tags(text_fragment: fragment) do + self.logger.warn("lime_report_malformed_csv", e) + Suma::Logutil.sentry_scope.set_extras(error: e) + Sentry.capture_message("Lime trip report CSV malformed") end return end import_successful = true imported_rows = 0 missing_member_rows = 0 + invalid_rows = 0 existing_rows = 0 blank_rows = 0 row_count = 0 @@ -92,16 +104,16 @@ def run_for_report(txt, log_context: {}) # We can't really predict these reliably, so if the trip token or email is missing, # assume the row is broken and just skip it. trip_token = row[TRIP_TOKEN] - user_email = row[USER_EMAIL] + user_email = row[USER_EMAIL] || row[EMAIL_ADDRESS] if trip_token.blank? || user_email.blank? blank_rows += 1 next end # Ignore older spreadsheets, not worth supporting them. unless row[LIME_ACCESS_COST] - self.logger.warn("lime_report_invalid_csv", **log_context) - Sentry.capture_message("Lime trip report with invalid headers") do |scope| - scope.set_extras(headers: row.headers, **log_context) + Suma::Logutil.with_tags(headers: row.headers) do + self.logger.warn("lime_report_invalid_csv") + Sentry.capture_message("Lime trip report with invalid headers") end # Break/return instead of processing the next row. import_successful = false @@ -111,44 +123,47 @@ def run_for_report(txt, log_context: {}) account: Suma::AnonProxy::VendorAccount.where(configuration_id: Suma::Lime.trip_report_vendor_configuration_id), external_program_id: user_email, ) - if reg_ds.empty? - log_args = {member_contact_email: user_email, trip_token:, **log_context} - self.logger.warn("lime_report_missing_member", log_args) - Sentry.capture_message("Lime trip taken by unknown user") do |scope| - scope.set_extras(log_args) + Suma::Logutil.with_tags(member_contact_email: user_email, trip_token:) do + if reg_ds.empty? + self.logger.warn("lime_report_missing_member") + Sentry.capture_message("Lime trip taken by unknown user") + missing_member_rows += 1 + next end - missing_member_rows += 1 - next - end - if (existing = Suma::Mobility::Trip[external_trip_id: trip_token]) - # If we already have this trip recorded, we want to noop, or update it if configured. - # We do NOT handle the race condition with a unique violation on external_trip_id, - # since in that case we assume we're dealing with the same code and it isn't worth the complexity. - if Suma::Lime.trip_report_overwrite - new_values = self.parse_trip_from_row(row)[:receipt] - existing.update(new_values.trip.values) + if (existing = Suma::Mobility::Trip[external_trip_id: trip_token]) + # If we already have this trip recorded, we want to noop, or update it if configured. + # We do NOT handle the race condition with a unique violation on external_trip_id, + # since in that case we assume we're dealing with the same code, and it isn't worth the complexity. + if Suma::Lime.trip_report_overwrite + new_values = self.parse_trip_from_row(row, user_email:)[:receipt] + existing.update(new_values.trip.values) + end + existing_rows += 1 + self.logger.debug("lime_trip_exists", overwritten: Suma::Lime.trip_report_overwrite) + next + end + self.logger.debug("lime_importing_trip") + begin + self.import_trip_from_row(row, user_email:) + imported_rows += 1 + rescue InvalidReportRow + self.logger.error("lime_report_missing_field") + Sentry.capture_message("Lime trip row missing necessary fields") + invalid_rows += 1 end - existing_rows += 1 - next end - imported_rows += 1 - self.import_trip_from_row(row) end # Already logged, don't re-log as successful return unless import_successful self.logger.info("lime_report_import_successful", - blank_rows:, missing_member_rows:, imported_rows:, existing_rows:, row_count:, - **log_context,) + blank_rows:, missing_member_rows:, imported_rows:, existing_rows:, row_count:, invalid_rows:,) rescue StandardError => e - self.logger.error("lime_report_import_unhandled_error", log_context, e) - Sentry.with_scope do |scope| - scope&.set_extras(**log_context) - end + self.logger.error("lime_report_import_unhandled_error", e) raise end - def parse_trip_from_row(row) - registration = Suma::AnonProxy::VendorAccountRegistration.find!(external_program_id: row.fetch(USER_EMAIL)) + def parse_trip_from_row(row, user_email:) + registration = Suma::AnonProxy::VendorAccountRegistration.find!(external_program_id: user_email) vendor_config = registration.account.configuration program = Suma::Enumerable.one!(vendor_config.programs) pricing = Suma::Enumerable.one!(program.pricings) @@ -161,8 +176,8 @@ def parse_trip_from_row(row) return {receipt:, program:} end - def import_trip_from_row(row) - r = self.parse_trip_from_row(row) + def import_trip_from_row(row, user_email:) + r = self.parse_trip_from_row(row, user_email:) Suma::Mobility::TripImporter.import(receipt: r[:receipt], program: r[:program], logger: self.logger) end @@ -174,19 +189,23 @@ def import_trip_from_row(row) # The only price column we keep track of is ACTUAL_COST; # this is what Lime charges suma. def parse_row_to_receipt(row, rate:) - r = Suma::Mobility::TripImporter::Receipt.new - r.trip.set( + fields = { vehicle_id: row.fetch(TRIP_TOKEN), vehicle_type: DEFAULT_VEHICLE_TYPE, began_at: parsetime(row.fetch(START_TIME)), ended_at: parsetime(row.fetch(END_TIME)), - begin_lat: BigDecimal(row.fetch(START_LATITUDE)), - begin_lng: BigDecimal(row.fetch(START_LONGITUDE)), - end_lat: BigDecimal(row.fetch(END_LATITUDE)), - end_lng: BigDecimal(row.fetch(END_LONGITUDE)), + begin_lat: parsedecimal(row.fetch(START_LATITUDE)), + begin_lng: parsedecimal(row.fetch(START_LONGITUDE)), + end_lat: parsedecimal(row.fetch(END_LATITUDE)), + end_lng: parsedecimal(row.fetch(END_LONGITUDE)), external_trip_id: row.fetch(TRIP_TOKEN), - our_cost: Monetize.parse(row.fetch(COST_TO_SUMA)), - ) + our_cost: parsemoney(row.fetch(COST_TO_SUMA)), + } + + raise InvalidReportRow if fields.values.any?(&:nil?) + + r = Suma::Mobility::TripImporter::Receipt.new + r.trip.set(fields) r.charged_at = r.trip.began_at r.paid_off_platform_amount = Money.zero r.subsidized_off_platform_amount = Money.zero @@ -207,12 +226,16 @@ def parse_row_to_receipt(row, rate:) return r end + def parsedecimal(v) = v.blank? ? nil : BigDecimal(v) + def parsemoney(v) = v.blank? ? nil : Monetize.parse(v) + # Lime's time formats are ridiculous. # We perform our own time parsing, since the alternatives around being clever # or making assumptions are even worse. # We see a mixture of AM/PM, 12 hour clock, seconds, date formats, etc. # See specs for examples of all the formats we've seen. def parsetime(t) + return nil if t.blank? if /\d\d\d\d \d\d:\d\d:\d\d GMT[+-]\d/.match?(t) # Handle "Mon Dec 08 2025 09:38:00 GMT+0100 (Central European Standard Time)" format specially. # No idea why just some rides in the same report use a format like this. diff --git a/lib/suma/logutil.rb b/lib/suma/logutil.rb new file mode 100644 index 000000000..6cc046cbc --- /dev/null +++ b/lib/suma/logutil.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Suma::Logutil + class << self + def with_tags(tags={}, &) + Sentry.with_scope do |scope| + scope&.set_extras(tags) + SemanticLogger.named_tagged(tags, &) + end + end + + # @return [Sentry::Scope] + def sentry_scope = Sentry.get_current_scope || Sentry::Scope.new + end +end diff --git a/lib/suma/marketing/sms_dispatch.rb b/lib/suma/marketing/sms_dispatch.rb index dac15503c..8dc663de0 100644 --- a/lib/suma/marketing/sms_dispatch.rb +++ b/lib/suma/marketing/sms_dispatch.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "suma/external_links" +require "suma/logutil" require "suma/marketing" require "suma/postgres/model" @@ -32,7 +33,7 @@ def send_all broadcast_id: dispatch.sms_broadcast.id, broadcast: dispatch.sms_broadcast.label, } - SemanticLogger.named_tagged(log_tags) do + Suma::Logutil.with_tags(log_tags) do if dispatch.sms_broadcast.sending_number.blank? self.logger.info("sms_dispatch_no_marketing_number") dispatch.cancel.save_changes diff --git a/lib/suma/message/delivery.rb b/lib/suma/message/delivery.rb index 8a66b57da..09d123a23 100644 --- a/lib/suma/message/delivery.rb +++ b/lib/suma/message/delivery.rb @@ -3,6 +3,7 @@ require "suma/admin_actions" require "suma/admin_linked" require "suma/external_links" +require "suma/logutil" require "suma/message" require "suma/postgres/model" @@ -49,7 +50,7 @@ def body_with_mediatype!(mt) def send! return nil if self.sent_at || self.aborted_at - SemanticLogger.named_tagged(message_delivery_id: self.id, to: self.to) do + Suma::Logutil.with_tags(message_delivery_id: self.id, to: self.to) do self.db.transaction do self.lock! return nil if self.sent_at || self.aborted_at diff --git a/spec/suma/lime/sync_trips_from_report_spec.rb b/spec/suma/lime/sync_trips_from_report_spec.rb index af2816ef2..59c292954 100644 --- a/spec/suma/lime/sync_trips_from_report_spec.rb +++ b/spec/suma/lime/sync_trips_from_report_spec.rb @@ -58,6 +58,33 @@ ) end + it "uses 3/2/2026 headers" do + txt = <<~CSV + TRIP_TOKEN,CONSEQUENCE,START_TIME,END_TIME,START_LATITUDE,START_LONGITUDE,END_LATITUDE,END_LONGITUDE,REGION_NAME,USER_TOKEN,EMAIL_ADDRESS,TRIP_DURATION_MINUTES,TRIP_DISTANCE_MILES,COST_TO_SUMA,UNLOCK_COST,DURATION_COST,COST_PER_MINUTE,LIME_ACCESS_COST,STANDARD_FEE,PERCENT_DISCOUNT_RATE,,,,,, + RK7PCB7HWXRK5,warning,Mon Mar 09 2026 00:08:00 GMT+0100 (Central European Standard Time),Mon Mar 09 2026 00:39:00 GMT+0100 (Central European Standard Time),45.57,-122.68,45.58,-122.69,Portland,UHAYR2RQDV7HF,m1@in.mysuma.org,32,0.79,0,0.5,2.24,0.07,2.74,14.02,80,,,,,,#REF! + CSV + described_class.new.run_for_report(txt) + expect(Suma::Mobility::Trip.all).to contain_exactly( + have_attributes( + vendor_service: be === program.pricings.first.vendor_service, + began_at: match_time("2026-03-08 23:08:00Z"), + ended_at: match_time("2026-03-08 23:39:00Z"), + member: be === member, + external_trip_id: "RK7PCB7HWXRK5", + ), + ) + end + + it "logs if a needed cell is blank" do + txt = <<~CSV + TRIP_TOKEN,CONSEQUENCE,START_TIME,END_TIME,START_LATITUDE,START_LONGITUDE,END_LATITUDE,END_LONGITUDE,REGION_NAME,USER_TOKEN,EMAIL_ADDRESS,TRIP_DURATION_MINUTES,TRIP_DISTANCE_MILES,COST_TO_SUMA,UNLOCK_COST,DURATION_COST,COST_PER_MINUTE,LIME_ACCESS_COST,STANDARD_FEE,PERCENT_DISCOUNT_RATE,,,,,, + RK7PCB7HWXRK5,warning,,Mon Mar 09 2026 00:39:00 GMT+0100 (Central European Standard Time),45.57,-122.68,45.58,-122.69,Portland,UHAYR2RQDV7HF,m1@in.mysuma.org,32,0.79,0,0.5,2.24,0.07,2.74,14.02,80,,,,,,#REF! + CSV + expect_sentry_capture(type: :message, arg_matcher: eq("Lime trip row missing necessary fields")) + described_class.new.run_for_report(txt) + expect(Suma::Mobility::Trip.all).to be_blank + end + it "only looks at the configured vendor configuration" do Suma::Lime.trip_report_vendor_configuration_id = 0 txt = <<~CSV