Remove "Comment Deleted" message #7711

luacmartins · 2022-02-11T18:57:13Z

Details

Simply deletes a comment instead of showing [Comment deleted]

Fixed Issues

$ #7646

Tests

Pull the Auth PR changes and run ./script/makeAll.sh
Send multiple messages to another user
Delete some messages and notice that the message completely disappears
Delete the last message sent and verify that the last message in the LHN is updated accordingly
Refresh the app and notice that the last message in the LHN is correct

Note: if the LHN message does not update, you might have to make clean and run ./script/makeAll.sh again.

Verify that no errors appear in the JS console

QA Steps

Steps 2-4 above.

Verify that no errors appear in the JS console

Tested On

Screenshots

Web

web.mov

Mobile Web

mweb.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

MonilBhavsar

Looks good!
Would like to have eyes from someone more familiar
@marcaaron will you please review this since you were involved in issue discussion

marcaaron

Nice job! I have a couple of questions. Mostly around where we are updating the reportAction and whether we are updating the report object twice.

marcaaron · 2022-02-16T22:15:20Z

src/libs/actions/Report.js

@@ -551,15 +551,19 @@ function updateReportActionMessage(reportID, sequenceNumber, message) {
            return;
        }

-        // If the message is deleted, update the last read in case the deleted message is being counted in the unreadActionCount
+        // If the message is deleted, we should
+        // 1. update the last read in case the deleted message is being counted in the unreadActionCount


Capitalize Update?

Also, I don't entirely understand this comment "in case the deleted message is being counted". Can we tell if the deleted message is counted in the unreadActionCount or not? This comment reads like "maybe we need to do this".

It seems that we are not counting deleted messages in unreadActionCount since we subtract it from the total count here. I'm not sure why we had that comment. I'll do some more digging.

It seems that we are not counting deleted messages in unreadActionCount, but we want to update the last read and the counter anyways. I updated the comment to make it clear.

marcaaron · 2022-02-16T22:17:11Z

src/libs/actions/Report.js

        Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
-            lastMessageText: ReportUtils.formatReportLastMessageText(message.text) || `[${Localize.translateLocal('common.deletedCommentMessage')}]`,
+            lastMessageText: ReportActions.getLastMessageText(reportID),


I'm having trouble thinking of a better naming suggestion, but feels maybe not very obvious that ReportActions .getLastMessageText() would filter out deleted messages.

Yea, I struggled with that too. Maybe getLastVisibleMessageText or getLastRemainingMessageText?

I like getLastVisibleMessageText() there might be others we hide for reasons besides "deleting" in the future.

marcaaron · 2022-02-16T22:24:42Z

src/libs/actions/Report.js

+    // If this is the most recent message and it wasn't deleted, update the lastMessageText in the report object as well
+    const lastMessageText = ReportUtils.formatReportLastMessageText(message.text);
+    if (lastMessageText && sequenceNumber === reportMaxSequenceNumbers[reportID]) {
+        Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {lastMessageText});


I'm confused about why we are doing this step here. It looks like we would:

call this once

Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {lastMessageText})

the promise above resolves and then we set the lastMessageText again on line 559

Is that intentional?

My memory is failing me and now I'm confused too 😅 I'll do some more testing and try to figure out why I thought this was necessary.

Ah, I added the call inside then to update deleted messages and kept it outside to update edited messages. I think we can simplify this and just call it when the promise resolves.

luacmartins · 2022-02-24T17:31:28Z

Updated!

marcaaron

Nice, thanks for the changes!

marcaaron · 2022-02-25T00:15:26Z

Approving, but should this be on HOLD until the Auth change?

luacmartins · 2022-03-01T15:35:13Z

Hold removed! Self-merging with approval!

OSBotify · 2022-03-01T15:46:11Z

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

OSBotify · 2022-03-01T19:53:32Z

🚀 Deployed to staging by @luacmartins in version: 1.1.41-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	failure ❌

OSBotify · 2022-03-02T10:46:49Z

🚀 Deployed to staging by @luacmartins in version: 1.1.41-0 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	failure ❌

OSBotify · 2022-03-11T00:03:23Z

🚀 Deployed to production by @francoisl in version: 1.1.41-6 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Stutikuls · 2022-03-11T02:32:01Z

Issue 1 -

Title- [High] Keyboard : Controls(Edit, Delete, etc.) are not appearing using keyboard.
Actual -Controls(Edit, Delete, etc.) are not appearing using keyboard, displayed on hover.
Expected - Controls(Edit, Delete, etc.) should appear using keyboard.
WCAG failure - 1.4.13
Reproducible in staging?: - Yes
Version Number: - v1.1.42-2
Platforms - Web(Chrome + Jaws), Desktop (MAC), Mobile-web (iOS-Safari + Voiceover), iOS, Android

7711_.Controls.Edit.Delete.etc.are.not.appearing.using.keyboard.mp4

Issue 2 -

Title- [High] Keyboard : Role is not defined for 'Delete' & 'Cancel' control.
Actual -Role is not defined for 'Delete' & 'Cancel' control. Screen reader only reads name of the controls.
Expected - Role - 'Button' should define for 'Delete' & 'Cancel' control.
WCAG failure - 1.4.2
Reproducible in staging?: - Yes
Version Number: - v1.1.42-2
Platforms - Web(Chrome + Jaws), Desktop (MAC), Mobile-web (iOS-Safari + Voiceover), iOS

