Modify/don't provide 'Reply' option for messages that can't be replied to #3811

agrawal-d · 2020-01-16T05:42:05Z

Instead of removing the reply option in an announcement stream, I have modified it to show 'Navigate to topic', since one might still want to go to the topic narrow, where the reply option is already disabled.
Option remains 'Reply' is the app user is an admin.
Also removes reply option in group PMs.

Screenshots

agrawal-d · 2020-01-16T09:44:19Z

@ray-kraesig please review.

rk-for-zulip · 2020-01-20T18:01:27Z

src/message/messageActionSheet.js

+    if (isAnnouncementOnly) {
+      reply.title = 'Navigate to topic';
+    }


This will permanently modify reply.title – it'll never be "Reply" again, or at least, not until the application is shut down and restarted.

(The fact that all these action-description-objects are mutable is a bug, not a feature. Please don't exploit it.)

rk-for-zulip · 2020-01-20T18:02:22Z

static/translations/messages_en.json

@@ -47,6 +47,7 @@
  "Narrow to conversation": "Narrow to conversation",
  "Reply": "Reply",
  "Add a reaction": "Add a reaction",
+  "Navigate to topic":"Navigate to topic",


Space after the colon, for consistency. (Ditto for en_GB.)

rk-for-zulip · 2020-01-20T18:39:26Z

src/message/messageActionSheet.js

-  if (!isAnOutboxMessage(message) && !isTopicNarrow(narrow) && !isPrivateNarrow(narrow)) {
-    if (isAnnouncementOnly) {
+  if (!isAnOutboxMessage(message) && !isTopicNarrow(narrow) && !isPrivateOrGroupNarrow(narrow)) {
+    if (isAnnouncementOnly && !ownIsAdmin) {


These two changes need to be in separate commits.

rk-for-zulip · 2020-01-20T19:00:58Z

src/webview/MessageList.js

@@ -75,6 +75,7 @@ export type BackgroundData = $ReadOnly<{
  mute: MuteState,
  ownEmail: string,
  ownUserId: number,
+  ownIsAdmin: boolean,


isAdmin will do. (The own affix here is of questionable utility anyway, IMO, even where it's grammatical.)

Also, please make sure the ordering of properties is consistent here and below.

(The own affix here is of questionable utility anyway, IMO, even where it's grammatical.)

I think it's important for "email" and "user id" and "user". There are lots and lots of places we have a variable (or parameter or property) named email or userId or user, and they normally mean "(the email or ID of) the user this widget, or this scrap of data, is about". When it's instead "the user that is looking at the app", that has a very different feel and it's good to make the contrast explicit.

I agree it's not helpful for "is admin", and probably most other possible names.

agrawal-d · 2020-01-21T13:40:06Z

@ray-kraesig have pushed some changes.

agrawal-d · 2020-01-30T14:05:02Z

Blocked until #3795 is merged.

agrawal-d · 2020-02-01T10:12:52Z

#3795 has been merged :-)
@ray-kraesig please review this, thanks!

rk-for-zulip · 2020-02-03T23:24:46Z

Code is good. Some commit-message issues:

Both commit summaries are section: uncapitalized sentence with a period., which is neither the preferred Zulip official standard (capitalization required) nor the less-preferred git.git style (no period).
The second commit doesn't actually have anything to do with Don't provide 'Reply' option for messages that can't be replied to #3810 and shouldn't say it fixes it.
The second commit's message says it eliminates "Reply" in topic narrows... but that's already true in master.

The first and second are uninteresting (if it were just those two I would do the copyediting myself before merging), but the third is odd enough that I have to ask what the intent was there.

Currently, it is possible to select the 'Reply' option from action sheet when in an announcement-only narrow, even if the user is not an admin. Modify 'Reply' option based on whether the user is an administrator. Fixes zulip#3810.

agrawal-d · 2020-02-04T11:52:28Z

Thanks for reviewing.
For your second point - I thought the second commit's message made sense because it is removing the Reply option in a place where it shouldn't be possible to reply - group private messages. I have removed it now.

For your third point - I made a mistake 😓 - Instead of documenting the change I made, I documented the entire feature.

I have reworded both the commit messages.

Remove the 'Reply' option from messageActionSheet when the user is in a group private message narrow.

rk-for-zulip · 2020-02-04T19:49:37Z

For your second point - I thought the second commit's message made sense because it is removing the Reply option in a place where it shouldn't be possible to reply - group private messages. I have removed it now.

It's entirely possible to reply to a group PM, though – you can still type into the compose box and send a message and have it be seen; it's just that the current "Reply" option doesn't have an effect there.

(This contrasts with announcement-only streams, where even if you send a message to the server it won't be accepted.)

For your third point - I made a mistake - Instead of documenting the change I made, I documented the entire feature.

I figured that was all it was, but I had to ask.

I have reworded both the commit messages.

Missed the commit summary on the second one, though. 🙂 Amended, and merging. Thanks!

gnprice

Thanks @agrawal-d , and thanks @ray-kraesig for the review!

This doesn't actually quite work, because of a subtle issue: the logic is in terms of the current narrow, but the question of whether the user can reply is about the specific message. And we have a lot of possible types of narrows, several of which cut across streams (and across PM conversations): starred messages, @-mentions, searches, and the "All messages" view. (There's also the all-PMs narrow.)

The result is that after this change, we still offer "Reply" even for messages in announcement-only streams, if you're looking at them through one of those narrows.

The right fix for this will involve some different plumbing for the data, which means a good bit of the code involved will be different from this. So I'm going to revert the main commit from this PR and reopen the issue; then we can make another go at it.

The key thing we'll want in the plumbing to make v2 of this is: instead of a prop like isAnnouncementOnly, let's add streams to the BackgroundData type. Then in constructMessageActionButtons, we can use the message (not the narrow) to look up the Stream object there, and read off is_announcement_only.

gnprice · 2020-02-04T22:15:40Z

src/message/messageActionSheet.js

+  if (!isAnOutboxMessage(message) && !isTopicNarrow(narrow) && !isPrivateOrGroupNarrow(narrow)) {
+    if (isAnnouncementOnly && !isAdmin) {
+      buttons.push('narrowToTopic');
+    } else {
+      buttons.push('reply');
+    }
  }


This is too many conditionals in one place 🙂 -- it's more than the reader, in particular the author, can reasonably keep track of in their head.

One symptom of that is that this causes "Narrow to topic" to appear only for non-outbox messages. That doesn't make sense.

This also leaves "Reply" in place when the message is in an announcement-only stream, any time you're in a narrow that isn't specific to the stream. You could be in the all-messages narrow; or starred messages, @-mentions, or a search.

gnprice · 2020-02-04T22:30:01Z

src/webview/webViewEventHandlers.js

  showActionSheet(target === 'header', dispatch, showActionSheetWithOptions, _, {
    backgroundData,
    message,
    narrow,
+    isAnnouncementOnly,
  });


One bonus of using backgroundData for the plumbing: it saves us from making a longer and longer list here of things we just pass through without change. (That was part of the origin of the backgroundData object in this message-list code.)

… narrow." This reverts commit 674f9a2 (#3811). That commit fixed only some cases of #3810: it relies on the current narrow rather than the specific message, and so it misses relevant messages when we're in a starred narrow, a search, etc. The right fix will involve plumbing a different set of data down through these layers, rather than `isAnnouncementOnly`, so it'll be simpler to do that fresh than starting from here.

gnprice · 2020-02-04T22:37:58Z

Reverted that commit, as 8f58f46.

… narrow." This reverts commit 674f9a2 (zulip#3811). That commit fixed only some cases of zulip#3810: it relies on the current narrow rather than the specific message, and so it misses relevant messages when we're in a starred narrow, a search, etc. The right fix will involve plumbing a different set of data down through these layers, rather than `isAnnouncementOnly`, so it'll be simpler to do that fresh than starting from here.

agrawal-d requested a review from rk-for-zulip January 16, 2020 05:42

agrawal-d mentioned this pull request Jan 16, 2020

Don't provide 'Reply' option for messages that can't be replied to #3810

Open

agrawal-d changed the title ~~Announcements~~ Don't provide/Modify 'Reply' option for messages that can't be replied to Jan 16, 2020

agrawal-d changed the title ~~Don't provide/Modify 'Reply' option for messages that can't be replied to~~ Don't provide/modify 'Reply' option for messages that can't be replied to Jan 16, 2020

agrawal-d changed the title ~~Don't provide/modify 'Reply' option for messages that can't be replied to~~ Modify/don't provide 'Reply' option for messages that can't be replied to Jan 16, 2020

agrawal-d force-pushed the announcements branch from 11105a8 to 3677f1b Compare January 16, 2020 09:42

rk-for-zulip reviewed Jan 20, 2020

View reviewed changes

agrawal-d force-pushed the announcements branch from 3677f1b to 034bf20 Compare January 21, 2020 13:38

agrawal-d requested a review from rk-for-zulip January 21, 2020 13:39

agrawal-d force-pushed the announcements branch from 034bf20 to 9c2b533 Compare January 30, 2020 13:56

agrawal-d force-pushed the announcements branch from 9c2b533 to 6fe67ca Compare February 1, 2020 09:57

agrawal-d force-pushed the announcements branch from 6fe67ca to a4df35a Compare February 1, 2020 10:25

agrawal-d requested review from rk-for-zulip and removed request for rk-for-zulip February 1, 2020 20:21

agrawal-d force-pushed the announcements branch from a4df35a to 20ab23d Compare February 4, 2020 11:44

message list: Remove "Reply" action when in group PM narrow.

b902d55

Remove the 'Reply' option from messageActionSheet when the user is in a group private message narrow.

rk-for-zulip force-pushed the announcements branch from 20ab23d to b902d55 Compare February 4, 2020 19:49

rk-for-zulip merged commit 4d3cec3 into zulip:master Feb 4, 2020

agrawal-d deleted the announcements branch February 4, 2020 20:36

gnprice reviewed Feb 4, 2020

View reviewed changes

gnprice mentioned this pull request Feb 4, 2020

message list [nfc]: Small cleanups around BackgroundData to prepare for adding more. #3867

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify/don't provide 'Reply' option for messages that can't be replied to #3811

Modify/don't provide 'Reply' option for messages that can't be replied to #3811

agrawal-d commented Jan 16, 2020 •

edited

Loading

agrawal-d commented Jan 16, 2020

rk-for-zulip Jan 20, 2020

rk-for-zulip Jan 20, 2020

rk-for-zulip Jan 20, 2020

rk-for-zulip Jan 20, 2020

gnprice Feb 4, 2020

agrawal-d commented Jan 21, 2020

agrawal-d commented Jan 30, 2020

agrawal-d commented Feb 1, 2020 •

edited

Loading

rk-for-zulip commented Feb 3, 2020

agrawal-d commented Feb 4, 2020 •

edited

Loading

rk-for-zulip commented Feb 4, 2020

gnprice left a comment

gnprice Feb 4, 2020

gnprice Feb 4, 2020

gnprice commented Feb 4, 2020

Modify/don't provide 'Reply' option for messages that can't be replied to #3811

Modify/don't provide 'Reply' option for messages that can't be replied to #3811

Conversation

agrawal-d commented Jan 16, 2020 • edited Loading

Screenshots

agrawal-d commented Jan 16, 2020

rk-for-zulip Jan 20, 2020

Choose a reason for hiding this comment

rk-for-zulip Jan 20, 2020

Choose a reason for hiding this comment

rk-for-zulip Jan 20, 2020

Choose a reason for hiding this comment

rk-for-zulip Jan 20, 2020

Choose a reason for hiding this comment

gnprice Feb 4, 2020

Choose a reason for hiding this comment

agrawal-d commented Jan 21, 2020

agrawal-d commented Jan 30, 2020

agrawal-d commented Feb 1, 2020 • edited Loading

rk-for-zulip commented Feb 3, 2020

agrawal-d commented Feb 4, 2020 • edited Loading

rk-for-zulip commented Feb 4, 2020

gnprice left a comment

Choose a reason for hiding this comment

gnprice Feb 4, 2020

Choose a reason for hiding this comment

gnprice Feb 4, 2020

Choose a reason for hiding this comment

gnprice commented Feb 4, 2020

agrawal-d commented Jan 16, 2020 •

edited

Loading

agrawal-d commented Feb 1, 2020 •

edited

Loading

agrawal-d commented Feb 4, 2020 •

edited

Loading