FEATURE: New and Unread messages for user personal messages. (PR #13603)

GitHub

This pull request introduces 1 alert when merging a5dd25d4e09fc87d579decf04a94624b858741ac into 87c1e98571631d83e0d9f0846bd95a2c7e9bce87 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

In JS our convention is to use UPPER_SNAKE_CASE for constants. ie PERSONAL_INBOX and ALL_INBOX

Looks good and I think this will be a huge improvement. I am mostly concerned about breaking changes.

I don’t think this needs to be a CP. In the template you could do {{if (and siteSettings.tagging_enabled tagsContent)}}

Ditto here, this could be {{#if inboxes}} (or perhaps {{#if inboxes.length}})

If it’s a site setting maybe we don’t need to pass it as a parameter? The server side route could use SiteSetting. max_tags_in_filter_list

These are breaking API changes. I suspect they are not used much by API consumers but maybe we can do a search and see?

Is the intention here to default to group 0 “everyone” if group is nil? It seems peculiar to me that we’d have private messages to “Everyone” and maybe this is not what we want? It actually seems quite odd to me to call list_private_messages_group if we have no group. Maybe it should raise an error instead?

(same comment applies in the other _group methods below.)