Skip to content

api: Remove backward-compat code for mark-read protocol in FL <155 #1658

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gnprice merged 1 commit into from
Jul 3, 2025
Merged

api: Remove backward-compat code for mark-read protocol in FL <155 #1658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 0 additions & 64 deletions lib/api/route/messages.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,9 +392,6 @@ class UpdateMessageFlagsResult {
}

/// https://zulip.com/api/update-message-flags-for-narrow
///
/// This binding only supports feature levels 155+.
// TODO(server-6) remove FL 155+ mention in doc, and the related `assert`
Future<UpdateMessageFlagsForNarrowResult> updateMessageFlagsForNarrow(ApiConnection connection, {
required Anchor anchor,
bool? includeAnchor,
Expand All @@ -404,7 +401,6 @@ Future<UpdateMessageFlagsForNarrowResult> updateMessageFlagsForNarrow(ApiConnect
required UpdateMessageFlagsOp op,
required MessageFlag flag,
}) {
assert(connection.zulipFeatureLevel! >= 155);
return connection.post('updateMessageFlagsForNarrow', UpdateMessageFlagsForNarrowResult.fromJson, 'messages/flags/narrow', {
'anchor': RawParameter(anchor.toJson()),
if (includeAnchor != null) 'include_anchor': includeAnchor,
Expand Down Expand Up @@ -439,63 +435,3 @@ class UpdateMessageFlagsForNarrowResult {

Map<String, dynamic> toJson() => _$UpdateMessageFlagsForNarrowResultToJson(this);
}

/// https://zulip.com/api/mark-all-as-read
///
/// This binding is deprecated, in FL 155+ use
/// [updateMessageFlagsForNarrow] instead.
// TODO(server-6): Remove as deprecated by updateMessageFlagsForNarrow
//
// For FL < 153 this call was atomic on the server and would
// not mark any messages as read if it timed out.
// From FL 153 and onward the server started processing
// in batches so progress could still be made in the event
// of a timeout interruption. Thus, in FL 153 this call
// started returning `result: partially_completed` and
// `code: REQUEST_TIMEOUT` for timeouts.
//
// In FL 211 the `partially_completed` variant of
// `result` was removed, the string `code` field also
// removed, and a boolean `complete` field introduced.
//
// For full support of this endpoint we would need three
// variants of the return structure based on feature
// level (`{}`, `{code: string}`, and `{complete: bool}`)
// as well as handling of `partially_completed` variant
// of `result` in `lib/api/core.dart`. For simplicity we
// ignore these return values.
//
// We don't use this method for FL 155+ (it is replaced
// by `updateMessageFlagsForNarrow`) so there are only
// two versions (FL 153 and FL 154) affected.
Future<void> markAllAsRead(ApiConnection connection) {
return connection.post('markAllAsRead', (_) {}, 'mark_all_as_read', {});
}

/// https://zulip.com/api/mark-stream-as-read
///
/// This binding is deprecated, in FL 155+ use
/// [updateMessageFlagsForNarrow] instead.
// TODO(server-6): Remove as deprecated by updateMessageFlagsForNarrow
Future<void> markStreamAsRead(ApiConnection connection, {
required int streamId,
}) {
return connection.post('markStreamAsRead', (_) {}, 'mark_stream_as_read', {
'stream_id': streamId,
});
}

/// https://zulip.com/api/mark-topic-as-read
///
/// This binding is deprecated, in FL 155+ use
/// [updateMessageFlagsForNarrow] instead.
// TODO(server-6): Remove as deprecated by updateMessageFlagsForNarrow
Future<void> markTopicAsRead(ApiConnection connection, {
required int streamId,
required TopicName topicName,
}) {
return connection.post('markTopicAsRead', (_) {}, 'mark_topic_as_read', {
'stream_id': streamId,
'topic_name': RawParameter(topicName.apiName),
});
}
8 changes: 3 additions & 5 deletions lib/model/unreads.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,22 +441,20 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier {
notifyListeners();
}

/// To be called on success of a mark-all-as-read task in the modern protocol.
/// To be called on success of a mark-all-as-read task.
///
/// When the user successfully marks all messages as read,
/// there can't possibly be ancient unreads we don't know about.
/// So this updates [oldUnreadsMissing] to false and calls [notifyListeners].
///
/// When we use POST /messages/flags/narrow (FL 155+) for mark-all-as-read,
/// we don't expect to get a mark-as-read event with `all: true`,
/// We don't expect to get a mark-as-read event with `all: true`,
/// even on completion of the last batch of unreads.
/// If we did get an event with `all: true` (as we do in the legacy mark-all-
/// If we did get an event with `all: true` (as we did in a legacy mark-all-
/// as-read protocol), this would be handled naturally, in
/// [handleUpdateMessageFlagsEvent].
///
/// Discussion:
/// <https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1680275>
// TODO(server-6) Delete mentions of legacy protocol.
void handleAllMessagesReadSuccess() {
oldUnreadsMissing = false;

Expand Down
51 changes: 0 additions & 51 deletions lib/widgets/actions.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,25 +26,7 @@ abstract final class ZulipAction {
/// This is mostly a wrapper around [updateMessageFlagsStartingFromAnchor];
/// for details on the UI feedback, see there.
static Future<void> markNarrowAsRead(BuildContext context, Narrow narrow) async {
final store = PerAccountStoreWidget.of(context);
final zulipLocalizations = ZulipLocalizations.of(context);
final useLegacy = store.zulipFeatureLevel < 155; // TODO(server-6)
if (useLegacy) {
Comment on lines -31 to -32
Copy link
Member

@gnprice gnprice Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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.

Copy link
Member

@gnprice gnprice Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

try {
await _legacyMarkNarrowAsRead(context, narrow);
return;
} catch (e) {
if (!context.mounted) return;
final message = switch (e) {
ZulipApiException() => zulipLocalizations.errorServerMessage(e.message),
_ => e.toString(), // TODO(#741): extract user-facing message better
};
showErrorDialog(context: context,
title: zulipLocalizations.errorMarkAsReadFailedTitle,
message: message);
return;
}
}

final didPass = await updateMessageFlagsStartingFromAnchor(
context: context,
Expand Down Expand Up @@ -208,39 +190,6 @@ abstract final class ZulipAction {
}
}

static Future<void> _legacyMarkNarrowAsRead(BuildContext context, Narrow narrow) async {
final store = PerAccountStoreWidget.of(context);
final connection = store.connection;
switch (narrow) {
case CombinedFeedNarrow():
await markAllAsRead(connection);
case ChannelNarrow(:final streamId):
await markStreamAsRead(connection, streamId: streamId);
case TopicNarrow(:final streamId, :final topic):
await markTopicAsRead(connection, streamId: streamId, topicName: topic);
case DmNarrow():
final unreadDms = store.unreads.dms[narrow];
// Silently ignore this race-condition as the outcome
// (no unreads in this narrow) was the desired end-state
// of pushing the button.
if (unreadDms == null) return;
await updateMessageFlags(connection,
messages: unreadDms,
op: UpdateMessageFlagsOp.add,
flag: MessageFlag.read);
case MentionsNarrow():
final unreadMentions = store.unreads.mentions.toList();
if (unreadMentions.isEmpty) return;
await updateMessageFlags(connection,
messages: unreadMentions,
op: UpdateMessageFlagsOp.add,
flag: MessageFlag.read);
case StarredMessagesNarrow():
// TODO: Implement unreads handling.
return;
}
}

/// Fetch and return the raw Markdown content for [messageId],
/// showing an error dialog on failure.
static Future<String?> fetchRawContentWithFeedback({
Expand Down
72 changes: 0 additions & 72 deletions test/api/route/messages_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,76 +829,4 @@ void main() {
});
});
});

group('markAllAsRead', () {
Future<void> checkMarkAllAsRead(
FakeApiConnection connection, {
required Map<String, String> expected,
}) async {
connection.prepare(json: {});
await markAllAsRead(connection);
check(connection.lastRequest).isA<http.Request>()
..method.equals('POST')
..url.path.equals('/api/v1/mark_all_as_read')
..bodyFields.deepEquals(expected);
}

test('smoke', () {
return FakeApiConnection.with_((connection) async {
await checkMarkAllAsRead(connection, expected: {});
});
});
});

group('markStreamAsRead', () {
Future<void> checkMarkStreamAsRead(
FakeApiConnection connection, {
required int streamId,
required Map<String, String> expected,
}) async {
connection.prepare(json: {});
await markStreamAsRead(connection, streamId: streamId);
check(connection.lastRequest).isA<http.Request>()
..method.equals('POST')
..url.path.equals('/api/v1/mark_stream_as_read')
..bodyFields.deepEquals(expected);
}

test('smoke', () {
return FakeApiConnection.with_((connection) async {
await checkMarkStreamAsRead(connection,
streamId: 10,
expected: {'stream_id': '10'});
});
});
});

group('markTopicAsRead', () {
Future<void> checkMarkTopicAsRead(
FakeApiConnection connection, {
required int streamId,
required String topicName,
required Map<String, String> expected,
}) async {
connection.prepare(json: {});
await markTopicAsRead(connection,
streamId: streamId, topicName: eg.t(topicName));
check(connection.lastRequest).isA<http.Request>()
..method.equals('POST')
..url.path.equals('/api/v1/mark_topic_as_read')
..bodyFields.deepEquals(expected);
}

test('smoke', () {
return FakeApiConnection.with_((connection) async {
await checkMarkTopicAsRead(connection,
streamId: 10,
topicName: 'topic',
expected: {
'stream_id': '10',
'topic_name': 'topic',
});
});
});
});
}
101 changes: 0 additions & 101 deletions test/widgets/actions_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import '../api/fake_api.dart';
import '../example_data.dart' as eg;
import '../flutter_checks.dart';
import '../model/binding.dart';
import '../model/unreads_checks.dart';
import '../stdlib_checks.dart';
import '../test_clipboard.dart';
import 'dialog_checks.dart';
Expand Down Expand Up @@ -119,106 +118,6 @@ void main() {
await future;
check(store.unreads.oldUnreadsMissing).isFalse();
});

testWidgets('CombinedFeedNarrow on legacy server', (tester) async {
const narrow = CombinedFeedNarrow();
await prepare(tester);
// Might as well test with oldUnreadsMissing: true.
store.unreads.oldUnreadsMissing = true;

connection.zulipFeatureLevel = 154;
connection.prepare(json: {});
final future = ZulipAction.markNarrowAsRead(context, narrow);
await tester.pump(Duration.zero);
await future;
check(connection.lastRequest).isA<http.Request>()
..method.equals('POST')
..url.path.equals('/api/v1/mark_all_as_read')
..bodyFields.deepEquals({});

// Check that [Unreads.handleAllMessagesReadSuccess] wasn't called;
// in the legacy protocol, that'd be redundant with the mark-read event.
check(store.unreads).oldUnreadsMissing.isTrue();
});

testWidgets('ChannelNarrow on legacy server', (tester) async {
final stream = eg.stream();
final narrow = ChannelNarrow(stream.streamId);
await prepare(tester);
connection.zulipFeatureLevel = 154;
connection.prepare(json: {});
final future = ZulipAction.markNarrowAsRead(context, narrow);
await tester.pump(Duration.zero);
await future;
check(connection.lastRequest).isA<http.Request>()
..method.equals('POST')
..url.path.equals('/api/v1/mark_stream_as_read')
..bodyFields.deepEquals({
'stream_id': stream.streamId.toString(),
});
});

testWidgets('TopicNarrow on legacy server', (tester) async {
final narrow = TopicNarrow.ofMessage(eg.streamMessage());
await prepare(tester);
connection.zulipFeatureLevel = 154;
connection.prepare(json: {});
final future = ZulipAction.markNarrowAsRead(context, narrow);
await tester.pump(Duration.zero);
await future;
check(connection.lastRequest).isA<http.Request>()
..method.equals('POST')
..url.path.equals('/api/v1/mark_topic_as_read')
..bodyFields.deepEquals({
'stream_id': narrow.streamId.toString(),
'topic_name': narrow.topic,
});
});

testWidgets('DmNarrow on legacy server', (tester) async {
final message = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]);
final narrow = DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId);
final unreadMsgs = eg.unreadMsgs(dms: [
UnreadDmSnapshot(otherUserId: eg.otherUser.userId,
unreadMessageIds: [message.id]),
]);
await prepare(tester, unreadMsgs: unreadMsgs);
connection.zulipFeatureLevel = 154;
connection.prepare(json:
UpdateMessageFlagsResult(messages: [message.id]).toJson());
final future = ZulipAction.markNarrowAsRead(context, narrow);
await tester.pump(Duration.zero);
await future;
check(connection.lastRequest).isA<http.Request>()
..method.equals('POST')
..url.path.equals('/api/v1/messages/flags')
..bodyFields.deepEquals({
'messages': jsonEncode([message.id]),
'op': 'add',
'flag': 'read',
});
});

testWidgets('MentionsNarrow on legacy server', (tester) async {
const narrow = MentionsNarrow();
final message = eg.streamMessage(flags: [MessageFlag.mentioned]);
final unreadMsgs = eg.unreadMsgs(mentions: [message.id]);
await prepare(tester, unreadMsgs: unreadMsgs);
connection.zulipFeatureLevel = 154;
connection.prepare(json:
UpdateMessageFlagsResult(messages: [message.id]).toJson());
final future = ZulipAction.markNarrowAsRead(context, narrow);
await tester.pump(Duration.zero);
await future;
check(connection.lastRequest).isA<http.Request>()
..method.equals('POST')
..url.path.equals('/api/v1/messages/flags')
..bodyFields.deepEquals({
'messages': jsonEncode([message.id]),
'op': 'add',
'flag': 'read',
});
});
});

group('updateMessageFlagsStartingFromAnchor', () {
Expand Down