fix: Removed delete message option for message and allowed empty content during editing message #5632

KshitizSareen · 2022-12-31T08:01:43Z

I aligned the content deletion experience with the web app. The changes I made allowed users to edit the message when there is empty content. When the user enters empty content, and edits the message, it will show as deleted.

I did this by adding an if condition checking if the composebox is in isEditing mode in ComposeBox.js. If it is not in isEditing, I add the message-empty error as the validation error. If it is in isEditing, I don't add the error.

I also changed the if condition in the ChatScreen.js, where I removed the content == "" condition. I removed it to allow empty content to be accepted as input when editing messages.

I finally commented on the buttons.push(deleteMessage) for message.isOutbox === true condition and the message.sender_id === ownUser.user_id && messageNotDeleted(message) condition in index.js, which is for actionsheets. I also added another condition in the same file (ownUser.role === 200 && messageNotDeleted(message)) to allow admins to delete messages. (I used ownUser.role instead of ownUser.isAdmin as the isAdmin property is not readable).

Here is a demo.

VIdeo.Demo.For.Algin.Content.Experience.mov

I tested this change on both android and iOS devices, and its working fine for both instances.

fix: #5528

Please let me know of your feedback.
Thank you,
Regards,
Kshitiz.

chrisbobbe

Thanks! Comments below.

chrisbobbe · 2023-01-03T20:51:19Z

src/chat/ChatScreen.js

