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.
-
I have evaluated the MR acceptance checklist for this MR.