Skip to content

Conversation

chrisbobbe
Copy link
Contributor

Fixes: #3654.

The server correctly interprets this special number as indicating
that we want the latest messages (0 is used to request the oldest
messages). Later, we can conditionalize on the server version to
support the updated API merged in
zulip/zulip#13747, that use the strings
'oldest' and 'newest' instead of 0 and 10000000000000000.

@chrisbobbe chrisbobbe requested a review from gnprice February 6, 2020 19:29
@chrisbobbe chrisbobbe force-pushed the pms-from-new-sources branch from 49dfc47 to df86418 Compare February 6, 2020 23:55
@gnprice
Copy link
Member

gnprice commented Feb 7, 2020

Thanks @chrisbobbe !

One point in the commit message: in the API, 0 doesn't mean the oldest unread messages, but simply the oldest. The legacy API for "oldest unread" is a separate boolean flag use_first_unread_anchor: true, which causes anchor to be ignored.

... Looking closer through our code for this, now that I have those facts about the API in my head from reading zulip/zulip#13747 recently, I see that within our code we do it the way the commit message describes, though. (And I guess we never make a request for simply the oldest messages.) It's confusing that we do something that's so almost, but not quite, like the server's API. I may see if there's a quick refactor I can do to clean that up.

Anyway, I'll make that adjustment in the commit message and then merge.

Chris Bobbe and others added 2 commits February 6, 2020 18:51
Fixes: zulip#3654.

The server correctly interprets this special number as indicating
that we want the latest messages.  The larger value we had been
using will certainly get the latest messages, because it's larger
than any possible message ID, but the server won't make that
inference and can incorrectly report `found_newest: false`.

Later, we can conditionalize on the server version to use the
updated API merged in zulip/zulip#13747,
which uses the string 'newest' instead of a special number.

[greg: revised commit message]
@gnprice gnprice force-pushed the pms-from-new-sources branch from df86418 to b818479 Compare February 7, 2020 02:51
@gnprice gnprice merged commit b818479 into zulip:master Feb 7, 2020
@gnprice
Copy link
Member

gnprice commented Feb 7, 2020

And pushed 7a3a845 to try to clean up that confusion a bit:

commit 7a3a845477127fdb7a10b2dd682650c06f93ebaf
Author: Greg Price <[email protected]>
Date:   Thu Feb 6 20:45:28 2020 -0800

    anchor [nfc]: Correct the jsdoc on FIRST_UNREAD_ANCHOR.
    
    This jsdoc didn't match the name at all!  It turns out the name was
    right... and the jsdoc described what, confusingly, the very same
    value means in the actual Zulip API.

I also explored a refactor to actually stop using 0 in this confusing way. I have a draft that more or less does it, but more work is required before that's mergeable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PMs from new sources not immediately displayed in PMs list

2 participants