Skip to content

Fix PhoneVerification::TelesignCallbacksController#notify returning 500s

What does this MR do and why?

Resolves https://gitlab.com/gitlab-org/modelops/anti-abuse/team-tasks/-/issues/520

Problem

The action errors out because of the use of find_by inside a scope. find_by doesn't return an ActiveRecord::Relation so when it eventually returns nil (i.e. when it can't find a record) the enclosing scope returns all (ActiveRecord::Relation) (source) instead of nil.

    def track_status_update_event
      # when there is no matching record for `payload.reference_id` the scope returns `all` instead of `nil`
      record = Users::PhoneNumberValidation.by_reference_id(payload.reference_id)

      # when find_by returns `nil` and the scope returns `all` as a result, the early return is skipped
      return unless record

      event = payload.failed_delivery? ? 'telesign_sms_delivery_failed' : 'telesign_sms_delivery_success'

      Gitlab::Tracking.event(
        'IdentityVerification::Phone',
        event,
        user: record.user, # this throws the "undefined method `user' for #<ActiveRecord::Relation"
        extra: { country_code: record.country, status: payload.status }
      )
    end

Solution

This MR moves the use of find_by from a scope (.by_reference_id) to a class method. The new class method (same name) returns either nil or a Users::PhoneNumberValidation as expected.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Eugie Limpin

Merge request reports