Delete Comments #2046

sketchydroide · 2021-03-24T18:40:17Z

Details

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/147483
Fixes #2966

Tests / QA Steps (web/desktop)

Hover over a message that you did not write, and verify that you do not see the Edit Comment or Delete Comment actions as options in the ReportActionContextMenu
Right-click a message that you did not write, and verify that you do not see the Edit Comment or Delete Comment actions as options in the ReportActionContextMenu
Hover over a message that you did write, and press Edit Comment
Make a change to the message, and press Save Changes.
Verify that the change takes effect.
Wait for a few seconds, then refresh the page.
Verify that the comment remains edited.
Right-click a message that you did write, and press Edit Comment
Make a change to the message, and press Save Changes.
Verify that the change takes effect.
Wait for a few seconds, then refresh the page.
Verify that the comment remains edited.
Hover over a message that you did write, and press Delete Comment
A confirmation modal should appear. Press cancel.
Verify that the message is not deleted.
Wait for a few seconds, then refresh the page.
Verify that the message is still not deleted.
Hover over a message that you did write, and press Delete Comment
A confirmation modal should appear. Press confirm.
Verify that the message no longer appears.
Wait for a few seconds, then refresh the page.
Verify that the message remains deleted.
Right-click a message that you did write, and press Delete Comment
A confirmation modal should appear. Press confirm.
Verify that the message no longer appears.
Wait for a few seconds, then refresh the page.
Verify that the message remains deleted.
Hover over a message that you did write, and press Edit Comment
Erase all the content of the message, then hit Save Changes.
Verify that the message no longer appears.
Wait for a few seconds, then refresh the page.
Verify that the message remains deleted.

Tests / QA Steps (iOS/Android/mWeb)

LongPress a message that you did not write, and verify that you do not see the Edit Comment or Delete Comment actions as options in the ReportActionContextMenu
LongPress a message that you did write, and press Edit Comment.
Make a change to the message and press Save Changes
Verify that the change takes effect.
(mWeb only) Wait for a few seconds, then refresh the page.
(mWeb only) Verify that the comment remains edited.
LongPress a message that you did write, and press Delete Comment
A confirmation modal should appear. Press cancel.
Verify that the message is not deleted.
(mWeb only) Wait for a few seconds, then refresh the page.
(mWeb only) Verify that the comment is still visible.
LongPress a message that you did write, and press Delete Comment
A confirmation modal should appear. Press confirm.
Verify that the message no longer appears.
(mWeb only) Wait for a few seconds, then refresh the page.
(mWeb only) Verify that the comment remains deleted.
LongPress a message that you did write, and press Edit Comment
Erase all the content of the message, then hit Save Changes
Verify that the message no longer appears.
(mWeb only) Wait for a few seconds, then refresh the page.
(mWeb only) Verify that the comment remains deleted.

Tested On

github-actions · 2021-03-24T18:40:40Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

sketchydroide · 2021-03-26T17:34:01Z

I have read the CLA Document and I hereby sign the CLA

# Conflicts: # src/libs/API.js # src/pages/home/report/ReportActionContextMenu.js # src/pages/home/report/ReportActionContextMenuItem.js # src/pages/home/report/ReportActionsView.js

but I'm going to try use ``` javascript import Onyx, {withOnyx} from 'react-native-onyx'; ``` instead

# Conflicts: # src/pages/home/report/ReportActionContextMenu.js # src/pages/home/report/ReportActionItem.js

roryabraham · 2021-05-21T16:00:50Z

src/pages/home/report/ReportActionContextMenu.js

