Open m-natarajan opened 1 year ago
Incorrect message displayed in LHN after deleting IOU thread messages: showing "This is the beginning of your chat" Instead of "No activity yet"
In the current, we only display "No activity yet" if this report is isChatRoom || result.isPolicyExpenseChat || result.isThread || result.isTaskReport. we don't have the condition to check if it is request money report
In here we should add || result.isMoneyRequestReport
In case we want to hide request money report, we can add more condition here https://github.com/Expensify/App/blob/6e17b64be0e571773bed42446346045c659640d6/src/libs/ReportUtils.js#L2781-L2784 like this
if ((isChatThread(report) || isMoneyRequestReport(report))&& isEmptyChat) {
return false;
}
In this way we still need to update correct lastmessagetext of IOU chat by doing
In here we should add || result.isMoneyRequestReport
Because from the main chat, user still can go to IOU chat (although the IOU chat is hidden from LHN) as the hovering message in the below image
So we can consider adding the condition to hide the IOU report action with 0$ and 0 reply
Triggered auto assignment to @joekaufmanexpensify (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Incorrect message displayed in LHN after deleting IOU thread messages: showing "This is the beginning of your chat" Instead of "No activity yet"
This is happening because the money request report is not included in the conditions that result in No activity yet when there is no last message in the thread. https://github.com/Expensify/App/blob/56f0b0f2af316cbda0667373741afa8caa941e8b/src/libs/SidebarUtils.js#L299-L309
We need to add isMoneyRequestReport to include money reports and display No activity yet for them.
if ((... || result.isMoneyRequestReport || result.isTaskReport) && !result.isArchivedRoom) {
https://github.com/Expensify/App/assets/74202751/1ebb0072-c9d9-4de6-a7da-b02849e780e0
Platforms
in OP are ✅)I can reproduce this, but I'm not sure if the bug here is really that we don't show "No activity". To me, I'd think we'd want to remove the IOU thread entirely.
For example, if you send an IOU, and then send a message in its thread, and then delete the message, and IOU (in that order), we remove the IOU thread entirely. However, if you do the same, and the delete the IOU first, and then the message in it's thread, we leave the $0 IOU thread in the chat. That feels inconsistent to me.
Curious if @mountiny or @luacmartins agree though?
Not overdue
@joekaufmanexpensify Could you check this thread https://expensify.slack.com/archives/C049HHMV9SM/p1692916405623119?thread_ts=1691789691.040869&cid=C049HHMV9SM ? I still can reproduce
cc @ayazhussain79
Correct, I would imagine same as if user does not comment in there and all requests are deleted the thread would disappear. But also this is kind of consistency with a normal thread, when there are no requests it can be seen as any other thread and that does not disappear when last comment is deleted.
But also this is kind of consistency with a normal thread, when there are no requests it can be seen as any other thread and that does not disappear when last comment is deleted.
@mountiny On my end, we are deleting a regular thread if the parent message is deleted, and you delete all of the remaining child messages. Here's how it looks. If this is expected behavior, I'd think we'd handle this the same way here. Here's how it looks:
https://github.com/Expensify/App/assets/54866469/f7191dd1-9a8b-4715-a600-4f457b4ffa89
@mountiny @joekaufmanexpensify Let's see this comment https://github.com/Expensify/App/blob/93bde2010f145bbb97c54c06922f0ea52453958c/src/libs/ReportUtils.js#L2781
In addition, in the task report we also display "No activity yet" if there are no action
https://github.com/Expensify/App/assets/141406735/5b71a0da-a408-4f7f-b2ce-2ae958d98239
So I think we also should display "No activity yet" for request money report with no action. Anyway, if we want to hide request money report with no action we also do that by adding filter in here https://github.com/Expensify/App/assets/141406735/5b71a0da-a408-4f7f-b2ce-2ae958d98239
@joekaufmanexpensify What you're saying makes 100% sense, it does seem like a strange behaviour that we remove all empty threads except for this case, which based on how other threads get removed, it should be removed in this bug as well.
updated proposal with another solution: hide the IOU chat from LHN
@mountiny @joekaufmanexpensify If we hide the IOU chat in LHN, the user still can access to this IOU chat by clicking the IOU message in main chat
https://github.com/Expensify/App/assets/141406735/58fd645c-e47e-4f2d-82ea-620b55eaa80d
@mountiny @joekaufmanexpensify Isn’t it better to remove the ReportPreview and the LHN entry like how it is shown below? Although there is some similarity with normal thread behaviour, I don’t see any real reason or value for a zero money request entry that has no comments in thread and the associated LHN entry to be shown.
https://github.com/Expensify/App/assets/3004659/7983c98b-8a57-4c92-a311-b94d2d43c54d
@rojiphil I agree. That is what I am proposing.
In any event, there is definitely a bug here, we just need to align on correct expected behavior going forward
Job added to Upwork: https://www.upwork.com/jobs/~014ccb11728107f0a1
Current assignee @joekaufmanexpensify is eligible for the External assigner, not assigning anyone new.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @jjcoffee (External
)
Reviewing tomorrow!
Sounds good!
The original problem/solution statement was that when the last message in an IOU Report was deleted, the LHN message for IOUReport should display "No activity yet". However, the expected behavior does not seem correct. As mentioned here, here, here, and here, we are trying to solve this problem such that it behaves as if there were no money requests and comments for the chat report.
Possible Expected Behaviour:
1) Transaction View:
a) When we delete Money Request/Comments in Transaction View and there is at least one Comment left, let the MoneyRequest/Comment be deleted and Transaction View will remain shown.
b) When we delete the Money Request and there are no other comments in Transaction View, we know that the Transaction View is no longer needed. Also, the IOU Report is not needed if this was the only Money Request and there are no other comments in IOUReport. This means that we can navigate back to Chat Report. This case is handled with the help of shouldDeleteIOUReport
variable within deleteMoneyRequest
.
c) When we delete the Money Request and there is at least one comment left in Transaction View, we cannot remove the Transaction View, the focus remains with Transaction View. The IOU Report will, now, show [Deleted request]
with number of replies matching the number of comments.
d) When we delete the last Comment in Transaction View and the Money Request was already deleted before, we can expect it to behave like 1b.
2) IOU Report View:
a) When we delete Money Request/Comments in IOU Report and there is at least one Money Request/Comment/[Deleted request] left, let the entry be deleted and the IOU Report will remain shown.
b) When we delete the last Money Request and there are no other comments/[Deleted request] in IOU Report View, we know that the IOU Report view is no longer needed. This means that we can navigate back to Chat Report. This case is handled with the help of shouldDeleteIOUReport
variable within deleteMoneyRequest
.
3) Chat Report View: a) When there is at least one MoneyRequest/Comment/[Deleted request] in IOU Report, the Report Preview in Chat Report must remain shown. b) When where are no MoneyRequest/Comment/[Deleted request] in IOU Report, the Report Preview in Chat Report must be hidden.
The problem is due to not clearing the IOU Report and associated Report Preview in Chat Report as we would have done when the last deleted message in the IOU report was a money request as shown here and here. Instead, we call deleteReportComment
here.
This proposal requires changes both in the FE and BE. The suggested changes are as follows:
1) For all non Money Request Actions (i.e. of actionName IOU
), let us find out if we are dealing with the last visible action in an IOU Report. If so, let us call deleteMoneyRequest
for this last comment in the IOU Report and return from there. Since shouldDeleteIOUReport
would return true due to this last visible action, it will handle all that is needed to clear the IOU Report and associated Report Preview. Finally, if the newly added condition does not satisfy, we continue as before by calling deleteReportComment
.
The changes as shown below can be added at this location just before calling deleteReportComment
const report = ReportUtils.getReport(this.state.reportID);
if(ReportUtils.isIOUReport(report))
{
const updatedReportAction = {
[this.state.reportAction.reportActionID]: {
message: [
{
type: 'COMMENT',
html: '',
text: '',
isEdited: true,
},
],
},
};
const lastVisibleAction = ReportActionsUtils.getLastVisibleAction(this.state.reportID, updatedReportAction);
const iouReportLastMessageText = ReportActionsUtils.getLastVisibleMessage(this.state.reportID, updatedReportAction).lastMessageText;
if(iouReportLastMessageText.length === 0 && !ReportActionsUtils.isDeletedParentAction(lastVisibleAction))
{
this.state.reportAction.originalMessage.IOUReportID = this.state.reportID;
IOU.deleteMoneyRequest(null, this.state.reportAction);
return;
}
}
2) Currently, the BE will fail for the DeleteMoneyRequest
API as there are no valid transactions for this IOU Report. We need to make necessary changes in the BE such that this last visible comment in the IOU Report can be handled to clear up the IOU Report and associated ReportPreview.
We can implement a helper function hasVisibleActionsInMoneyRequestReport
which can return false if the lastVisibleMessage
is empty and if the IOU Report does not have any deleted parent action item. Since the lastVisibleMessage
will be empty string for IOU report actions with deleted parent action, we also need to additionally check if IOU Report has any deleted parent action in it. This can be done as follows:
function hasVisibleActionsInMoneyRequestReport(reportOrID) {
const iouReport = _.isObject(reportOrID) ? reportOrID : allReports[`${ONYXKEYS.COLLECTION.REPORT}${reportOrID}`];
if(!iouReport)
return true;
const iouReportLastMessageText = ReportActionsUtils.getLastVisibleMessage(iouReport.reportID).lastMessageText;
const hasDeletedParent = ReportActionsUtils.hasDeletedParentActionItem(iouReport.reportID);
if(iouReportLastMessageText.length === 0 && !hasDeletedParent) {
return true;
}
return false;
}
function hasDeletedParentActionItem(reportID) {
return _.some(allReportActions[reportID], (reportAction) => reportAction && isDeletedParentAction(reportAction));
}
Now, we can use this helper function in:
a) shouldReportBeInOptionList
here as shown below so that IOU Report can be hidden in LHN if there are no visible actions within it.
if(isMoneyRequestReport(report) && hasVisibleActionsInMoneyRequestReport(report)){
return false;
}
b) shouldReportActionBeVisible
here as shown below so that Report Preview action can be hidden in Report Screen when IOU Report has no money request and no comments and also do not have any [Deleted request] entries.
if(isReportPreviewAction(reportAction)){
if(getNumberOfMoneyRequests(reportAction) === 0 && lodashGet(reportAction, 'childVisibleActionCount', 0) === 0 && ReportUtils.isThreadParent(reportAction) && ReportUtils.hasVisibleActionsInMoneyRequestReport(reportAction.childReportID))
{
return false;
}
}
c) Current BE will send the report preview message in lastMessageText
even when IOU Report does not have any money requests and comments in it. The BE would need to update the lastMessageText
such that it points to the previous visible message.
Sorry higher priority PR got in the way, will review Monday!
Sounds good! I wonder if we should hold on this issue because of this discussion. Asking there!
@DylanDylann Do I understand correctly that your proposal would result in the IOU being hidden in the LHN, but still shown within the main chat? It sounds like we're leaning towards hiding off the IOU from everywhere as it's essentially a useless, empty, 0-value report (cc @joekaufmanexpensify in case you disagree!).
There actually appears to be a related bug here where the reportActionPreview initially hides when you delete an IOU request, but after refresh re-appears since the reportAction never deletes. Of course when you try and open it there's an error:
https://github.com/Expensify/App/assets/27287420/39d0b1b9-898a-454b-ab9b-39e71bc29826
There actually appears to be a related bug here where the reportActionPreview initially hides when you delete an IOU request, but after refresh re-appears since the reportAction never deletes.
That is issue #26627
@jjcoffee in my proposal, I suggest 2 solutions:
@DylanDylann Yes, my question is regarding your alternative solution; will it hide the report preview in the chat too?
[Updated]
@jjcoffee IOU chat report will be hidden in LHN when clicking to another chat. But it doesn't hide the report preview in the main chat
@jjcoffee But It makes sense to me. IMO, we also should hide the report preview in the main chat. Will update ASAP. Thanks
An interesting thing here is that when deleting all thread messages (i.e. the case that we're trying to be consistent with), the deleted parent message/report is actually not completed removed by the BE (if you copy the link before deleting the last message, you can still access it (it just shows [Deleted message])).
This makes me wonder why we're handling things differently for IOU reports. Ideally we'd have a consistent approach so that calling deleteReportComment
removes the parent report once all messages are deleted (and the BE response should then just be consistent regardless of report type). Having to mix deleteReportComment
with custom functions for different types of reports (as in @rojiphil's proposal) seems like a workaround we shouldn't have to add!
@jjcoffee @joekaufmanexpensify
For more information, Let's see how the App works in this case (deleting IOU message normally)
https://github.com/Expensify/App/assets/141406735/5f0bc1f1-2d3a-4ded-89a3-615dc789ce94
Could we confirm the expected before moving forward? IMO, the expected should be: After all messages in IOU report are deleted, we will hide IOU report in LHN and hide the IOU message in the main chat. If user come back to IOU report by link, we will show Not Found Page and the IOU message should not re-appear in the main chat
Still pending further discussion
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
Ideally we'd have a consistent approach so that calling
deleteReportComment
removes the parent report once all messages are deletedStill pending further discussion
@jjcoffee On giving further thought to this, I also think that it should behave normally like how it would be using deleteReportComment
. So, are we waiting for any conclusion to internal discussion or are we waiting for proposals? I can propose one using this approach.
Could we confirm the expected before moving forward? IMO, the expected should be: After all messages in IOU report are deleted, we will hide IOU report in LHN and hide the IOU message in the main chat. If user come back to IOU report by link, we will show Not Found Page and the IOU message should not re-appear in the main chat
@DylanDylann @jjcoffee this makes sense to me!
Updated. @jjcoffee Kindly review changes as per the updated understanding of expected behavior.
Just thought to put together an expected behaviour so that we are all on the same page:
1) Transaction View:
a) When we delete Money Request/Comments in Transaction View and there is at least one Comment left, let the MoneyRequest/Comment be deleted and Transaction View will remain shown.
b) When we delete the Money Request and there are no other comments in Transaction View, we know that the Transaction View is no longer needed. Also, the IOU Report is not needed if this was the only Money Request and there are no other comments in IOUReport. This means that we can navigate back to Chat Report. This case is handled with the help of shouldDeleteIOUReport
variable within deleteMoneyRequest
. Here, additionally, we can hide IOU Report in LHN.
c) When we delete the Money Request and there is at least one comment left in Transaction View, we cannot remove the Transaction View, the focus remains with Transaction View. The IOU Report will, now, show [Deleted request]
with number of replies matching the number of comments.
d) When we delete the last Comment in Transaction View and the Money Request was already deleted before, we can expect it to behave like when a last message in the normal thread is deleted. i.e. Keep the focus on the Transaction View and keep showing the LHN entry for Transaction View. When the user navigates away from the Transaction View, the LHN entry disappers.
2) IOU Report View:
a) When we delete Money Request/Comments in IOU Report and there is at least one Money Request/Comment/[Deleted request] left, let the entry be deleted and the IOU Report will remain shown.
b) When we delete the last Money Request and there are no other comments/[Deleted request] in IOU Report View, we know that the IOU Report view is no longer needed. This means that we can navigate back to Chat Report. This case is handled with the help of shouldDeleteIOUReport
variable within deleteMoneyRequest
. Here, additionally, we can hide IOU Report in LHN.
3) Chat Report View: a) When there is at least one MoneyRequest/Comment/[Deleted request] in IOU Report, the Report Preview in Chat Report must remain shown. b) When where are no MoneyRequest/Comment/[Deleted request] in IOU Report, the Report Preview in Chat Report must be hidden.
@joekaufmanexpensify @jjcoffee Is this what we are agreeing to? Or are there any updates required?
@jjcoffee In this comment of yours, wondering if you were referring to handling cases 2-b and 1-b for IOU Reports in the same way as deleteReportComment
?
@jjcoffee, @joekaufmanexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!
@jjcoffee, @joekaufmanexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
I'm thinking that all sounds good to me. @jjcoffee any concerns?
Sorry ran out of time for today, reviewing tomorrow!
@rojiphil I think we're expecting 1b and 1d to behave the same here, so whether or not you delete the Money Request first (1d) or last (1b), the IOU report is deleted. My comment was more referring to a "nice to have" that deleteReportComment
would behave consistently for all report types. There's probably a reason why it doesn't though, so I think we can move ahead without that for now.
So I think @rojiphil's proposal (alternative solution) is the way forward here. It had the correct RCA (@rojiphil can you add that back in, please?) and the solution fits the expected behaviour that removing all comments + the money request itself on a transaction thread results in the IOU report getting deleted, regardless of the order in which you delete. We do need BE changes too, but I think it makes sense that deleteMoneyRequest
shouldn't fail out when there are no valid transactions.
:ribbon::eyes::ribbon: C+ reviewed
Triggered auto assignment to @grgia, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
Updated to swap the alternate solution to the primary solution.
@jjcoffee Since the BE changes have no impact on FE as FE currently does not call the API in the scenario of (1d), wouldn’t it be better to start work in FE after BE changes so that we can make the necessary changes in FE, test and close this out?
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
Pending internal engineering sign off
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:
Expected Result:
Upon deleting all messages from the IOU thread (and the IOU itself), we should hide the IOU thread entirely from the LHN.
Actual Result:
Incorrect message displayed in LHN after deleting IOU thread messages: showing "This is the beginning of your chat" Instead of "No activity yet"
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.57-3 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation
https://github.com/Expensify/App/assets/38435837/bc69afa7-d723-48bb-8ec3-585aec3a48b7
https://github.com/Expensify/App/assets/38435837/0f889b8e-7748-4493-98e6-af9e12fe1e37
Expensify/Expensify Issue URL: Issue reported by: @ayazhussain79 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1691789691040869
View all open jobs on GitHub
Upwork Automation - Do Not Edit