feat: Added FXA profile image

[wassafshahzad]

Description

Added FXA avatar scope and the avatar will be loaded on runtime using the user_gravatar_url if available. Also changed the property of UserModel from gravatar_url to avatar_url

Linked Issue

Closes #4017

Screenshot

Caveats

There several caveats with this implementation

• Users using the Gravatar Image with FXA accounts will have there avatars replaced with FXA one since we prioritize FXA ones. The reason is because we are sending the Gravatar image url back to the template. To prioritize it we will first have to send a http request to the url and if it fails default to FXA.
• The class rounded on img tag does make the FXA image look a bit boxy, see screenshot.

[mathjazz]

That update looks good.

I'm concerned about the number of times we hit the DB, because we call avatar_url() from so many places so many times.

Let's give it a test on pontoon.allizom.org. We can also check how it performs before we dive into any optimizations.

[flodolo]

I'm concerned about the number of times we hit the DB, because we call avatar_url() from so many places so many times.

That is one query for each user every time we ask for a group of users? If that's the case, worth prefetch the social account data?

[mathjazz]

That is one query for each user every time we ask for a group of users?

One query for each avatar.

[flodolo]

One query for each avatar.

Yes, but it's one additional query for each user, e.g. when we display a list of suggestions/translations, look at the contributors page?

[mathjazz]

Yes, one query for each avatar (not one for the entire group of avatars on the page).

[wassafshahzad]

One query for each avatar.

Yes, but it's one additional query for each user, e.g. when we display a list of suggestions/translations, look at the contributors page?

Since each user will be rendered with his avatar. We can add another dynamic property to the user model with the name fxa avatar which is a cached property initilized once per user model. Rather than calling the db for every avatar call, we can check the fxa_avatar property and if its not none; return that else follow the defauly path.

[mathjazz]

We could avoid the N+1 by prefetching FXA social accounts with Prefetch(..., to_attr="fxa_accounts"), and have avatar_url() read from that attribute when available.

[wassafshahzad]

We could avoid the N+1 by prefetching FXA social accounts with Prefetch(..., to_attr="fxa_accounts"), and have avatar_url() read from that attribute when available.

@mathjazz, Could we also not save the avatar from the fxa social account and save into fxa_avatar property rather than caching the social account instance, something like this

@cached_property
def fxa_avatar(self):
    fxa = self.social_auth.filter(provider="fxa").first()
    return fxa.extra_data.get("avatar") if fxa else None

[wassafshahzad]

@mathjazz Which approach would you like me to implement. As soon as I have your approval I can update the PR

[mathjazz]

Hey @wassafshahzad,

I like the idea of using @cached_property from readability perspective.

However, I don't see how this solves the N+1 problem.

For example, on the contributors page, we have:

• 100 users displayed
• user.fxa_avatar evaluated once per user
• 100 social_auth.filter(...).first() queries

Perhaps the ideal solution is to combine both:

@cached_property
def fxa_avatar(self):
    if hasattr(self, "_prefetched_fxa_avatar"):
        return self._prefetched_fxa_avatar

    fxa = self.social_auth.filter(provider="fxa").first()
    return fxa.extra_data.get("avatar") if fxa else None

And then in list views:

users = User.objects.prefetch_related(
    Prefetch(
        "social_auth",
        queryset=SocialAccount.objects.filter(provider="fxa"),
        to_attr="_prefetched_fxa_accounts",
    )
)

[wassafshahzad]

Hey @wassafshahzad,

I like the idea of using @cached_property from readability perspective.

However, I don't see how this solves the N+1 problem.

For example, on the contributors page, we have:

• 100 users displayed
• user.fxa_avatar evaluated once per user
• 100 social_auth.filter(...).first() queries

Perhaps the ideal solution is to combine both:

@cached_property
def fxa_avatar(self):
    if hasattr(self, "_prefetched_fxa_avatar"):
        return self._prefetched_fxa_avatar

    fxa = self.social_auth.filter(provider="fxa").first()
    return fxa.extra_data.get("avatar") if fxa else None

And then in list views:

users = User.objects.prefetch_related(
    Prefetch(
        "social_auth",
        queryset=SocialAccount.objects.filter(provider="fxa"),
        to_attr="_prefetched_fxa_accounts",
    )
)

I will implement this, but just one question should we not attach the _prefetched_fxa_avatar in the cached property if its not available after getting it ?

[mathjazz]

I will implement this, but just one question should we not attach the _prefetched_fxa_avatar in the cached property if its not available after getting it ?

Yeah, that would make sense!

[flodolo]

This function crashes for me locally, it needs a guard if user.pk is not None:.

[wassafshahzad]

Yes, I will add it. Anonymous users might be the reason since there pk is None

[wassafshahzad]

@flodolo Just checked again but Anonymous users don't access avatar_url so I am not use what kind of User would be missing a pk but I have added a check regardless

[wassafshahzad]

@mathjazz I have added the fxa_avatar function. The reason its not a property is because the User model doc strings say to not monkey patch without good reason. I have also updated most if the User list views if I miss something please to inform me

[mathjazz]

Thanks for the update.

Why is fxa_avatar() in a different module avatar_url()?

Looks like it's only called once? Why don't we simply drop it and move the code to avatar_url()?

We talked about using cached_property (I'm not sure it would have noticeable performance improvements), but we don't actually use it.

If we want to keep the function separate, a more consistent name would be fxa_avatar_url().

[mathjazz]

I'd be explicit here:

Suggested change

	return
	return None

[wassafshahzad]

Thanks for the update.

Why is fxa_avatar() in a different module avatar_url()?

Looks like it's only called once? Why don't we simply drop it and move the code to avatar_url()?

We talked about using cached_property (I'm not sure it would have noticeable performance improvements), but we don't actually use it.

If we want to keep the function separate, a more consistent name would be fxa_avatar_url().

The reason its in a separate module is because it initialy was a cached_property but it will not work with cached_property decorator since add_to_class will bypass set_name. To remedy that we can add that property to the UserModel but the docstring on the UserModel says to avoid that approach. I can move it to user_util but would like to keep it separate and not make avatar_url more complex, they also deal with separate concerns.
I can add @cache to fxa_avatar but we will not gain that much performance

[mathjazz]

OK, let's move it to user_utils and call it fxa_avatar_url().

[wassafshahzad]

@mathjazz Done

[mathjazz]

Thanks for the update @wassafshahzad, passing this to @functionzz for the final look.

[mathjazz]

@wassafshahzad Could you please rebase?