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

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

KshitizSareen
Copy link

@KshitizSareen KshitizSareen commented Dec 31, 2022

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.

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

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

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

Thanks! Comments below.

Comment on lines 167 to 169
if ((content !== undefined && content !== '') || (topic !== undefined && topic !== '')) {
if (content !== undefined || (topic !== undefined && topic !== '')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

Hi, I will remove them.

Comment on lines 134 to 136
(value: EditMessage | null) => {
navigation.setParams({ editMessage: value });
},
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

It was unintentional. I will revert this change.

@@ -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);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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);
  }

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

Choose a reason for hiding this comment

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

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

Copy link
Author

@KshitizSareen KshitizSareen Jan 5, 2023

Choose a reason for hiding this comment

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

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 KshitizSareen force-pushed the zulip-mobile-align-message-content branch 2 times, most recently from 7e1a196 to cfe26ad Compare January 17, 2023 08:07
@KshitizSareen
Copy link
Author

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

Regards,
Kshitiz.

Copy link
Contributor

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

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

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).

Comment on lines 537 to 543
}, [
messageInputState,
destinationNarrow,
mandatoryTopics,
isEditing,
numUploading,
anyQuoteAndReplyInProgress,
messageInputState,
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

if ((content !== undefined && content !== '') || (topic !== undefined && topic !== '')) {
if (content !== undefined || topic !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

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

Regards,
Kshitiz.

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.

Align message content deletion experience with web app
2 participants