Change all occurrences of ApplicationHelper#avatar_icon to use a User object where possible
In https://gitlab.com/gitlab-org/gitlab-ce/issues/40755 we discovered the second N+1 query problem (that I can remember) caused by using ApplicationHelper#avatar_icon with an Email instead of a User object. That is, instead of this:
avatar_icon(object.author)We were using:
avatar_icon(object.author_email)This will then lead to this method running an extra query to get the user that belongs to the given Email.
Grepping through the source code I see a lot of instances where we pass an Email when we instead should pass a User object. For example:
app/views/shared/members/_member.html.haml
11: = image_tag avatar_icon(user, 40), class: "avatar s40", alt: ''
39: = image_tag avatar_icon(member.invite_email, 40), class: "avatar s40", alt: ''Here on line 39 we're probably able to just pass member, assuming invite_email is the same as the member's Email address. There are also other cases such as:
app/views/shared/projects/_project.html.haml
20: = image_tag avatar_icon(project.creator.email, 40), class: "avatar s40", alt:''
app/views/projects/network/show.json.erb
12: icon: image_path(avatar_icon(c.author_email, 20))
app/views/issues/_issue.atom.builder
6: xml.media :thumbnail, width: "40", height: "40", url: image_url(avatar_icon(issue.author_email))We need to go over all places where we use avatar_icon and make sure that:
- We pass a
Userobject whenever possible - We ensure that these
Userobjects are eager loaded
Once done we need to slightly adjust avatar_icon. Having a single method that supports both a String and a User may be convenient but the above shows it's too easy to pass the wrong data. As such I would propose we split this method into avatar_icon_for_user and avatar_icon_for_email. This won't necessarily make it harder to make a mistake, but it will make it very explicit. This in turn means that it should be easier to find these problems, and a wee bit harder for a developer to use the wrong method.