Skip to content
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.

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

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

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

sirpengi
Copy link
Contributor

@sirpengi sirpengi commented Nov 2, 2023

This pr was split off from #361.

Also fixes part of #130

@sirpengi sirpengi changed the title Api: mark messages as read (markAllAsRead, markStreamAsRead, markTopicAsRead) api: Mark messages as read (markAllAsRead, markStreamAsRead, markTopicAsRead) Nov 2, 2023
@sirpengi sirpengi requested a review from gnprice November 3, 2023 12:02
@sirpengi sirpengi mentioned this pull request Nov 3, 2023
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good modulo two small comments (and the completion of #361, which this is stacked upon).

/// https://zulip.com/api/mark-stream-as-read
///
/// This binding is deprecated, in FL 155+ use
/// updateMessageFlagsForNarrow instead.
Copy link
Member

Choose a reason for hiding this comment

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

nit: linkify:

Suggested change
/// updateMessageFlagsForNarrow instead.
/// [updateMessageFlagsForNarrow] instead.

}) async {
connection.prepare(json: {});
await markStreamAsRead(connection, streamId: streamId);
check(connection.lastRequest).isNotNull().isA<http.Request>()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
check(connection.lastRequest).isNotNull().isA<http.Request>()
check(connection.lastRequest).isA<http.Request>()

As I mentioned in our call yesterday, the isNotNull is redundant with the subsequent isA. The type we're checking for with isA already excludes null.

(I think I originally introduced this redundant pattern to this file. But we should stop propagating it.)

Copy link
Member

Choose a reason for hiding this comment

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

#369 for the existing cases of that pattern.

@sirpengi
Copy link
Contributor Author

sirpengi commented Nov 6, 2023

@gnprice this is ready as well!

@gnprice
Copy link
Member

gnprice commented Nov 7, 2023

Thanks! Looks good; merging.

@gnprice gnprice merged commit 6d9f22d into zulip:main Nov 7, 2023
1 check passed
@sirpengi sirpengi deleted the pr-api-mark-foo-as-read branch January 23, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants