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

#3293 update last message details on message deletion #3526

Closed
wants to merge 7 commits into from
+36 −1
Closed

#3293 update last message details on message deletion #3526

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d7250c5
#3293 update last message details on message deletion
dklymenk Jun 9, 2021
71867f6
#3293 add comment for last message html manipulations
dklymenk Jun 15, 2021
0c9d2fa
#3293 remove comma after CREATED
dklymenk Jun 17, 2021
f480ca5
#3293 use destructuring to avoid having reportActionItem and reportAc…
dklymenk Jun 17, 2021
3c54c02
#3293 use Str.stripHTML
dklymenk Jun 18, 2021
e0d282d
#3293 replace _.once with Onyx.disconnect
dklymenk Jun 20, 2021
6c57d8d
Merge branch 'Expensify:main' into 3293-update-last-message-details-o…
dklymenk Jun 24, 2021
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
38 changes: 37 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 @@ -147,7 +147,9 @@ function getChatReportName(sharedReportList) {
*/
function getSimplifiedReportObject(report) {
const reportActionList = lodashGet(report, ['reportActionList'], []);
const lastReportAction = !_.isEmpty(reportActionList) ? _.last(reportActionList) : null;
const lastReportAction = !_.isEmpty(reportActionList)
? _.find([...reportActionList].reverse(), reportAction => reportAction.message.html !== '')
NikkiWines marked this conversation as resolved.
Show resolved Hide resolved
: null;
const createTimestamp = lastReportAction ? lastReportAction.created : 0;
const lastMessageTimestamp = moment.utc(createTimestamp).unix();
const isLastMessageAttachment = /<img([^>]+)\/>/gi.test(lodashGet(lastReportAction, ['message', 'html'], ''));
Expand Down Expand Up @@ -1084,6 +1086,40 @@ function deleteReportComment(reportID, reportAction) {
};

Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, reportActionsToMerge);
} else {
// Update last message information in the report object
Onyx.connect({
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
callback: _.once((reportActionList) => {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry but the use of _.once is very bad here. You are creating n number of subscriptions without releasing them. Where n === no of deleted comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry but _.once is required there. If you remove _.once and put console.log inside that callback, you'll see that the callback is called 2-3 times per message delete call. Can you please explain what exactly is the big downside to this approach? Is it using extra memory?

Copy link
Member

Choose a reason for hiding this comment

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

So you can call Onyx.disconnect here after the operation is done. Otherwise, this subscription will remain, and On the next delete operation, both old ones and the new ones will be called. And I believe that this _.once will not be required then. But it is possible that while you are listening to this key, other factors update the Store, and thus this callback fire multiple times.
So yeah just Onyx.disconnect will be fine. _.once has not drawbacks just I was thinking that it won't be required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I'll check that out and update PR accordingly if that helps.

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 actually works flawlessly. Thanks.

PR is updated.

// Find last non-empty message
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to pitch in very late but this solution does not seem to follow the optimistic approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The decision to ditch optimistic approach is validated with Expensify engineers. The tl;dr is since the edit comment doesn't update last message optimistically, there is no reason the delete comment should.

Copy link
Member

@parasharrajat parasharrajat Jun 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

https://github.com/Expensify/Expensify.cash/blob/3c54c020bfab47a66f75d3dd8a8cc158b2337e42/src/libs/actions/Report.js#L1061-L1072
This is happening optimistically. In simpler words, we delete the comment from the Onyx even though we have not called the API yet. and then based on API response if there was a failure we revert it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sir, how is that code snippet you linked relevant to what I said?

the edit comment doesn't update last message optimistically

The code snippet you sent doesn't have anything to do with edit comment action or LHN.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, just saw a post from Nikki about this. It's fine then. But can't we do this before the API call. and revert those changes if the response is not 200?

Copy link
Contributor Author

@dklymenk dklymenk Jun 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, we can. That's actually what I was going to do initially, but since Nikki said there is no need to do it optimistically, I happily abandoned that idea. Anyway I didn't want to think about any potential race conditions with both optimistic update and then revert update happening in async manner, deleting multiple comments in a quick succession, etc.

const lastReportAction = !_.isEmpty(reportActionList)
NikkiWines marked this conversation as resolved.
Show resolved Hide resolved
? _.find(
Object.values(reportActionList).reverse(),
reportActionItem => _.last(reportActionItem.message).html !== ''
&& reportActionItem.actionName !== 'CREATED',
)
: null;

// Populate last message details
const lastMessageDetails = {};
if (lastReportAction) {
// Replace line breaks with spaces and remove all HTML from the message
lastMessageDetails.lastMessageText = lastReportAction.isAttachment
NikkiWines marked this conversation as resolved.
Show resolved Hide resolved
? '[Attachment]'
: _.last(lastReportAction.message)
.html.replace(/((<br[^>]*>)+)/gi, ' ')
.replace(/(<([^>]+)>)/gi, '');
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
lastMessageDetails.lastMessageTimestamp = lastReportAction.timestamp;
lastMessageDetails.lastActorEmail = lodashGet(lastReportAction, 'actorEmail', '');
} else {
lastMessageDetails.lastMessageText = '';
lastMessageDetails.lastMessageTimestamp = 0;
lastMessageDetails.lastActorEmail = '';
}

Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, lastMessageDetails);
Copy link
Member

Choose a reason for hiding this comment

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

This statement causes a recursion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please explain how does it cause a recursion? Is there some specific edge case I didn't see. The code runs only once after the response from BE arrives.

Copy link
Member

@parasharrajat parasharrajat Jun 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Oh sorry didn't notice at first sight that this is a different subscription key.

Copy link
Member

Choose a reason for hiding this comment

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

Please release the connection here before this statement. And remove _.once. Could you please try to think of your solution in an optimistic way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am aware this is not the best approach. As I stated in one of the messages above, ideally we move this entire code to updateReportActionMessage function and have the handling for delete action next to edit action. However, until #3524 is resolved this cannot be done for delete action.

}),
});
}
});
}
Expand Down