7711_Role.is.not.defined.for.Delete.Cancel.control.mp4

Issue 3 -

Title- [High] Keyboard : Role is not defined for 'Delete comment' control.
Actual -Role is not defined for 'Delete comment' control. Screen reader only reads name of the control.
Expected - Role - 'Button' should define for 'Delete comment' control.
WCAG failure - 1.4.2
Reproducible in staging?: - Yes
Version Number: - v1.1.42-2
Platforms - Web(Chrome + Jaws), Desktop (MAC), Mobile-web (iOS-Safari + Voiceover), iOS

7711_Role.is.not.defined.for.Delete.Cancel.control.mp4

luacmartins added 8 commits February 11, 2022 11:50

add early return for deleted comment

607966c

add early return for deleted comment and cleanup

8d824a0

create previousMessageText

80cc000

move getLastMessageText

315810d

filter out deleted messages

0aee84a

add conditional to update lastMessage only when its available

cd09165

remove usages of Comment Deleted

02ce537

refactor getLastMessageText

97c70a4

luacmartins self-assigned this Feb 11, 2022

luacmartins added 2 commits February 11, 2022 16:04

update comments

3ab48ae

rm unused import

3b7c8fc

luacmartins marked this pull request as ready for review February 15, 2022 15:31

luacmartins requested a review from a team as a code owner February 15, 2022 15:31

MelvinBot requested review from MonilBhavsar and removed request for a team February 15, 2022 15:31

MonilBhavsar previously approved these changes Feb 16, 2022

View reviewed changes

MonilBhavsar requested a review from marcaaron February 16, 2022 13:29

marcaaron requested changes Feb 16, 2022

View reviewed changes

rushatgabhane mentioned this pull request Feb 21, 2022

LHN - [Comment Deleted] text translated in Spanish after refresh the page #7764

Closed

luacmartins added 2 commits February 24, 2022 11:34

Merge branch 'main' into cmartins-rmDeletedComment

0397482

rename getLasMessageText, rm duped Onyx call, update comments

60d218f

luacmartins dismissed MonilBhavsar’s stale review via 60d218f February 24, 2022 17:29

luacmartins requested a review from marcaaron February 24, 2022 17:31

marcaaron approved these changes Feb 25, 2022

View reviewed changes

luacmartins changed the title ~~Remove "Comment Deleted" message~~ [HOLD Auth #6430] Remove "Comment Deleted" message Feb 25, 2022

luacmartins changed the title ~~[HOLD Auth #6430] Remove "Comment Deleted" message~~ Remove "Comment Deleted" message Mar 1, 2022

luacmartins merged commit 572caa9 into main Mar 1, 2022

luacmartins deleted the cmartins-rmDeletedComment branch March 1, 2022 15:35

OSBotify mentioned this pull request Mar 1, 2022

Deploy Checklist: New Expensify 2022-03-01 #7954

Closed

68 tasks

ahmdshrif mentioned this pull request Mar 5, 2022

"Comment deleted" as last message in LHN #8001

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove "Comment Deleted" message #7711

Remove "Comment Deleted" message #7711

luacmartins commented Feb 11, 2022 •

edited

Loading

MonilBhavsar left a comment

marcaaron left a comment

marcaaron Feb 16, 2022

luacmartins Feb 24, 2022 •

edited

Loading

luacmartins Feb 24, 2022 •

edited

Loading

marcaaron Feb 16, 2022

luacmartins Feb 24, 2022

marcaaron Feb 25, 2022

marcaaron Feb 16, 2022

luacmartins Feb 24, 2022

luacmartins Feb 24, 2022

luacmartins commented Feb 24, 2022

marcaaron left a comment

marcaaron commented Feb 25, 2022

luacmartins commented Mar 1, 2022

OSBotify commented Mar 1, 2022

OSBotify commented Mar 1, 2022

OSBotify commented Mar 2, 2022

OSBotify commented Mar 11, 2022

Stutikuls commented Mar 11, 2022 •

edited

Loading

Remove "Comment Deleted" message #7711

Remove "Comment Deleted" message #7711

Conversation

luacmartins commented Feb 11, 2022 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

MonilBhavsar left a comment

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron Feb 16, 2022

Choose a reason for hiding this comment

luacmartins Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

luacmartins Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

marcaaron Feb 16, 2022

Choose a reason for hiding this comment

luacmartins Feb 24, 2022

Choose a reason for hiding this comment

marcaaron Feb 25, 2022

Choose a reason for hiding this comment

marcaaron Feb 16, 2022

Choose a reason for hiding this comment

luacmartins Feb 24, 2022

Choose a reason for hiding this comment

luacmartins Feb 24, 2022

Choose a reason for hiding this comment

luacmartins commented Feb 24, 2022

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron commented Feb 25, 2022

luacmartins commented Mar 1, 2022

OSBotify commented Mar 1, 2022

OSBotify commented Mar 1, 2022

OSBotify commented Mar 2, 2022

OSBotify commented Mar 11, 2022

Stutikuls commented Mar 11, 2022 • edited Loading

luacmartins commented Feb 11, 2022 •

edited

Loading

luacmartins Feb 24, 2022 •

edited

Loading

luacmartins Feb 24, 2022 •

edited

Loading

Stutikuls commented Mar 11, 2022 •

edited

Loading