zulip-flutter Start Requiring Zulip Server 5

Blocked by #267, but these are migrations that can happen in parallel. I imagine that there won't be much overhead rebasing this kind of changes.

Fixes: #268

Aug 22 '24 21:08 PIG208

The PR itself is ready, but we won't be ready to merge it until we refuse to connect to older servers. Going through some reviews will still be helpful, though.

Aug 23 '24 21:08 PIG208

Thanks! Updated the PR.

Aug 27 '24 04:08 PIG208

Thanks! LGTM except bump on this part of https://github.com/zulip/zulip-flutter/pull/904#discussion_r1732067432 :

it seems like we should keep the test that confirms that false is chosen if is_system_bot is absent.

Aug 28 '24 22:08 chrisbobbe

Got that test back. Thanks for the review!

Aug 29 '24 06:08 PIG208

Great! Marking for Greg's review once he's back from vacation.

Aug 29 '24 21:08 chrisbobbe

BTW there's generally no need to push a revision when it's a pure rebase, with no merge conflicts or other changes. The update causes notifications, and doesn't really save effort later — there'll typically be another rebase anyway just before merge, and git rebase is a very easy step to do at merge time when there's no conflict to resolve.

(As it happens I'm just about to take a look at this PR, but it wasn't because of the notification from the revision just now; it was from scanning the list of PRs marked for my review.)

Sep 07 '24 04:09 gnprice

Thanks for the review! Added some TODOs commits for GetServerSettingsResult changes, making them easy to drop, and addressed the issues from the review. This revision should be fairly readable with range-diff.

Sep 10 '24 18:09 PIG208

Also marked as draft to reflect that we won't be ready to merge this until we have #267.

Marking as draft helps avoid me forgetting that and going ahead and merging after a next round of review, if the thread otherwise looks ready :slightly_smiling_face:

Sep 16 '24 23:09 gnprice

Removed the label to get this out of my queue since it's on hold (as described above). We'll bring it back when we pick up the underlying issue again.

Feb 14 '25 07:02 gnprice

We have #267 now, so this is something we could potentially merge when its code is ready. I've just rebased it atop current main.

Jul 04 '25 05:07 gnprice

The comments in my previous round https://github.com/zulip/zulip-flutter/pull/904#pullrequestreview-2307995616 above were all addressed.

Skimming now, there are a few commits whose summary lines indicate they're not ready: 9f15154d9 TODO api: Add TODO to modernize GetServerSettingsResult.zulipMergeBase c514b59f6 FIXME LINK api/notif [nfc]: Cut comment on FcmMessageChannelRecipient.streamId, relying on server 5+, FL 115+. 662ccaeae TODO api: Mark GetServerSettingsResult.realmWebPublicAccessEnabled as required, relying on server 5+, FL 116+

Other than that, it seems like these changes are probably ready. I guess I'll want to reread, though, at least briefly, since it's been most of a year since we put it on hold (awaiting #267).

Jul 04 '25 05:07 gnprice

Yeah. I think this mainly needs an update resolving the TODOs/new comments and a reread. I can get back to this tomorrow night (don't have my laptop with me today). Feel free to beat me to it if needed.

Jul 08 '25 01:07 PIG208

Pushed an update addressing the new TODOs and the comments above.

Jul 09 '25 03:07 PIG208

OK, revised and updated. @chrisbobbe would you take a look at the revision?

Jul 23 '25 01:07 gnprice

Thanks for the review (and for #1783)!

Made those commit-message edits and rebased. I also added a related comment in a small new commit: 506b87a74 server_support [nfc]: Explain why zulipMergeBase should remain nullable

I'll let CI run on this, and then I plan to merge.

Aug 14 '25 02:08 gnprice