FEATURE: Add user attributes to directory items (PR #8376)

Adds a hidden site setting user_directory_includes_profile, which when enabled, serializes all the attributes from UserSerializer to the DirectoryItem

GitHub

You’ve signed the CLA, fzngagan. Thank you! This pull request is ready for review.

The title of this pull request changed from “Add user atts to dir item” to "Add user attributes to directory items

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/adding-user-attributes-to-the-directory-item-serializer/130927/19

The title of this pull request changed from “Add user attributes to directory items” to "FEATURE: Add user attributes to directory items

How does this affect performance? We need to be sure this doesn’t introduce n+1 queries in the directory serializer.

For 3 users on my dev environment

Property/Setting On Off
Number of SQL queries 146 5
Query Execution Time 375ms 20.6ms

It does tax performance. I’m fetching the exact same data which is fetched by the default user card in discourse. I also checked its stats.

When I click on a user’s avatar and a discourse user card is triggered, it fires 52 sql queries taking ~200ms to execute.

cc @angusmcleod

@davidtaylorhq The approach I have in mind is…

  1. Create a UserCardSerializer which serializes only those attributes which the user card is currently supposed to display.

  2. Use UserCardSerializer instead of the ::UserSerializer.

This approach will reduce the number of queries to only the essential ones. Also, the team can utilize this new serializer for the default user card as well.

@davidtaylorhq Thoughts on this one ?

The issue here isn’t necessarily the amount of information being loaded. We need to avoid N+1 queries - and this issue will persist even with a simpler UserCardSerializer. The number of queries should remain constant, regardless of how many users you have in the list.

The way to mitigate N+1 is to add something like includes(...) when loading the user object in the controller. This should leave us with 1 query per table, not n queries per user. There are plenty of examples of this in Discourse, but here’s one:

I think we should add another test that expect some user information on a serialized ItemUser.

Is this introducing any N+1?

Yes, probably…

I really hate to close this, you spent so much work here.

I feel though it makes most sense though to put this on hold while @davidtaylorhq works out all the internal changes we want to see to support his kind of stuff.

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/user-card-directory/144479/14