UX: Include public groups in mentionable groups set (PR #8516)

GitHub

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

Could you please provide a little more context for the change?

This commit includes public groups in autocomplete and also wraps group mentions in a link as long as the group is visible to everyone, even though the group is not mentionable.

That API feels a little weird. I think the UI should instead use something like Group.visible and on the backend, we rely on Group.mentionnable to generate the notifications.

For this feature, I was expecting more like an integration test that would

  1. create a public group that can’t be mentioned (with at least one member)
  2. create a post with an @mention to that group
  3. ensure no mention notifications were sent to members of the group
  4. ensure the mention gets wrapped in the cooked version of the post

It feels a little bit weird and I was not happy at first either, but I have a few arguments why this makes sense: :thinking:

We already have Group.visible_groups, but that’s not what we want here. What we want is really just mentionable groups, which sometimes can include the public ones as well. Also, it is consistent with the API for Group.visible_groups, which has a include_everyone option.

1 Like

Each individual step is tested somewhere, cooked_post_processor_spec, post_alerter_spec or pretty_text_spec, but I agree that an integration test would be ideal, so I wrote one. :slight_smile:

Nice :+1:

Nice! :+1:

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

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