@@ -112,9 +120,10 @@ class ReportActionContextMenu extends React.Component {
            {
                text: this.props.translate('reportActionContextMenu.editComment'),
                icon: Pencil,
-                shouldShow: this.props.reportAction.actorEmail === this.props.session.email
+                shouldShow: () => (
+                    this.canEdit


Suggested change

this.canEdit

this.canEdit()

# Conflicts: # src/languages/en.js

tgolen · 2021-05-24T16:42:44Z

src/pages/home/report/ReportActionContextMenu.js

+        /**
+         * A list of all the context actions in this menu.
+         */
+        this.contextActions = [


sooooo, heh, sorry to keep commenting here, but are these going to be moved outside the class?

tgolen · 2021-05-24T16:43:46Z

src/pages/home/report/ReportActionContextMenu.js

+    }
+
+    /**
+     Confirms the deletion of the comment and hides the confirmation modal.


Don't need this method doc (or the one below) at all (like PHP, they are only needed if the method is non-obvious)

tgolen · 2021-05-24T16:45:13Z

src/pages/home/report/ReportActionsView.js

@@ -248,7 +248,13 @@ class ReportActionsView extends React.Component {
    updateSortedReportActions(reportActions) {
        this.sortedReportActions = _.chain(reportActions)
            .sortBy('sequenceNumber')
-            .filter(action => action.actionName === 'ADDCOMMENT' || action.actionName === 'IOU')
+            .filter((action) => {
+                // Only show non-empty ADDCOMMENT actions or IOU actions


This comment does a good job of explaining "what" the code is doing, but not "why" it's doing it. The comment could be improved to explain why it's not showing any other actions.

tgolen

OK, thanks for moving that. After seeing it, I agree with you that it can be reverted. I didn't realize we would have to pass more than props and seeing all this stuff being passed around is worse than just keeping it in the constructor. There are probably some ways around all this, but let's just revert this last change and get this out.

This reverts commit f10de2e.

tgolen · 2021-05-25T19:54:33Z

I just tested this quickly and found an interesting bug.

On Web (maybe desktop too) Click the delete icon in the hover-menu
Verify the confirmation pops up
Move your mouse outside of the window
💥 notice that the confirmation window disappears
Move your mouse back inside the window
Once you hover over the same comment you are trying to delete, the confirmation window appears again

roryabraham · 2021-05-26T20:57:37Z

src/pages/home/report/ReportActionContextMenu.js

+     * @return {Boolean}
+     */
+    canEdit() {
+        return this.props.reportAction.actorEmail === this.props.session.email


@sketchydroide we also shouldn't be able to edit/delete IOU actions, or any other type of report action (not yet, anyways). So let's add a check for that here:

canEditReportAction() { return this.props.reportAction.actionName === 'ADDCOMMENT' // Bonus: make a constant for report action names && this.props.reportAction.actorEmail === this.props.session.email && this.props.reportAction.reportActionID; }

sketchydroide · 2021-05-27T12:07:12Z

I just tested this quickly and found an interesting bug.

On Web (maybe desktop too) Click the delete icon in the hover-menu

Verify the confirmation pops up

Move your mouse outside of the window

💥 notice that the confirmation window disappears

Move your mouse back inside the window

Once you hover over the same comment you are trying to delete, the confirmation window appears again

While I completly agree this is a bug, I would suggest creating a GH for it, this PR is already a bit complex, and fixing that bug would add further complexity.
But I think the solution would involve changing Hoverable here and add a state variable that we would use on ReportActionItem here and that when we show the ConfirmModal this would let the hoverable know that it can't be dismissed until the ConfirmModal is resolved

Not sure if that makes sense.

tgolen · 2021-05-27T15:39:14Z

OK, I'm fine with opening a separate GH to handle that.

sketchydroide · 2021-05-27T15:47:46Z

Ok @tgolen created a GH https://github.com/Expensify/Expensify/issues/165320

tgolen · 2021-05-27T15:51:10Z

src/libs/actions/Report.js

+        ...reportAction,
+        message: [
+            {
+                type: 'COMMENT',


Looks like this should be moved to a constant to be consistent with the other actions having constants too?

I think that is just the type of the message not the action, but I'm happy with adding those to constants as well

src/pages/home/report/ReportActionContextMenu.js

roryabraham

Okay, I think this is looking good 👍

tgolen

Looks good! In some future improvement, we probably could allow editing the optimistic comment. In that case, it would just fire off a second edit comment request to the API and things would probably work fine.

tgolen · 2021-05-28T18:07:07Z

and that would allow you to edit comments while offline, which would be kinda cool...

OSBotify · 2021-05-28T18:07:52Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

started work on this

4607f2c

sketchydroide self-assigned this Mar 24, 2021

sketchydroide requested a review from a team as a code owner March 24, 2021 18:40

sketchydroide marked this pull request as draft March 24, 2021 18:40

botify requested review from Jag96 and removed request for a team March 24, 2021 18:40

sketchydroide added 3 commits March 24, 2021 18:53

correcting the command, and cleaning a bit

fbdba19

editing adding press

4f78dbd

adding on press on the correct side

831e55a

sketchydroide removed the request for review from Jag96 March 26, 2021 16:53

I need to give myself the beta

b667c12

sketchydroide added 10 commits April 6, 2021 17:30

Using ONYX to call delete comment

83811f4

Merge branch 'master' into afonseca_delete_comments

3f5e2e3

# Conflicts: # src/libs/API.js # src/pages/home/report/ReportActionContextMenu.js # src/pages/home/report/ReportActionContextMenuItem.js # src/pages/home/report/ReportActionsView.js

a couple of small changes needed after the merge

e4719dd

ok this works now,

3bc3e83

but I'm going to try use ``` javascript import Onyx, {withOnyx} from 'react-native-onyx'; ``` instead

updating the name and how we call deleteReportAction

7c44b08

transforming ReportActionContextMenu into a class

ed28d4c

doing an optimistic response

bc5b393

using the correct copy

7a38825

Merge branch 'master' into afonseca_delete_comments

9127eea

# Conflicts: # src/pages/home/report/ReportActionContextMenu.js # src/pages/home/report/ReportActionItem.js

need to use sequence number for now

e144e3a

sketchydroide changed the title ~~[WIP] started work on this~~ [WIP] Delete Comments Apr 9, 2021

sketchydroide added 3 commits April 9, 2021 19:22

cleaning

0d6827a

fixing linter

91f915d

filtering this correctly

802b053

sketchydroide changed the title ~~[WIP] Delete Comments~~ Delete Comments Apr 12, 2021

sketchydroide requested review from roryabraham and a team April 12, 2021 15:50

after adding binding this works

9c73a88

roryabraham requested changes May 21, 2021

View reviewed changes

Merge branch 'main' into afonseca_delete_comments

e9afb8b

# Conflicts: # src/languages/en.js

sketchydroide requested review from tgolen, yuwenmemon and roryabraham May 24, 2021 14:27

tgolen requested changes May 24, 2021

View reviewed changes

sketchydroide added 2 commits May 24, 2021 17:54

removing some unneeded comments, and adding a specification to another

da1136f

remove contextActions from outside class.

f10de2e

tgolen reviewed May 24, 2021

View reviewed changes

Revert "remove contextActions from outside class."

d7691d5

This reverts commit f10de2e.

roryabraham requested changes May 26, 2021

View reviewed changes

sketchydroide added 2 commits May 27, 2021 12:13

Merge branch 'main' into afonseca_delete_comments

4b85cc1

only show the delete and edit for ADDCOMMENTS

38e2d40

tgolen requested changes May 27, 2021

View reviewed changes

sketchydroide added 2 commits May 27, 2021 17:19

Adding a const for the Message type Comment as well

d0e8520

add a comment to canEdit

6122f49

roryabraham approved these changes May 27, 2021

View reviewed changes

sketchydroide requested a review from tgolen May 28, 2021 16:29

tgolen approved these changes May 28, 2021

View reviewed changes

tgolen merged commit e7ec636 into main May 28, 2021

tgolen deleted the afonseca_delete_comments branch May 28, 2021 18:07

This was referenced May 31, 2021

Deploy Checklist: Expensify.cash 2021-05-28 #3199

Closed

Delete comment - Deleted comment is still visible in receiver side #3249

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete Comments #2046

Delete Comments #2046

sketchydroide commented Mar 24, 2021 •

edited by yuwenmemon

Loading

github-actions bot commented Mar 24, 2021 •

edited

Loading

sketchydroide commented Mar 26, 2021

roryabraham May 21, 2021

tgolen May 24, 2021

tgolen May 24, 2021

tgolen May 24, 2021

tgolen left a comment

tgolen commented May 25, 2021

roryabraham May 26, 2021

sketchydroide commented May 27, 2021

tgolen commented May 27, 2021

sketchydroide commented May 27, 2021

tgolen May 27, 2021

sketchydroide May 27, 2021

roryabraham left a comment

tgolen left a comment

tgolen commented May 28, 2021

OSBotify commented May 28, 2021

Delete Comments #2046

Delete Comments #2046

Conversation

sketchydroide commented Mar 24, 2021 • edited by yuwenmemon Loading

Details

Fixed Issues

Tests / QA Steps (web/desktop)

Tests / QA Steps (iOS/Android/mWeb)

Tested On

github-actions bot commented Mar 24, 2021 • edited Loading

sketchydroide commented Mar 26, 2021

roryabraham May 21, 2021

Choose a reason for hiding this comment

tgolen May 24, 2021

Choose a reason for hiding this comment

tgolen May 24, 2021

Choose a reason for hiding this comment

tgolen May 24, 2021

Choose a reason for hiding this comment

tgolen left a comment

Choose a reason for hiding this comment

tgolen commented May 25, 2021

roryabraham May 26, 2021

Choose a reason for hiding this comment

sketchydroide commented May 27, 2021

tgolen commented May 27, 2021

sketchydroide commented May 27, 2021

tgolen May 27, 2021

Choose a reason for hiding this comment

sketchydroide May 27, 2021

Choose a reason for hiding this comment

roryabraham left a comment

Choose a reason for hiding this comment

tgolen left a comment

Choose a reason for hiding this comment

tgolen commented May 28, 2021

OSBotify commented May 28, 2021

sketchydroide commented Mar 24, 2021 •

edited by yuwenmemon

Loading

github-actions bot commented Mar 24, 2021 •

edited

Loading