api: Mark messages as read (updateMessageFlags and updateMessageFlagsForNarrow) #361

sirpengi · 2023-11-01T20:00:57Z

This PR adds all the api routes related to marking messages as read.

The complexities around the response of https://zulip.com/api/mark-all-as-read are ignored as it is deprecated (it is also ignored in zulip-mobile). On an examination of lib/api/core.dart it doesn't look like we check the value of the result field (where the status code is 200) so if the server returns partially_completed for the affected FLs it doesn't raise any errors (although we would treat the operation as completed).

Fixes part of #130

gnprice

Thanks @sirpengi! Comments below on the first three commits:
ba5e3dd api [nfc]: Refactor out logic that supports serializing legacy DmNarrow
58be9c9 api [nfc]: Pull out a method Anchor.toJson
346e94c api: Add updateMessageFlags and updateMessageFlagsForNarrow routes

Let's split out the remaining commit to a separate followup PR:
eabae44 api: Add markAllAsRead, markStreamAsRead, and markTopicAsRead routes

just to keep each PR simpler, since GitHub's UI doesn't do great as threads get longer.

gnprice · 2023-11-01T21:06:08Z

lib/api/model/narrow.dart

 typedef ApiNarrow = List<ApiNarrowElement>;

+/// Convenience function to resolve legacy elements of a narrow.


The description "resolve legacy elements" doesn't sound quite right to me — the input isn't expected to have legacy elements (rather it may have unresolved ApiNarrowDm elements), and the resolution is just as likely or more so to produce modern elements as to produce legacy elements.

Here's a version:

Suggested change

/// Convenience function to resolve legacy elements of a narrow.

/// Resolve any [ApiNarrowDm] elements appropriately.

And then resolveDmElements would work as a name.

(If we later have a second legacy/modern split to resolve, this name and description won't cover that, but we can handle that if it comes up.)

gnprice · 2023-11-01T21:07:45Z

lib/api/model/narrow.dart

+ApiNarrow resolveLegacyElements(ApiNarrow narrow, ApiConnection connection) {
+  final supportsOperatorDm = connection.zulipFeatureLevel! >= 177; // TODO(server-7)


Let's have it take an int zulipFeatureLevel instead — that feels more like the right layer for this to be operating at.

gnprice · 2023-11-01T21:13:06Z

lib/api/route/messages.dart

-  if (narrow.any((element) => element is ApiNarrowDm)) {
-    final supportsOperatorDm = connection.zulipFeatureLevel! >= 177; // TODO(server-7)
-    narrow = narrow.map((element) => switch (element) {


The narrow.any conditional here is an optimization that avoids unnecessarily copying the list when there's nothing to change. Let's preserve that in the refactor.

gnprice · 2023-11-01T21:19:29Z

lib/api/route/messages.dart

-      AnchorCode.oldest             => RawParameter('oldest'),
-      AnchorCode.firstUnread        => RawParameter('first_unread'),
-    },
+    'narrow': resolveLegacyElements(narrow, connection),


api [nfc]: Refactor out logic that supports serializing legacy DmNarrow

s/Refactor/Factor/

For me at least "refactor out" isn't a term; there's "refactor" for a general category of things you might do, one of which is to "factor out" something, but no "refactor out".

Also "legacy DmNarrow" doesn't quite make sense; DmNarrow itself isn't legacy, and nor is there such a thing as a legacy value of DmNarrow vs. a non-legacy value. (If talking about ApiNarrowDm, then there are modern and legacy versions of that — but because "DmNarrow" is spelled in a way that isn't normal English, it can only refer to the identifier with that name, not to the general concept of a "DM narrow".)

I think "supports serializing ApiNarrowDm" covers it. A necessary part of serializing one of those is to resolve it to modern or legacy.

gnprice · 2023-11-01T21:27:01Z

lib/api/route/messages.dart

+  });
+}
+
+/// The type of [updateMessageFlags.op] and [updateMessageFlagsForNarrow.op].


Suggested change

/// The type of [updateMessageFlags.op] and [updateMessageFlagsForNarrow.op].

/// An `op` value for [updateMessageFlags] or [updateMessageFlagsForNarrow].

The dotted syntax doesn't work for referring to a function parameter — basically because there's no such syntax in Dart itself for referring to such a thing.

gnprice · 2023-11-01T22:12:48Z

test/api/route/messages_test.dart

+      return result;
+    }
+    final fakeResult = UpdateMessageFlagsForNarrowResult(
+      processedCount: 11, updatedCount: 11, foundOldest: true,


nit: make updatedCount less than processedCount, because it often might be

gnprice · 2023-11-01T22:15:32Z

test/api/route/messages_test.dart

+        connection.prepare(json: fakeResult.toJson());
+        await checkUpdateMessageFlagsForNarrow(connection,
+          anchor: const NumericAnchor(42),
+          numBefore: 0, numAfter: 20,


nit: if numBefore is 0, then the real server behavior will be for foundOldest to be false (because it won't look to see if there are any older); so the test data should reflect that

(I'm not sure if that's still true when anchor is oldest, as above. Could do a quick experiment to find out. If not, then fakeResult could become a function taking {required bool foundOldest}.)

Confirmed by both a quick experiment and looking in zerver source that anchor=oldest does send true for foundOldest.

gnprice · 2023-11-01T22:18:22Z

test/api/route/messages_test.dart

+      });
+    });
+
+    test('narrow uses resolveLegacyElements to encode', () {


nit: put the "numeric anchor" and "narrow uses …" tests in the same order here as for getMessages, for ease of comparison

gnprice · 2023-11-01T22:19:45Z

test/api/route/messages_test.dart

+            'op': 'add',
+            'flag': 'read',
+          });
+        connection.zulipFeatureLevel = eg.futureZulipFeatureLevel;


