Add unread_msgs structure to index

[kaustubh-nair]

Fixes #496

[kaustubh-nair]

@neiljp Initially, I was thinking of storing only the message_ids in the index, but I'm not sure how much of this extra information ( like stream_id, user_ids_string) is useful. Do you think all these attributes need to be stored? The structure of unread_msgs is here https://zulip.readthedocs.io/en/latest/subsystems/unread_messages.html

[kaustubh-nair]

@zulipbot add "in progress"

[neiljp]

@kaustubh-nair We know the extra information is there in the initial_data; I would work towards a version that works simply first, perhaps leaving comments referring to the additional data available for quick reference later.

At a glance the code looks like it should broadly do the right thing, though we might be able to move things around and simplify them once it is more settled. You can probably squash the second commit with the first?

[neiljp]

@kaustubh-nair You may want to investigate integrating this into classify_unread_counts and the UnreadCounts data, though currently this may work with the data being separate.

[kaustubh-nair]

@kaustubh-nair You may want to investigate integrating this into classify_unread_counts and the UnreadCounts data, though currently this may work with the data being separate.

@neiljp Integrating it into classify_unread_counts is a good idea. We could modify UnreadCounts to store the message_ids instead of only counts. But does it make sense to let unread_msg_ids remain in index? Why is the UnreadCounts structure kept separate from the index?

[neiljp]

@kaustubh-nair This looks like being on the way to a good minimal approach 👍

I've left some comments on the changes as a whole only, as you've left your commits showing your different ideas and adjustments during development. That makes it more difficult to review them on a commit-wise basis as mergeable commits. Do you plan to restructure them? Let us know if you need more information on what I mean.

Do you plan to include the response to events in this PR, or build on this later? It might be easier here if it is not too complex, as you can ensure that the data structure has the appropriate elements.

[neiljp]

@kaustubh-nair This approach seems more straightforward; I've left some notes as the code seems a bit mixed between commits, but otherwise seems simpler 👍.

The commit flow might be more logically ordered as:

• adjust count updates to occur on read messages from server (just messages we have fetched) [currently commit 3]
• add the data structure support for keeping track of unread message ids, not just fetched ones [currently commit 1]
• use the new data structure to track the unread counts better [partly in commit 2]

The first two stages look reasonable so far, and you have part of the third in one commit. In addition to splitting commits cleanly, getting a readable 'flow' of commits helps with reviewing larger PRs where changes build on one another.

[kaustubh-nair]

@neiljp I've reorganized the commits and fixed the other issues. The unread counts now get updated for indexed messages. The next step would be to store all the unread data in the index. Then, modify set_counts to rely entirely on index['unread_msgs'] instead of index['messages'] to calculate unread counts.

[neiljp]

@kaustubh-nair These look like good progress towards using the 'read' flag events 👍

I was thinking of just merging the first commit as it is, as it should be a good first step from using the workaround of set_count in the model method, to using the actual 'read' flag event, but given we're changing the behavior I'd like to get the tests a little better first - as per the inline comments.

The other factor is that your commit text could be generally improved, which I think I mentioned in your other PR.

The other commits look broadly good, but before we add them I'd prefer to see the intended use of the extra indexed data, to make sure we've got the right approach. Re next steps, I think we should be able to manage with just the unread message ids, and update when getting new messages?

[neiljp]

@kaustubh-nair I'm not sure that has_calls is part of the mock features? I'd suggest printing it and checking it's doing what you think.

[kaustubh-nair]

@neiljp Some important points:

• I think it's a good idea to merge the first commit, I'll fix the tests and push the changes soon.
• Re to your question about using unread_msg_ids: I don't think we can do anything with just the unread message ids. If we want to update the unread counts, we need to know which stream/topic it belongs to. This is available in the initial data, which we do not store. So we can't really retrieve it using the message id from anywhere. Though, if have any solution in mind please let me know
• I just realized that the set_counts method isn't being tested anywhere? Can you please confirm this? If this is true, I could write a test for it before modifying it to accommodate the new datastructure (Maybe in a different pull, to avoid clutter) Also, I guess you could feel safer about making changes to set_counts if it is well tested.

[zulipbot]

Hello @kaustubh-nair, it seems like you have referenced #496 in your pull request description, but you have not referenced them in your commit message description(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #496..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

Thank you for your contributions to Zulip!

[kaustubh-nair]

@neiljp I've fixed the tests. Also, I've added a comment above explaining a few other things

[zulipbot]

Heads up @kaustubh-nair, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

[neiljp]

@kaustubh-nair Thanks for getting this work going! 👍 I've merged the first commit as-is, but have expanded quite a bit on the commit text, as this feels like an important change. The commit is 013f898 🎉 While this just covers those in the index, this feels like a great first step :)

If you'd like to work on the message-ids extension, then feel free to continue it in this PR, or elsewhere if you'd prefer to reduce the github noise.

[neiljp]

Re the message_ids, we do have this data to begin with in unread_msgs from the server, so perhaps a dict mapping message id to a 'category' (stream, topic, pm, huddle, ...) could work?

[kaustubh-nair]

v2 is being continued in #535