[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

[dklymenk]

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:

1. Send 2 messages (message 1 and message 2) in a chat
2. Delete the last message (message 2)
3. 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

[MelvinBot]

Triggered auto assignment to @strepanier03 (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[NikkiWines]

A note that this issue is related to the changes made in this PR: #3526

[dklymenk]

Is there any way to push this one forward? It's blocking PR #3526 and all the code is already there, I'm just waiting for a green light to go ahead with the implementation.

[parasharrajat]

So this is related to changes in #3526 which NikkiWines mentioned above. So I think you are good to go here to submit the PR for this. As this part of old PR.

[parasharrajat]

But if you are waiting for your solution to be reviewed then waiting is good.

[NikkiWines]

Hmm, it looks like @strepanier03 is out-of-office currently. Going to add the External label so this issue can be triaged appropriately

[MelvinBot]

Triggered auto assignment to @mallenexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[mallenexpensify]

Everything looks good to me, I'm going to assign an engineer to review.

[MelvinBot]

Triggered auto assignment to @iwiznia (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[mallenexpensify]

Job posted https://www.upwork.com/jobs/~01a6f9be9af612f0cd
@iwiznia please review the proposal then assign to to @dklymenk

[MelvinBot]

Triggered auto assignment to @puneetlath (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[mallenexpensify]

@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

[iwiznia]

That proposal seems it would work, but I am a bit concerned about the one time connect call to get all actions of the report of the action being updated.
Do you think we could instead put the logic to update the maxSequenceNumber here instead where we should already have all the data available without having to do that one time connect call @dklymenk?

[NikkiWines]

Adding the Help Wanted label, as it looks like we're still accepting new proposals on this one (please remove the label if it's not applicable)

[mallenexpensify]

So close #3737 (comment)
but... we're on hold for N6 for a week-ish so we likely have a lil while longer to wait.

[mallenexpensify]

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

[dklymenk]

Thanks for bringing it up. Apparently that fix is already deployed too.

I did quick check and my PR #4133 still works. I'm marking it as ready for review now.

[dklymenk]

Yep, only thing holding the PR for this issue from being merged is the HOLD label on the issue itself. It can 100% be removed now as the fix for #3737 has been merged and deployed and the issue itself is closed.

[mallenexpensify]

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)

[kadiealexander]

Please refer to this post for updated information on the n6 hold, we've raised the bonus to $250 for all issues where a PR is created before the N6 hold is lifted.

[mallenexpensify]

n6-hold is lifted, label removed!
@dklymenk can you provide an update on when you'll have a PR ready for review? Thanks

[dklymenk]

It's here

#4133

[dklymenk]

Ah, there are conflicts again, sorry.

I will resolve those and update the PR within a few hours.

[dklymenk]

@mallenexpensify Conflict is fixed and PR is ready to be merged again as far as I am concerned.

[mallenexpensify]

Sorry for the delay @dklymenk , I commented on the PR and removed [HOLD] from the title so we should be good to go.
#4133 (comment)

[botify]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.16-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• remove deleted comment message on new chats #6458
• remove deleted comment message on new chats #6458

If no regressions arise, payment will be issued on 2021-12-06. 🎊

[botify]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.17-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• remove deleted comment message on new chats #6458
• remove deleted comment message on new chats #6458

If no regressions arise, payment will be issued on 2021-12-13. 🎊

[mallenexpensify]

Paid @dklymenk in Upwork. Thanks for the help!

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