-        if ((content !== undefined && content !== '') || (topic !== undefined && topic !== '')) {
+        if (content !== undefined || (topic !== undefined && topic !== '')) {


In main, it looks like the content !== '' and topic !== '' checks aren't actually doing anything:

content can't be the empty string because of the message-empty validation rule in ComposeBox.

topic can't be the empty string because ComposeBox never passes an empty topic to its onSend callback (it passes "(no topic)" when the input is the empty string).

Probably, these checks were once used for input validation, and we forgot to remove them when we added the validationErrors code in ComposeBox.

So, let's remove them. That removal should go in a small preparatory NFC commit before your more interesting commit(s).

Hi, I will remove them.

chrisbobbe · 2023-01-03T20:54:03Z

src/chat/ChatScreen.js

+    (value: EditMessage | null) => {
+      navigation.setParams({ editMessage: value });
+    },


Was this intentional? If not, let's leave out this change; it's nice to fit all this code on one line.

It was unintentional. I will revert this change.

chrisbobbe · 2023-01-03T21:05:29Z

src/action-sheets/index.js

@@ -728,6 +728,9 @@ export const constructMessageActionButtons = (args: {|
  }
  if (message.sender_id === ownUser.user_id && messageNotDeleted(message)) {
    // TODO(#2793): Don't show if message isn't deletable.
+    // buttons.push(deleteMessage);
+  }
+  if (ownUser.role === 200 && messageNotDeleted(message)) {
    buttons.push(deleteMessage);
  }


In general we try to avoid committing commented-out code, especially without a code comment explaining why and what it's about.

Do I understand correctly that you're trying to fix both #5528 and #4701 at the same time? I believe the way forward for the latter is #4701 (comment):

[…]

If you're an admin, you have permission.

Otherwise, if you didn't send the message, you don't have permission.

Otherwise (so, you sent the message but you're not an admin), it gets complicated -- you might have permission, and might not.

I think we can round off that third point to assuming you have permission, and showing the option in the UI. […]

The code in this revision doesn't match that:

It should show the button for a non-admin user's own messages.

The messageNotDeleted thing (checking if the message's content is <p>(deleted)</p>) should be irrelevant; probably that should be removed as part of dealing with Align message content deletion experience with web app #5528.

Also, if your commit is meant to fix two issues at once, please put a Fixes: #… line for both issues in the commit message, and also in the PR description: https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#github-prs-issues

Ah, and there is a role that's even more powerful than an admin: an owner. When admins have permission to do something, owners should too.

For an example of another admin role check in this file, see constructTopicActionButtons:

if (roleIsAtLeast(ownUserRole, Role.Admin)) { buttons.push(deleteTopic); }

chrisbobbe · 2023-01-03T21:09:58Z

src/action-sheets/index.js

@@ -697,7 +697,7 @@ export const constructMessageActionButtons = (args: {|
  if (message.isOutbox === true) {
    buttons.push(copyToClipboard);
    buttons.push(shareMessage);
-    buttons.push(deleteMessage);
+    // buttons.push(deleteMessage);


Why comment this out? It looks like deleteMessage wants to do something interesting when message.isOutbox === true.

So the idea was to fix issue #5528, and once those changes are merged, I would move on to issue #4701. It states that issue #4701 is blocked on #5528. I only intended to fix issue #5528. In Issue #5528, it states that I should drop the "Delete message" option from the menu for your own messages.

Your comments make sense. I will leave the button for non-admin user's own messages, and I will replace the if condition to allow users with the minimum role of admin to delete the message.

I was not sure of whether I should not show the button when message.isOutbox == true. I raised that question in the issue itself. It seems like I should leave it there.

Let me make the proposed changes.

Thanks.

KshitizSareen · 2023-01-17T08:22:54Z

Hi @chrisbobbe. I pushed some changes. Please review them.

Regards,
Kshitiz.

chrisbobbe

Thanks for the revision!

So the idea was to fix issue #5528, and once those changes are merged, I would move on to issue #4701. It states that issue #4701 is blocked on #5528. I only intended to fix issue #5528. In Issue #5528, it states that I should drop the "Delete message" option from the menu for your own messages.

Ah, OK, makes sense. From your previous revision, it appeared that you were trying to fix both issues at once. 🙂

I'm still a bit confused, though. This revision doesn't drop the "Delete message" option from the menu for your own messages, in the non-outbox case. Please do that.

In my previous review, this comment—

Why comment this out? It looks like deleteMessage wants to do something interesting when message.isOutbox === true.

—was only referring to the message.isOutbox === true case. I think that can stay in, because it's not a case where we use "Delete message" to mean "replace contents with the string '(deleted)'". Perhaps it should be given a different name, like "Cancel sending", if that's actually what it does (I haven't fully checked).

chrisbobbe · 2023-01-17T22:03:26Z

src/compose/ComposeBox.js

  }, [
+    messageInputState,
    destinationNarrow,
    mandatoryTopics,
+    isEditing,
    numUploading,
    anyQuoteAndReplyInProgress,
-    messageInputState,
  ]);


nit: No need to change the order of existing items in this array (messageInputState)

chrisbobbe · 2023-01-17T22:06:52Z

src/chat/ChatScreen.js

-        if ((content !== undefined && content !== '') || (topic !== undefined && topic !== '')) {
+        if (content !== undefined || topic !== undefined) {


This change should go in a separate commit that makes clear the reason for the change, as I mentioned in #5632 (comment).

…pty strings

…essage.isOutbox == true

KshitizSareen · 2023-01-18T03:13:13Z

Hi @chrisbobbe. I pushed some changes. Please review them.

Regards,
Kshitiz.

KshitizSareen force-pushed the zulip-mobile-align-message-content branch from 4b5983a to c5cd6a7 Compare December 31, 2022 09:35

chrisbobbe reviewed Jan 3, 2023

View reviewed changes

KshitizSareen force-pushed the zulip-mobile-align-message-content branch 2 times, most recently from 7e1a196 to cfe26ad Compare January 17, 2023 08:07

chrisbobbe requested changes Jan 17, 2023

View reviewed changes

Removed unnceccessary checks for checking if content and topic are em…

833e7c4

…pty strings

KshitizSareen force-pushed the zulip-mobile-align-message-content branch from 91fd970 to 833e7c4 Compare January 18, 2023 02:50

KshitizSareen added 2 commits January 17, 2023 18:51

Removed delete message option of own users messages and left it for m…

fd14b17

…essage.isOutbox == true

Removed Validation error of Message Empty when user is in editing mode

a217579

Merge branch 'zulip:main' into zulip-mobile-align-message-content

3a350ef

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Removed delete message option for message and allowed empty content during editing message #5632

fix: Removed delete message option for message and allowed empty content during editing message #5632

KshitizSareen commented Dec 31, 2022 •

edited

chrisbobbe left a comment

chrisbobbe Jan 3, 2023

KshitizSareen Jan 5, 2023

chrisbobbe Jan 3, 2023

KshitizSareen Jan 5, 2023

chrisbobbe Jan 3, 2023

chrisbobbe Jan 3, 2023

chrisbobbe Jan 3, 2023

KshitizSareen Jan 5, 2023 •

edited

KshitizSareen commented Jan 17, 2023

chrisbobbe left a comment

chrisbobbe Jan 17, 2023

chrisbobbe Jan 17, 2023

KshitizSareen commented Jan 18, 2023

		if ((content !== undefined && content !== '') \|\| (topic !== undefined && topic !== '')) {
		if (content !== undefined \|\| (topic !== undefined && topic !== '')) {

		if ((content !== undefined && content !== '') \|\| (topic !== undefined && topic !== '')) {
		if (content !== undefined \|\| topic !== undefined) {

fix: Removed delete message option for message and allowed empty content during editing message #5632

Are you sure you want to change the base?

fix: Removed delete message option for message and allowed empty content during editing message #5632

Conversation

KshitizSareen commented Dec 31, 2022 • edited

chrisbobbe 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

KshitizSareen Jan 5, 2023 • edited

Choose a reason for hiding this comment

KshitizSareen commented Jan 17, 2023

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KshitizSareen commented Jan 18, 2023

KshitizSareen commented Dec 31, 2022 •

edited

KshitizSareen Jan 5, 2023 •

edited