Fix null streams #283

Sam1301 · 2017-01-07T13:58:45Z

EDIT:
Currently stream objects retrieved from messages are null throughout the project. I have corrected this by querying a stream object when we fetch new messages.

listen for add, remove and update operations for subscription type events
listen for muted topics events.
in user configuration response, also get unsubscribed streams to ensure streams are not null in the message list.

smarx · 2017-01-07T13:58:46Z

Automated message from Dropbox CLA bot

@Sam1301, it looks like you've already signed the Dropbox CLA. Thanks!

kunall17 · 2017-01-07T20:48:07Z

Awesome work @Sam1301 but can you please put the second commit into a new PR!

Sam1301 · 2017-01-07T20:53:29Z

sure 👍

Sam1301 · 2017-01-08T10:26:56Z

As stream objects will be added to each message, we would need to reset the existing database which has null values for streams in each message. Hence, I have suggested to bump up the database version which would reset the database and new messages will be fetched.

kunall17 · 2017-01-09T18:57:53Z

Nice work!
Works as expected.

@timabbott LGTM, just FYI this upgrades the database version hence all the existing data are dropped and reloaded we have to take this big step because the streams saved in the messages table are null therefore which is causing problems in other features!
There is no noticeable UI change as sam1301 stated no re-login required!
Here is the Databases.zip (contains the old and the newly updated database after running the project through this branch) if any one wants to check/debug!

timabbott · 2017-01-11T06:57:18Z

app/src/main/java/com/zulip/android/models/Message.java

                        Person person = Person.getOrUpdate(app, m.getSenderEmail(), m.getSenderFullName(), m.getAvatarUrl());
                        m.setSender(person);
+                        Stream stream = Stream.getByName(app, m.getRecipients());
+                        m.setStream(stream);


How does this work for messages sent to an individual rather than a stream?

Apologies, I totally missed that, after giving it some thought I think setting streams to null for private messages can be a potential solution.

timabbott · 2017-01-12T00:21:59Z

OK; may be worth some careful testing; @kunall17 @saketkumar95 does one of you have time?

kunall17 · 2017-01-14T04:53:49Z

