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.

Sign up for GitHub

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

Jump to bottom

Fix bugs surrounding report name when deleting first thread comment #23514

Merged
merged 4 commits into from
Jul 24, 2023
Merged

Fix bugs surrounding report name when deleting first thread comment #23514

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
93e5812
Fix bugs surrounding report name when deleting first thread comment
marcaaron Jul 24, 2023
f59e58e
Use isThreadFirstChat
marcaaron Jul 24, 2023
f841af3
Remove check for reportAction.reportActionID
marcaaron Jul 24, 2023
e99d99a
Improve comments and naming
marcaaron Jul 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/libs/actions/Report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -897,7 +897,7 @@ function deleteReportComment(reportID, reportAction) {
lastMessageText: '',
lastVisibleActionCreated: '',
};
if (reportAction.reportActionID && reportAction.childVisibleActionCount > 0) {
if (reportAction.reportActionID && reportAction.childVisibleActionCount === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to check reportAction.reportActionID here?

    const optimisticReportActions = {
        [reportActionID]: {

Doesn't this imply it's truthy? If it's still needed, can it be replaced with if (reportActionID && ...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. If it's not defined then something is wrong because we are about to save an object with a key of undefined. I think this was probably just overly cautious programming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there is a reason it should have been documented better 🤷‍♂️

optimisticReport = {
lastMessageTranslationKey: '',
lastMessageText: '',
Expand Down Expand Up @@ -963,6 +963,16 @@ function deleteReportComment(reportID, reportAction) {
optimisticData.push(optimisticParentReportData);
}

// If the reportID doesn't equal the originalReportID then this is the first comment on a thread report and we need to trigger
// some sort of update to let the LHN know that the parentReportAction is now deleted.
if (reportID !== originalReportID) {
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
optimisticData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {isParentReportActionDeleted: true},
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB but just an idea to make it more in line with the comment

Suggested change
value: {isParentReportActionDeleted: true},
value: {updateReportInLHN: true},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, yes, I prefer this. It solves my concern about people trying to use isParentReportActionDeleted for things when it may be unreliable.

});
}

const parameters = {
reportID: originalReportID,
reportActionID,
Expand Down