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.)

I’ll like to pass the limit intentionally in the params for now. I’m not sure how that route is being used but I’ll be refactoring that route soon in a separate PR.

These are not really considered APIs because they don’t actually return anything in the response’s body. The routes exists for compatibility with the client side.

Remove this method without depreciation because we’ve never listed this as a public API: https://docs.discourse.org/

Good catch, I was copying and pasting code when refactoring the methods. If the group is missing which we don’t expect since the controller would have handled it, TopicQuery should just raise an error. I’ve updated the code to remove any expectations that group might be nil.

@martin-brennan @lis2 Can you help me out with a review here as well? The changes are pretty extensive so I’ll like to get more eyes on this.

We should be careful taking this approach, because that doesn’t guarantee anyone is not relying on it anyway. For better or worse we have recommended in the past that people look at XHR requests and use those.

@tgxworld sure I will review it this morning; thanks for reviewing my uppy PR.

Absolutely massive change but looks great, it worked well when I tested it locally, I really like the new dropdowns. Seems very well tested too :+1:

I’ve kept the route for now and added a comment to deprecate it after Discourse 2.9 has been released.