app/src/main/java/com/zulip/android/models/Message.java


            for (Person person : people) {
-                if (person.id != app.getYou().id || people.length == 1) {
+                if (!person.getEmail().equals(app.getYou().getEmail()) || people.length == 1) {


What's the problem using ID's here?

I think the person object here has id 0 because the user configuration response we receive is not deserialized into a person object with an id as it is supposed to be auto-generated.

No, the person ID is auto generated so it shouldn't be the same and if you check the dumped databases i posted the person have unique ID's

thanks, made the changes 👍

kunall17 · 2017-01-15T18:44:33Z

@vishwesh3 @saketkumar95 It would be great if you guys could help testing this big PR, this fixes the unread count + messages.streams columns?

yadav-rahul · 2017-01-16T08:20:49Z

@kunall17 Though I am still a newbie to Zulip but Can I test it too?

kunall17 · 2017-01-16T09:15:05Z

@yadav-rahul Sure go ahead.. 👍

jainkuniya · 2017-01-16T15:44:28Z

sure.

kunall17 · 2017-01-16T18:09:41Z

@Sam1301 The new bubble looks good for small numbers, but for double digits the round shape looks small!
Also how about displaying the unread count in the toolbar like Zulip (12) ?

Sam1301 · 2017-01-16T19:06:10Z

@kunall17 Thanks, I will make the changes 👍

Sam1301 · 2017-01-17T20:42:48Z

Update

resize bubbles based on counts
https://drive.google.com/open?id=0B4jK5QX65b0ZX2k0S1REc0lfbW8
show counts in toolbar
https://drive.google.com/open?id=0B4jK5QX65b0ZcWx4a0xNTTAwQlU

kunall17 · 2017-01-19T11:27:51Z

@Sam1301 I think we should this PR, and fix unread counts in another one because we are facing issue if the pointer is very low from the latest message ID then the android app will not fetch those messages and hence we have lesser unread counts, which is not the right thing to display!
So we can merge the fixing part?
The expandable adapter will be up and working again!

Sam1301 · 2017-01-19T17:42:25Z

Since we only fetch 100 messages below the latest pointer, we won't get the correct number of unread counts for all the messages past that. Can we change this and fetch all the messages after the current pointer?

timabbott · 2017-01-19T19:11:21Z

@Sam1301 we probably want to limit it in some way. The backend is happy fetching up to ~1k messages, but if a user is somehow 25K messages behind, the request will fail.

What we do on web is run a loop fetching in batches of 400 until we get to current. We also offer the user the opportunity to "declare bankruptcy" if they have more than ~500 unread messages.

I think for practical purposes, once the user has over 1000 unread messages beyond the pointer, that's more than they're realistically going to scroll through anyway, so we could probably just stop at some point as a solution for now (there's not a cheap way to query the backend for grand total unread messages by recipient).

kunall17 · 2017-01-19T19:38:17Z

@Sam1301 Awesome Work! 👍

@timabbott Now this PR has no unread counts commit's so we can merge this one! :)

timabbott · 2017-01-20T00:01:35Z

app/src/main/java/com/zulip/android/models/Stream.java

-                app.getDao(Stream.class).createIfNotExists(stream);
-            }
+                // catch the new stream object with auto-generated id
+                stream = app.getDao(Stream.class).createIfNotExists(new Stream(name));            }


Why do you need to generate IDs for Stream objects? Streams have an ID on the backend...

I guess maybe fixing that would be harder to fix...

I think when stream is null, and a new stream object is created... using Stream(name) constructor, it doesn't get an id assigned to it and streams with id 0 are stored in the database with createIfNotExists, hence I tried to generate the ids. Am I mistaken?

I should explain more. I think your code is probably correct for how the Android app works today. But what I think is going on here that is probably not ideal is that Zulip servers give streams an ID, and the Android app gives streams an ID (here), and those IDs have nothing to do with each other. :(.

I know the stream objects have id given from server which are deserialized into id field in stream class here. I guess my concern is if the stream is null after querying for it in the database, we would be making a new stream object for it with id zero and store it in the database. Eventually, other stream objects generated this way would also have id 0 as well. Storing these in the database might cause a unique constraint exception. Am I mistaken? I just want to clarify this doubt :)

Well, storing a made-up stream ID for it might also cause a unique constraint exception if/when the stream that actually has that ID on the backend shows up, right? More rare, but still a problem. Hrm.

It seems like there are 2 possible correct solutions here:

The same approach the web client uses, namely that we fetch from the server the complete list of streams we're subscribed to on startup, and maintain that list over time using the events system, so that this condition never happens.

We modified the way messages are sent down to clients to optionally include the Stream ID in the display_recipient field (or something), so that the correct stream ID would be available here.

What do you think we should do?

I agree with the approach followed in the web client. That seems less susceptible to future bugs.

Thanks for giving me such a nice explanation, I learned a lot from this pr :)

kunall17 · 2017-01-24T04:08:41Z

app/src/main/java/com/zulip/android/models/Stream.java


    @SerializedName("subscribers")
-    private List<Integer> subscribers;
+    private List<String> subscribers;


These subscribers is the list of email right?

kunall17 · 2017-01-24T04:29:00Z

Looks good to me now
LGTM @timabbott !

timabbott · 2017-01-24T04:56:01Z

Merged, thanks @Sam1301!

Sam1301 force-pushed the fix-streams-null branch from 9e45fb8 to e096378 Compare January 7, 2017 21:00

Sam1301 changed the title ~~Add stream object to messages and correct emoji paths~~ Add stream object to messages Jan 7, 2017

kunall17 mentioned this pull request Jan 11, 2017

Fixes the expandableListView in the streams drawer #254

Closed

timabbott reviewed Jan 11, 2017

View reviewed changes

Sam1301 force-pushed the fix-streams-null branch from 08b3508 to b607659 Compare January 11, 2017 21:15

kunall17 suggested changes Jan 14, 2017

View reviewed changes

Sam1301 force-pushed the fix-streams-null branch from b607659 to 223465f Compare January 14, 2017 19:21

Sam1301 changed the title ~~Add stream object to messages~~ Fix unread counts and null streams Jan 16, 2017

Sam1301 force-pushed the fix-streams-null branch from cb25d84 to 07bc283 Compare January 19, 2017 17:45

timabbott reviewed Jan 20, 2017

View reviewed changes

Sam1301 added 2 commits January 20, 2017 07:32

Add stream object to message whenever new messages are received.

e822f1b

Bump up database version to account for new stream objects.

43b5203

Sam1301 force-pushed the fix-streams-null branch from 07bc283 to 43b5203 Compare January 20, 2017 02:11

Sam1301 added 3 commits January 23, 2017 07:35

Listen for subscriptions updates, add and remove events.

b164c1e

Listen for muted_topics events.

442d686

Get unsubscribed streams from user configuration response.

42b6750

Sam1301 changed the title ~~Fix unread counts and null streams~~ Fix null streams Jan 23, 2017

Sam1301 force-pushed the fix-streams-null branch from a9477f6 to de99727 Compare January 23, 2017 04:08

Add documentation.

805f1bc

Sam1301 force-pushed the fix-streams-null branch from de99727 to 805f1bc Compare January 23, 2017 12:14

kunall17 reviewed Jan 24, 2017

View reviewed changes

timabbott closed this Jan 24, 2017

Sam1301 mentioned this pull request Mar 3, 2017

Show unread counts and add bankruptcy option. #430

Open

Fix null streams #283

Fix null streams #283

Uh oh!

Conversation

Sam1301 commented Jan 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarx commented Jan 7, 2017

Uh oh!

kunall17 commented Jan 7, 2017

Uh oh!

Sam1301 commented Jan 7, 2017

Uh oh!

Sam1301 commented Jan 8, 2017

Uh oh!

kunall17 commented Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timabbott commented Jan 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sam1301 Jan 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kunall17 commented Jan 15, 2017

Uh oh!

yadav-rahul commented Jan 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kunall17 commented Jan 16, 2017

Uh oh!

jainkuniya commented Jan 16, 2017

Uh oh!

kunall17 commented Jan 16, 2017

Uh oh!

Sam1301 commented Jan 16, 2017

Uh oh!

Sam1301 commented Jan 17, 2017

Uh oh!

kunall17 commented Jan 19, 2017

Uh oh!

Sam1301 commented Jan 19, 2017

Uh oh!

timabbott commented Jan 19, 2017

Uh oh!

kunall17 commented Jan 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sam1301 Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kunall17 commented Jan 24, 2017

Uh oh!

timabbott commented Jan 24, 2017

Sam1301 commented Jan 7, 2017 •

edited

Loading

kunall17 commented Jan 9, 2017 •

edited

Loading

Sam1301 Jan 14, 2017 •

edited

Loading

yadav-rahul commented Jan 16, 2017 •

edited

Loading

Sam1301 Jan 20, 2017 •

edited

Loading