Refactor Users::ActivityService to not check for username to determine what object to use as the User

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

• Close this issue

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.