Add settings to allow multiple badges to be displayed

[DomLennonZA]

This exposes a setting in meta to say whether all of a user's badges should be displayed. This also exposes all the user's groups in a property called allGroups in controllers/topics.js. Even though this setting exists, it still needs to be supported by the theme the user has. The default behavior of using the selected group still exists to ensure that no existing sites break.

Re: #5338

[pichalite]

@DomLennonZA this is better suited as a plugin than being in core.

[DomLennonZA]

@pichalite Wouldn't this then lead themes to have a dependency on a plugin? Right now, any user who does not want this functionality wont get it. I cant really see any harm in adding it to core. If thats the consensus though, I will move it out into a plugin.

[pichalite]

Right now, any user who does not want this functionality wont get it.

Not really. Based on the code in the PR, all user groups will be added to post data no matter what the ACP setting is.

Also, if multiple badges setting is turned on in ACP... user level "Group Title" setting becomes useless.

You may bot know this but NodeBB used to show all group badges but decided to remove it and move to only one badge because it's not easy to fit all the badges on the post. Takes up a lot of space on the post.

[DomLennonZA]

@pichalite Your assumption here is incorrect. The Group Title is always passed through, even if multiple badges setting is true. I understand that it can be difficult to display multiple badges but that is up to the template to decide how to handle that.

[pichalite]

I did not say Group Title is not passed. I said if multiple badges is turned on and the theme displays multiple badges on the post. What ever the user sets for Group Title is ignored and will become useless.

Are you planning to send a PR with template changes to handle multiple badges in persona theme?

[DomLennonZA]

If the theme wishes to use multiple badges, it should also handle the scenario where a user does not want multiple badges.

Are you planning to send a PR with template changes to handle multiple badges in persona theme?

No, not unless @julianlam or @psychobunny want me to. This is purely for people who want to use this in their current or future templates and can be ignored if it is not needed.

[pichalite]

What about the group data always being included in the payload no matter what the ACP setting is for allowing multiple badges.

Also as you said, this is for people who want multiple badges. So, a plugin makes lot more sense than adding this to core.

[julianlam]

Here are my thoughts:

1. Is there significant overhead in constructing the API payload to render all user badges? Only insomuch as we need to pull the group name (or badge title value) from the group hash itself, but it's fairly minimal.
2. It is fairly easy for a theme to either show multiple badges, or one badge. For the former, <!-- BEGIN badges --><!-- IF @first -->badge code<!-- ENDIF @first --><!-- END badges -->, for the latter, no need for the IF..ENDIF, and it is then up to the theme designer themselves to handle overflow of badges.
3. There is still the issue of the disconnect between badges shown and the "Group Title" setting. I would advise changing the logic so that all badges are shown, but the badge matching groupTitle be floated to the top, then the setting itself is not ignored from a theme that elects to only show one badge.
4. We should add a blurb in UCP settings stating that themes can elect to display either one, multiple, or all badges.

With above, you then won't need to have a specific ACP setting to enable or disable multiple badges, the badges themselves will all be available for the theme to parse if it elects to do so.

If you're worried about performance, drop in an LRU cache for group badge data, even...

We do have to think of edge cases, however... what would a badge set look like with no badges? one badge? a hundred badges?

(a million badges? -1 badges? 3.1415 badges? NaN badges? 😆 )

[DomLennonZA]

@julianlam Sorry, Im just trying to figure out what tasks should be done here?

As far as I can see, performance shouldn't be an issue. Implementing a LRU cache seems a little overkill for this but can be done if deemed necessary.

Secondly, if I understand correctly you are saying we should put all the user's groups into a single array and just push their selected group to the top of that array. While thats fine, this would break current templates.

[pitaj]

It doesn't have to break current templates. You add the field for the array, but you don't need to remove the field for the single entry.

[julianlam]

Yeah that is a good point @DomLennonZA, we'll have to introduce a new field for groups, while maintaining selectedGroup for backwards compatibility, although we can deprecate it in favour of groups array.

[barisusakli]

As others have mentioned groups.getUserGroups(uids, function (err, groups) { shouldn't be called if the acp setting is turned off.

[DomLennonZA]

Ok, I think my last commit handles most of the issues raised here. There is some work that needs to be done in the template for multiple badges to be shown(and for letting the user know that may actually be a thing). @barisusakli @julianlam any more changes you think should go in before I jump onto something else?

[DomLennonZA]

@julianlam @barisusakli any additional comments?

[barisusakli]

Can you remove the styling changes? What does selecting a group in the user profile page do? Does it move that group to the front of the allGroups array?

Persona theme needs a change as well for this PR to make sense, without that this is not functional.

Theme needs to check if showMultipleBadges is on and use the allGroups array and use selected group if not.

[DomLennonZA]

@barisusakli Showing multiple groups is configurable in the ACP. When it is enabled, the user's selectedGroup is pushed to the top of the array. When it is disabled, it works how it currently works now.

For making the Persona changes. As I mentioned, this is optional and only for themes that want to display multiple badges. I am more than happy to do the work if you want this in the Persona theme, otherwise these changes wont affect the theme at all. What do you want?

[barisusakli]

@DomLennonZA can you remove the styling changes in this PR and make a PR to persona to add the multiple badge template if it's enabled. You will have to expose meta.config.showMultipleBadges to the client side. Take a look here for that https://github.com/NodeBB/NodeBB/blob/master/src/controllers/api.js#L20

[DomLennonZA]

@barisusakli Sorry, been caught up with other work. Will get onto this ASAP

[DomLennonZA]

@barisusakli Styling changes are removed. Ill add the persona changes when this is in or would you prefer me to add them now?

[psychobunny]

Yes please, we'd like to review both PR's at the same time, thanks :)

[DomLennonZA]

@psychobunny @barisusakli Pull request submitted. I couldn't find a guide on the formatting practices but I hope this its OK. NodeBB/nodebb-theme-persona#351

[DomLennonZA]

Hi @psychobunny @barisusakli . Any update on this getting merged in?

[barisusakli]

I had an idea about this, how about if showMultipleBadges is turned on we change the user group title select into a multi select so the user can pick which badges to display? This way if a user is a member of 10 groups they can only display the ones they care about.

Let's say I am a member of Administrators, Gamers, Translators, Plugin Devs

But I only want to display Admin and Gamer. I select those 2 and it gets saved into userData.groupTitle as Admin,Gamer. Then the code can split by , and only return the data for those 2 groups.

In the user edit page the template would change to

<!-- IF config.showMultipleBadges -->
<select multiple>
<!-- ELSE -->
<select>
<!-- ENDIF config.showMultipleBadges -->
... options as usual..
</select>

Let me know what you think?

[DomLennonZA]

@barisusakli That sounds fine. Do we select all by default?

[barisusakli]

@DomLennonZA I don't think they should all be selected by default, since userData.groupTitle will already have a selection or empty if the user doesn't want to show anything..

[pitaj]

Any updates on this @DomLennonZA ?

[barisusakli]

#6433