FIX: avatar flair wasn't displaying on the user summary page (PR #12867)

It’s an additional fix for FEATURE: include avatar flair on the avatars listed in a user summary’s “Most…” sections.

Avatar flair on the user summary page was displaying when it was an icon and wasn’t displaying when it was an image. On g/team/manage/membership:

GitHub

This should maybe be tested then? :wink:

This should maybe be tested then? :wink:

Hmm, a bit tricky to test and we don’t have server-side tests for avatar flair yet. But I think you’re right :slightly_smiling_face:

Why does this user needs to be both admin and mod?

What does it add to test that? Aren’t these fields already being tested “implicitly” already?

Code that decides how to display here is on the client-side, here user-avatar-flair and here.

All that the server should do for correct displaying of the flair of an automatic group is to return to the client-side:

  • user.admin
  • user.moderator
  • user.trust_level

With this information, the client is able to decide how to display flair. If we forget to return one of this field flair may be shown incorrectly.

We just need to be sure that the server returns it to the client-side component. It wasn’t working because we didn’t return the right information from the server. The component on the client-side is on the place and in the order.

I think it isn’t a very good separation of logic between client and server and a pretty tricky thing to test.

Right, since this is mostly information that is shown on the UI, wouldn’t it be better to have it tested in an acceptance test rather than a unit test?

But in acceptance tests, we use a pretender. We would return proper JSON and it would pass. But the problem that this PR and the previous one are fixing is the lack of information that the server returns.

An acceptance test wouldn’t catch this bug.

I can add an acceptance test though, but anyway we should decide if we want to leave these pretty weird server tests in the code base. I agree that they are pretty weird, that’s why I didn’t add tests in the beginning.

I would leave them, cause they do the job and document current implementation.

Then later we can move this logic to the server.

Good point :+1: