[wip] Cut various support for ancient servers#5708
Draft
gnprice wants to merge 24 commits intozulip:mainfrom
Draft
[wip] Cut various support for ancient servers#5708gnprice wants to merge 24 commits intozulip:mainfrom
gnprice wants to merge 24 commits intozulip:mainfrom
Conversation
Since 9620629, we've always had a realm_uri property in this object -- we've rejected any notifications where it was absent in the payload. So this check is no longer needed.
TODO Perhaps make ServerCompatBanner more insistent? This is awfully core functionality that this breaks for ancient servers. Tested manually (on a current server) that sending to a stream, and sharing to a stream, still work.
Tested manually that sending a PM still works.
The server just ignores this. At the other api.sendMessage call site, in ShareWrapper, we already don't send it.
The server actually accepts and ignores these, but best to make the interface clear.
This is really part of the details of how things are encoded in the API, not something application code should have to concern itself with.
And add a TODO for changing what we actually send to the server. (Which in turn we'll take care of shortly.)
Tested manually that this feature still works fine.
(previously took care of topic_links and subject_links too, but that's been done since) These descriptions are copied from the corresponding properties on an `update_message` event, at EventUpdateMessageAction; the same changes happened on messages at the same time: https://zulip.com/api/get-messages#response https://zulip.com/api/get-events#message
This encapsulates a bit more into this module of how we represent this list of recipients. Expand jsdoc on the remaining ways we expose the other details, to point out the pitfall in them. Also note with a TODO an opportunity this opens up for improving performance.
TODO clarify in server that the plural field is the only one that counts, as a matter of API
This comment mentioned server 2.0, but just as the latest as of when it was written. This field was deleted at a later version, so note that down; and the real prompt for deleting the comment will be when "user_id" is always present and can be relied on. So copy the "TODO(server-2.1)" from there.
…2.0+ TODO cut the fallback, too; and its test case.
This lets Jest give more informative output when some of them break.
This reflects what this was testing before 144dbb1, where we started expecting realm_uri. The ancient payloads that didn't have recipient_type didn't have that either.
TODO test manually
TODO test manually; perhaps squash with Android side
TODO perhaps rearrange/squash this, the Android, and the iOS sides
Contributor
|
Thanks! I confirmed that For the Then another search for |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a rebase of the draft branch I wrote a little over a year ago for this purpose. After writing it I realized that much of it should block on refusing to connect to ancient servers. We've now done that, in #5633.
This still needs some polishing-up, and the last few commits are to rely on server 2.1 which should wait for a sequel to #5633. But other than those last few, I think it's pretty close to mergeable.