Refactor Users::ActivityService to not check for `username` to determine what object to use as the User
Currently Users::ActivityService
has this logic:
@user = if author.respond_to?(:username)
author
elsif author.respond_to?(:user)
author.user
end
@user = nil unless @user.is_a?(User)
This logic is super confusing, and even wrong. For example, if author
does respond to username
but is not a User
, then the code that follows it a few lines below might break. In fact, I'd argue that this service should not have an if
like this at all, and instead should just require the caller to pass in the right object.
This particular code played a role in the incident described in gitlab-org/release/retrospectives#1 (closed), and was fixed (using a hack) in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/20993.