api: Remove backward-compat code for mark-read protocol in FL <155 #1658
Conversation
chrisbobbe
added
a-api
chrisbobbe
added
integration review
|
Bumping to integration review, given this should be low-risk (removing code that's dead as long as we have accurate FLs) and would help toward search, #1662. |
| final useLegacy = store.zulipFeatureLevel < 155; // TODO(server-6) | ||
| if (useLegacy) { |
There was a problem hiding this comment.
We support Zulip Server 7 and later (see README) and refuse to
connect to older servers. Since we haven't been using this protocol
for servers FL 155+, this should be NFC for all servers we connect
to, as long as we know their feature level accurately.
I think this can be a bit stronger: it's just NFC.
The condition on these lines is based on the zulipFeatureLevel we have. If that feature level were below our supported threshold, we wouldn't have gotten to this point: UpdateMachine.load would have (caught and re-)thrown _ServerVersionUnsupportedException, rather than getting as far as constructing a PerAccountStore. So this feature level is always at least 185, and this condition is always false.
Because this is false, the rest of the affected lib/ code is dead (except from tests), so the change is NFC.
There was a problem hiding this comment.
I guess more specifically it's NFC in the actual app (where the store comes from UpdateMachine.load), though not in tests: we have various tests that construct stores with older zulipFeatureLevel, pending #992 and similar cleanups.
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks for cleaning this up! The changes all look good; one comment below.
This code had a switch/case on the Narrow type, so I discovered it while implementing keyword-search narrows. We support Zulip Server 7 and later (see README) and refuse to connect to older servers. Since we haven't been using this protocol for servers FL 155+, this is NFC. Related: zulip#992
chrisbobbe
force-pushed
the
pr-remove-legacy-mark-read-protocol
branch
from
8b5ed9b to
898d907
Compare
|
Thanks! Revision pushed. |
|
Thanks! Looks good; merging. |
2 participants
This code had a switch/case on the Narrow type, so I discovered it while implementing keyword-search narrows.
We support Zulip Server 7 and later (see README) and refuse to connect to older servers. Since we haven't been using this protocol for servers FL 155+, this should be NFC for all servers we connect to, as long as we know their feature level accurately.
Related: #992