[HOLD for payment 2023-11-27] [$1000] Web - Chat - The display name goes down when deleting an attachment in main thread #25415
Comments
izarutskaya
added
DeployBlockerCash
|
Triggered auto assignment to @sophiepintoraetz ( |
Bug0 Triage Checklist (Main S/O)
|
|
👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open
|
|
Triggered auto assignment to @yuwenmemon ( |
|
Going to see if I can replicate this shortly but reviewing the information, I'm not sure its deploy-blocker worthy? |
|
Yes, defintely not deploy blocker worthy. |
yuwenmemon
removed
DeployBlockerCash
|
This can be external as well @sophiepintoraetz - feel free to add at your leisure. |
|
I think regression from this PR as immediate we may not removing the space. |
|
Yes, it is a regression of my PR. @cubuspl42 looks like we missed this case. When we delete the attachment, We have a special case here to immediately show the [Deleted message] before the delete request is completed. App/src/pages/home/report/ReportActionItemFragment.js Lines 108 to 114 in c435734 I'm thinking that we can move that condition above to view code to understand betterdiff --git a/src/pages/home/report/ReportActionItemFragment.js b/src/pages/home/report/ReportActionItemFragment.js
index 009c111840..d19ae5abbe 100644
--- a/src/pages/home/report/ReportActionItemFragment.js
+++ b/src/pages/home/report/ReportActionItemFragment.js
@@ -105,14 +105,6 @@ function ReportActionItemFragment(props) {
}
const {html, text} = props.fragment;
- // Threaded messages display "[Deleted message]" instead of being hidden altogether.
- // While offline we display the previous message with a strikethrough style. Once online we want to
- // immediately display "[Deleted message]" while the delete action is pending.
-
- if ((!props.network.isOffline && props.hasCommentThread && props.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) || props.fragment.isDeletedParentAction) {
- return <RenderHTML html={`<comment>${props.translate('parentReportAction.deletedMessage')}</comment>`} />;
- }
-
// If the only difference between fragment.text and fragment.html is <br /> tags
// we render it as text, not as html.
// This is done to render emojis with line breaks between them as text.
diff --git a/src/pages/home/report/ReportActionItemMessage.js b/src/pages/home/report/ReportActionItemMessage.js
index 7dcb2b51db..6f32cf4496 100644
--- a/src/pages/home/report/ReportActionItemMessage.js
+++ b/src/pages/home/report/ReportActionItemMessage.js
@@ -3,12 +3,15 @@ import {View, Text} from 'react-native';
import PropTypes from 'prop-types';
import _ from 'underscore';
import lodashGet from 'lodash/get';
+import CONST from '../../../CONST';
import styles from '../../../styles/styles';
import ReportActionItemFragment from './ReportActionItemFragment';
import * as ReportUtils from '../../../libs/ReportUtils';
import * as ReportActionsUtils from '../../../libs/ReportActionsUtils';
import reportActionPropTypes from './reportActionPropTypes';
import withLocalize, {withLocalizePropTypes} from '../../../components/withLocalize';
+import useNetwork from '../../../hooks/useNetwork';
+import RenderHTML from '../../../components/RenderHTML';
const propTypes = {
/** The report action */
@@ -33,25 +36,35 @@ const defaultProps = {
};
function ReportActionItemMessage(props) {
+ const {isOffline} = useNetwork();
+ const hasCommentThread = ReportActionsUtils.hasCommentThread(props.action);
const messages = _.compact(props.action.previousMessage || props.action.message);
const isAttachment = ReportUtils.isReportMessageAttachment(_.last(messages));
return (
- <View style={[styles.chatItemMessage, !props.displayAsGroup && isAttachment ? styles.mt2 : {}, ...props.style]}>
+ <View style={[styles.chatItemMessage, ...props.style]}>
{!props.isHidden ? (
- _.map(messages, (fragment, index) => (
- <ReportActionItemFragment
- key={`actionFragment-${props.action.reportActionID}-${index}`}
- fragment={fragment}
- isAttachment={props.action.isAttachment}
- hasCommentThread={ReportActionsUtils.hasCommentThread(props.action)}
- attachmentInfo={props.action.attachmentInfo}
- pendingAction={props.action.pendingAction}
- source={lodashGet(props.action, 'originalMessage.source')}
- accountID={props.action.actorAccountID}
- loading={props.action.isLoading}
- style={props.style}
- />
- ))
+ _.map(messages, (fragment, index) =>
+ // Threaded messages display "[Deleted message]" instead of being hidden altogether.
+ // While offline we display the previous message with a strikethrough style. Once online we want to
+ // immediately display "[Deleted message]" while the delete action is pending.
+ (!isOffline && hasCommentThread && props.action.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) || fragment.isDeletedParentAction ? (
+ <RenderHTML html={`<comment>${props.translate('parentReportAction.deletedMessage')}</comment>`} />
+ ) : (
+. // we need to move the margin style here so the flaggedContent text does not get affected with the style (this exist before too)
+ <View style={!props.displayAsGroup && isAttachment ? styles.mt2 : {}}>
+ <ReportActionItemFragment
+ key={`actionFragment-${props.action.reportActionID}-${index}`}
+ fragment={fragment}
+ isAttachment={props.action.isAttachment}
+ attachmentInfo={props.action.attachmentInfo}
+ pendingAction={props.action.pendingAction}
+ source={lodashGet(props.action, 'originalMessage.source')}
+ accountID={props.action.actorAccountID}
+ loading={props.action.isLoading}
+ style={props.style}
+ />
+ </View>
+ ),
+ )
) : (
<Text style={[styles.textLabelSupporting, styles.lh20]}>{props.translate('moderation.flaggedContent')}</Text>
)}
Let me know what you think! |
ProposalPlease re-state the problem that we are trying to solve in this issue.There is a margin after deleting an attachment. What is the root cause of that problem?This is a regression from #24539 We should move the margin and conditions from the current place from here: we also have to pass <View style={[!props.displayAsGroup && props.isAttachment ? styles.mt2 : {}]}>
<RenderHTML ... />
</View>What alternative solutions did you explore? (Optional) |
|
@bernhardoj Damn! Indeed it looks like we missed something. We'll take the responsibility and fix this. Would you create a PR with your suggestion? We'll move the discussion there. |
|
@cubuspl42 Here is the PR #25484 |
|
@yuwenmemon, @sophiepintoraetz Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
|
I created two small PRs:
After they are merged, we'll choose whether we go with a more local fix or a refactoring. |
|
Yup - I can reproduce - @yuwenmemon, are you comfortable assigning @cubuspl42 given their work introduced the PR? |
melvin-bot
bot
added
Reviewing
|
This issue has not been updated in over 15 days. @cubuspl42, @flodnv, @sophiepintoraetz, @situchan eroding to Monthly issue. P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do! |
|
We're working on the last fixes on the last PR |
|
@flodnv the issue has closed without paying for reporter bonus. I think I am an eligible for the reporter bonus. Please check it again. |
|
Thanks for the call-out, we will also need to confirm it's indeed fixed. |
|
@misgana96 - you've been paid. @cubuspl42 or @situchan, can you confirm whether this is fixed? |
|
This was accidentally closed. This should go through QA. Not hit staging yet. |
melvin-bot
bot
added
Weekly
melvin-bot
bot
changed the title
|
|
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.0-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-11-27. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
|
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
This doesn't formally qualify for payment, as it was a regression from a PR I was reviewing as a C+. I would be grateful if this was treated individually, though. From my perspective, the bug had origins in the pre-existing convoluted code, which I cleaned over from unused leftovers and refactored, so a similar problem is less likely to return. For various reasons, it took more work than I originally predicted, but I'm glad we finally merged it. Thanks to everyone who helped review this! |
The bug itself is very minor and unusual case so I don't propose regression test. |
|
Fair enough, @cubuspl42 - have amended the offer. @situchan - you should have an offer in your inbox for review (I'll pay it out next week as I'm actually OOO now). |
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:
The display name should be in a fixed position
Actual Result:
The display name goes down
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: v1.3.55-1
Reproducible in staging?: Y
Reproducible in production?: N
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
Screenshare.-.2023-08-17.5_07_18.PM.mp4
Recording.1249.mp4
Expensify/Expensify Issue URL:
Issue reported by: @misgana96
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692281066076859
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: