FEATURE: Display unread and new counts for messages. (PR #14059)

GitHub

This pull request introduces 3 alerts when merging 0d9b02b9a472a5c20e97930a7c54459f802c6b2c into ecb117df598aff752eefe5538fa87b124b70ebf2 - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class

This pull request introduces 3 alerts when merging a6c484fb244173ab5d67bd4e245ae1dea68f7212 into ecb117df598aff752eefe5538fa87b124b70ebf2 - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class

Wonder if you could replace all of this by:

import { filterBy } from "@ember/object/computed";

// ...

groupsWithMessages: filterBy("model.groups", "has_messages", true),

The title of this pull request changed from “Redo pm topic tracking state” to "FEATURE: Display unread and new counts for messages.

Didnt look at code, but all these queryAll here seem weird, is there multiple li a.new ? Also text is jquery function we probably should avoid using this.

document.querySelectorAll("div").text() // Uncaught TypeError: document.querySelectorAll(...).text is not a function
    at <anonymous>:1:34

Didnt try but would this just work ?

incomingCount: computed("pmTopicTrackingState.newIncoming.length")

why don’t we check pmTopicTrackingState is not null here ? Is it intentional to think at this point it can’t be null ?

do we need explicit check on null here ?

!topic.last_read_post_number && ...
          user,
        user,

I know this is not your pr, but this is an invalid class name we would prefer no-glyph, no need to fix it there though

Didn’t look ruby part, but js part looks very clean to me :clap:

Yeah this should work we do it elsewhere:

I got an issue where I did the following:

  1. Send a PM to my user from another user, it comes up as New (1) as inspected
  2. Click the “See 1 new or updated topic” message
  3. Click the topic
  4. Click the envelope icon at the top left of the topic to go back to the list of messages
  5. The “See 1 new or updated topic” message remains

So maybe the incoming message count is not cleared? This happens with both new and unread messages.

Other than that I think this is really good, nice and clean, most comments are pretty minor.

Why use Ember.A when you could just set an array on init? I just ask because we haven’t used this anywhere else in the app and it’s not clear what it is without looking it up.

We should be using on("init") now:

That way you don’t need the super call either.

Thank you for keeping the improvements like Set and using modifyState, this class is a lot easier to follow than the other tracking state class!

I think we should keep it this way for consistency and clarity, that’s what we do in the other topic tracking state JS

Nitpick, but this doesn’t need to be in an else block because the first block is an early return:

if (groups.length === 0) {
  return true;
}

// other stuff