Let's take this line out; I feel like it's likely to just confuse someone about the semantics of FakeApiConnection.with_, which are that it's a fresh connection that's thrown away.

gnprice · 2023-11-01T22:21:09Z

test/api/route/messages_test.dart

+      return FakeApiConnection.with_((connection) async {
+        connection.zulipFeatureLevel = 176;


Oh in fact better yet: pass the zulipFeatureLevel to with_, rather than mutating it later. See the getMessageCompat tests for examples.

sirpengi · 2023-11-02T15:05:05Z

@gnprice split off the last commit to #363, comments here handled and ready for another round!

gnprice

Thanks for the revision! Generally looks good — just a few small comments below.

gnprice · 2023-11-03T20:23:07Z

test/api/route/messages_test.dart

-          {'operator': 'pm-with', 'operand': [123, 234]},
-        ]));
-        connection.zulipFeatureLevel = eg.futureZulipFeatureLevel;
+    test('narrow uses resolveLegacyElements to encode', () {


test name no longer matches code

gnprice · 2023-11-03T20:30:03Z

lib/api/model/narrow.dart

+  final supportsOperatorDm = zulipFeatureLevel >= 177; // TODO(server-7)
+  return (narrow.any((element) => element is ApiNarrowDm))
+    ? narrow.map((element) => switch (element) {
+        ApiNarrowDm() => element.resolve(legacy: !supportsOperatorDm),
+        _             => element,
+      }).toList()
+    : narrow;


This feels a bit tangly to read, as I think often happens with the ?: operator when one or both branches are more than a line or two long. Let's leave it as full statement-level control flow, like it is in the existing code:

Suggested change

final supportsOperatorDm = zulipFeatureLevel >= 177; // TODO(server-7)

return (narrow.any((element) => element is ApiNarrowDm))

? narrow.map((element) => switch (element) {

ApiNarrowDm() => element.resolve(legacy: !supportsOperatorDm),

_ => element,

}).toList()

: narrow;

if (!narrow.any((element) => element is ApiNarrowDm)) {

return narrow;

}

final supportsOperatorDm = zulipFeatureLevel >= 177; // TODO(server-7)

return narrow.map((element) => switch (element) {

ApiNarrowDm() => element.resolve(legacy: !supportsOperatorDm),

_ => element,

}).toList();

gnprice · 2023-11-03T20:32:20Z

test/api/route/messages_test.dart

+      return result;
+    }
+
+    mkResult({required bool foundOldest}) =>


function definition needs explicit return type

…icit This came up in a code review here: zulip#361 (comment) and it'd be good to have written down in a more accessible place. There are some existing parts of the API bindings -- particularly the parts written in the very first week or two of the prototype, before I thought through this pattern -- that don't follow this. I'll fix those up next.

…icit This came up in a code review here: #361 (comment) and it'd be good to have written down in a more accessible place. There are some existing parts of the API bindings -- particularly the parts written in the very first week or two of the prototype, before I thought through this pattern -- that don't follow this. I'll fix those up next.

Also pull out narrow.toJson tests.

sirpengi · 2023-11-06T14:59:30Z

@gnprice this is ready again!

gnprice

(Looks like I had a small followup comment pending which I'd meant to post.)

gnprice · 2023-11-04T01:49:32Z

lib/api/route/messages.dart

+    this.firstProcessedId,
+    this.lastProcessedId,


I think the existing API bindings aren't 100% consistent about this

→ #367.

gnprice · 2023-11-06T18:11:08Z

Thanks for the revision! Looks good; merging.

sirpengi requested a review from gnprice November 1, 2023 20:07

gnprice reviewed Nov 1, 2023

View reviewed changes

sirpengi force-pushed the pr-api-mark-messages-as-read branch from eabae44 to 4d2c4bd Compare November 2, 2023 14:57

sirpengi changed the title ~~Api: mark messages as read~~ Api: mark messages as read (updateMessageFlags and updateMessageFlagsForNarrow) Nov 2, 2023

sirpengi mentioned this pull request Nov 2, 2023

api: Mark messages as read (markAllAsRead, markStreamAsRead, markTopicAsRead) #363

Merged

sirpengi changed the title ~~Api: mark messages as read (updateMessageFlags and updateMessageFlagsForNarrow)~~ api: mark messages as read (updateMessageFlags and updateMessageFlagsForNarrow) Nov 2, 2023

sirpengi changed the title ~~api: mark messages as read (updateMessageFlags and updateMessageFlagsForNarrow)~~ api: Mark messages as read (updateMessageFlags and updateMessageFlagsForNarrow) Nov 2, 2023

sirpengi mentioned this pull request Nov 3, 2023

Mark as read #364

Merged

gnprice reviewed Nov 3, 2023

View reviewed changes

gnprice mentioned this pull request Nov 4, 2023

api: Document, and sweep for, our practice of making constructor arguments explicit #367

Merged

sirpengi added 2 commits November 6, 2023 14:41

api [nfc]: Factor out logic that supports serializing ApiNarrowDm

db46fcf

Also pull out narrow.toJson tests.

api [nfc]: Pull out a method Anchor.toJson

dfb1a9e

sirpengi force-pushed the pr-api-mark-messages-as-read branch from 4d2c4bd to 0d16270 Compare November 6, 2023 14:51

api: Add updateMessageFlags and updateMessageFlagsForNarrow routes

dfcc6ca

sirpengi force-pushed the pr-api-mark-messages-as-read branch from 0d16270 to dfcc6ca Compare November 6, 2023 15:06

gnprice reviewed Nov 6, 2023

View reviewed changes

gnprice merged commit dfcc6ca into zulip:main Nov 6, 2023

sirpengi deleted the pr-api-mark-messages-as-read branch January 23, 2024 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: Mark messages as read (updateMessageFlags and updateMessageFlagsForNarrow) #361

api: Mark messages as read (updateMessageFlags and updateMessageFlagsForNarrow) #361

sirpengi commented Nov 1, 2023 •

edited

Loading

gnprice left a comment

gnprice Nov 1, 2023

gnprice Nov 1, 2023

gnprice Nov 1, 2023

gnprice Nov 1, 2023

gnprice Nov 1, 2023

gnprice Nov 1, 2023

gnprice Nov 1, 2023

sirpengi Nov 2, 2023

gnprice Nov 1, 2023

gnprice Nov 1, 2023

gnprice Nov 1, 2023

sirpengi commented Nov 2, 2023

gnprice left a comment

gnprice Nov 3, 2023

gnprice Nov 3, 2023

gnprice Nov 3, 2023

sirpengi commented Nov 6, 2023

gnprice left a comment

gnprice Nov 4, 2023

gnprice commented Nov 6, 2023

		typedef ApiNarrow = List<ApiNarrowElement>;

		/// Convenience function to resolve legacy elements of a narrow.

	/// Convenience function to resolve legacy elements of a narrow.
	/// Resolve any [ApiNarrowDm] elements appropriately.

		ApiNarrow resolveLegacyElements(ApiNarrow narrow, ApiConnection connection) {
		final supportsOperatorDm = connection.zulipFeatureLevel! >= 177; // TODO(server-7)

	/// The type of [updateMessageFlags.op] and [updateMessageFlagsForNarrow.op].
	/// An `op` value for [updateMessageFlags] or [updateMessageFlagsForNarrow].

		return FakeApiConnection.with_((connection) async {
		connection.zulipFeatureLevel = 176;

api: Mark messages as read (updateMessageFlags and updateMessageFlagsForNarrow) #361

api: Mark messages as read (updateMessageFlags and updateMessageFlagsForNarrow) #361

Conversation

sirpengi commented Nov 1, 2023 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirpengi commented Nov 2, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirpengi commented Nov 6, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Nov 6, 2023

sirpengi commented Nov 1, 2023 •

edited

Loading