App: [HOLD for payment 2021-12-13] [HOLD for payment 2021-12-06] After deleting the last message, editing the next last message doesn't update the message displayed in LHN

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

Send 2 messages (message 1 and message 2) in a chat
Delete the last message (message 2)
Edit the next last message (message 1)

Expected Result:

The LHN should be updated after editing last message regardless of your previous actions

Actual Result:

The LHN is not updated

Workaround:

Sending a new message and editing it updates LHN properly

Platform:

Where is this issue occurring?

Web ✔️ iOS ✔️ Android ✔️ Desktop App ✔️ Mobile Web ✔️

Proposal:

The issue is caused by unreliable values in reportMaxSequenceNumbers in src/libs/actions/Report.js. This array is not updated when you delete a message.

Here is how I would rewrite updateReportActionMessage to update the reportMaxSequenceNumbers.

function updateReportActionMessage(reportID, sequenceNumber, message) {
   const actionToMerge = {};
   actionToMerge[sequenceNumber] = {message: [message]};
   Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, actionToMerge);

   // If this is the most recent message
   if (sequenceNumber === reportMaxSequenceNumbers[reportID]) {
       // If message is deleted we need to update maxSequenceNumber
       if (message.html === '') {
           const connectionID = Onyx.connect({
               key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
               callback: (reportActionList) => {
                   // To make sure the callback is only called once
                   Onyx.disconnect(connectionID);

                   // Find last non-empty message
                   const lastReportAction = !_.isEmpty(reportActionList)
                       ? _.find(Object.values(reportActionList).reverse(),
                           reportAction => _.last(reportAction.message).html !== ''
                           && reportAction.created !== 'CREATED')
                       : null;

                   Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
                       maxSequenceNumber: lastReportAction.sequenceNumber,
                   });
               },
           });
       } else {
           // Update the lastMessageText in the report object
           Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
               lastMessageText: message.html,
           });
       }
   }
}

I would also need to check how other functions across the app interact with maxSequenceNumber to make sure reducing it doesn’t break anything.

Version Number: 1.0.68-4 Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation Expensify/Expensify Issue URL:

View all open jobs on Upwork

About this issue

Original URL
State: closed
Created 3 years ago
Comments: 57 (50 by maintainers)

Commits related to this issue

#3743 #3293 add LHN label for deleted messages — committed to dklymenk/Expensify.cash by dklymenk 3 years ago
#3743 #3293 make LHN deleted message label translatable — committed to dklymenk/Expensify.cash by dklymenk 3 years ago

Most upvoted comments

Awesome, thanks for confirming my thoughts!

If I was to redesign this flow, I would remove the last message data from report collection and extract last non-empty message from report action collection directly in component that needs it.

This is what was running through my head as well. I think we should definitely consider this. Could you work on providing a proposal to do this? I think only the LHN is impacted by this, but we need to be careful that nothing else is.

The vague proposal above will also make it so we don’t need to care about maxSequenceNumber, at least in the context of LHN. There will still be some code that uses it, but that issue should IMO be addressed separately, since it won’t interfere with LHN in any way.

Cool. I think if it removes the need to worry about maxSequenceNumber in this context, then I agree it should be addressed separately.

tgolen on Jul 9, 2021

Paid @dklymenk in Upwork. Thanks for the help!

$250 for job
$250 for reporting
$250 for Company Offsite Hold bonue.

mallenexpensify on Dec 18, 2021

I took the issue off hold in the title, we’re still on n6-hold though so it might take a little bit of time for the PR to get merged, thanks for the patience @dklymenk (and… we’ll add a bonus for the hold… once the PR is deployed to production and doesn’t have regressions for 7 days)

mallenexpensify on Oct 1, 2021

Based off #3737 being closed (PR for it), I think we can take this off hold now, right @iwiznia

mallenexpensify on Sep 28, 2021

Hired @dklymenk in Upwork, assigned to this issue

mallenexpensify on Jul 12, 2021

I’d like to put a little bit of a hold on this. It seems to me that this is complex code, and both the code being proposed here and in https://github.com/Expensify/Expensify.cash/pull/3526 are only making it more and more complex.

I think it would be worth it to take a step back, look at some of the root causes of these problems, and see if there is a better solution.

Remember, this stuff was originally developed without the understanding that messages would be deleted. That’s OK, we avoid pre-optimizing solutions and over-engineering things. This means that when new functionality needs to be added (like deleting report actions), we refactor the original solutions to allow the new functionality to exist.

It seems like some of the root problems that lead up to this are:

There is data being duplicated in Onyx. Specifically, the last message details exist in both the report collection and the report action collection. This leads to complex code that must now keep both of those in sync
When the last message is deleted, the report’s maxSequenceNumber isn’t updated

Before talking any more about solutions, can we agree that these are the root cause of the problem? Am I missing anything?

tgolen on Jul 9, 2021

@puneetlath assigned to you via the External label because I’m out next week. Leaving myself on assignment too, can pick up when back if needed

mallenexpensify on Jul 2, 2021