-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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.
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
Delete Comments #2046
Delete Comments #2046
Changes from all commits
4607f2c
fbdba19
4f78dbd
831e55a
b667c12
83811f4
3f5e2e3
e4719dd
3bc3e83
7c44b08
ed28d4c
bc5b393
7a38825
9127eea
e144e3a
0d6827a
91f915d
802b053
fe20205
268daf4
984c8fb
94d72d4
9075af9
99f0e69
2898c4c
2695396
5c3ea4f
efd5baf
cd715d4
20f5166
fa64c3a
8c6e983
fb80f67
0f3d9bb
4db4017
c9802da
12d34a2
a639e77
a6fb1a0
adcd32a
eb0dd41
83f9a9b
f694247
049d402
ab22572
f0cca13
dff673d
2f05393
7a85a88
ccdacf0
1af55ea
b09d259
a57908f
0db0564
b7f975e
2d48f4c
4b25ec0
702b709
5c68540
8678c07
3f166d6
5503c8d
a148398
35c5aa1
8d6f0b5
586d79a
76d93fe
bd3d300
bb863a6
3a09346
23f2cb8
b2f48a9
d156556
9c73a88
e9afb8b
da1136f
f10de2e
d7691d5
4b85cc1
38e2d40
d0e8520
6122f49
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,14 +8,18 @@ import { | |
Clipboard as ClipboardIcon, LinkCopy, Mail, Pencil, Trashcan, Checkmark, | ||
} from '../../../components/Icon/Expensicons'; | ||
import getReportActionContextMenuStyles from '../../../styles/getReportActionContextMenuStyles'; | ||
import {setNewMarkerPosition, updateLastReadActionID, saveReportActionDraft} from '../../../libs/actions/Report'; | ||
import { | ||
setNewMarkerPosition, updateLastReadActionID, saveReportActionDraft, deleteReportComment, | ||
} from '../../../libs/actions/Report'; | ||
import ReportActionContextMenuItem from './ReportActionContextMenuItem'; | ||
import ReportActionPropTypes from './ReportActionPropTypes'; | ||
import Clipboard from '../../../libs/Clipboard'; | ||
import compose from '../../../libs/compose'; | ||
import {isReportMessageAttachment} from '../../../libs/reportUtils'; | ||
import ONYXKEYS from '../../../ONYXKEYS'; | ||
import withLocalize, {withLocalizePropTypes} from '../../../components/withLocalize'; | ||
import ConfirmModal from '../../../components/ConfirmModal'; | ||
import CONST from '../../../CONST'; | ||
|
||
const propTypes = { | ||
/** The ID of the report this report action is attached to. */ | ||
|
@@ -63,8 +67,13 @@ class ReportActionContextMenu extends React.Component { | |
constructor(props) { | ||
super(props); | ||
|
||
this.confirmDeleteAndHideModal = this.confirmDeleteAndHideModal.bind(this); | ||
this.hideDeleteConfirmModal = this.hideDeleteConfirmModal.bind(this); | ||
this.getActionText = this.getActionText.bind(this); | ||
this.canEdit = this.canEdit.bind(this); | ||
|
||
// A list of all the context actions in this menu. | ||
this.CONTEXT_ACTIONS = [ | ||
this.contextActions = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are these being moved inside here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kind of think that this should be set in the state and the view shouldn't display anything until these are set in the state (with an early return in the render method). I suggest something like this:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So the callbacks can access the component's props. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are you happy with this @tgolen? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is better yeah, but still not quite there yet. What I think we settled on in the other thread is:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did do the change, like you requested, but this was reverted again when I merged main in, and it had the changes for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, thanks. I still think it would be good to move these back to be outside the class 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sooooo, heh, sorry to keep commenting here, but are these going to be moved outside the class? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry, I can definetly do it, I'm a bit apreensive on doing that change, as I'm not sure if I'll be able to maintain some of the comments you and Rory have asked me to do. but let me give it a go. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've done some changes to go in this direction, but I'm not entirely sure about them, feel there should be a better way to do this, there are so many calls to |
||
// Copy to clipboard | ||
{ | ||
text: this.props.translate('reportActionContextMenu.copyToClipboard'), | ||
|
@@ -112,9 +121,10 @@ class ReportActionContextMenu extends React.Component { | |
{ | ||
text: this.props.translate('reportActionContextMenu.editComment'), | ||
icon: Pencil, | ||
shouldShow: this.props.reportAction.actorEmail === this.props.session.email | ||
shouldShow: () => ( | ||
this.canEdit() | ||
&& !isReportMessageAttachment(this.getActionText()) | ||
&& this.props.reportAction.reportActionID, | ||
), | ||
onPress: () => { | ||
this.props.hidePopover(); | ||
saveReportActionDraft( | ||
|
@@ -124,18 +134,19 @@ class ReportActionContextMenu extends React.Component { | |
); | ||
}, | ||
}, | ||
|
||
{ | ||
text: this.props.translate('reportActionContextMenu.deleteComment'), | ||
icon: Trashcan, | ||
shouldShow: false, | ||
onPress: () => {}, | ||
shouldShow: this.canEdit, | ||
onPress: () => this.setState({isDeleteCommentConfirmModalVisible: true}), | ||
}, | ||
]; | ||
|
||
this.wrapperStyle = getReportActionContextMenuStyles(this.props.isMini); | ||
|
||
this.getActionText = this.getActionText.bind(this); | ||
this.state = { | ||
isDeleteCommentConfirmModalVisible: false, | ||
}; | ||
} | ||
|
||
/** | ||
|
@@ -148,10 +159,33 @@ class ReportActionContextMenu extends React.Component { | |
return lodashGet(message, 'text', ''); | ||
} | ||
|
||
/** | ||
* Can the current user edit this report action? | ||
* | ||
* @return {Boolean} | ||
*/ | ||
canEdit() { | ||
// Can only edit if it's a ADDCOMMENT, the author is this user and it's not a optimistic response. | ||
// If it's an optimistic response comment it will not have a reportActionID, | ||
// and we should wait until it does before we show the actions | ||
return this.props.reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT | ||
&& this.props.reportAction.actorEmail === this.props.session.email | ||
&& this.props.reportAction.reportActionID; | ||
roryabraham marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
confirmDeleteAndHideModal() { | ||
deleteReportComment(this.props.reportID, this.props.reportAction); | ||
this.setState({isDeleteCommentConfirmModalVisible: false}); | ||
} | ||
|
||
hideDeleteConfirmModal() { | ||
this.setState({isDeleteCommentConfirmModalVisible: false}); | ||
} | ||
|
||
render() { | ||
return this.props.isVisible && ( | ||
<View style={this.wrapperStyle}> | ||
{this.CONTEXT_ACTIONS.map(contextAction => contextAction.shouldShow && ( | ||
{this.contextActions.map(contextAction => _.result(contextAction, 'shouldShow', false) && ( | ||
<ReportActionContextMenuItem | ||
icon={contextAction.icon} | ||
text={contextAction.text} | ||
|
@@ -162,6 +196,15 @@ class ReportActionContextMenu extends React.Component { | |
onPress={() => contextAction.onPress(this.props.reportAction)} | ||
/> | ||
))} | ||
<ConfirmModal | ||
title={this.props.translate('reportActionContextMenu.deleteComment')} | ||
isVisible={this.state.isDeleteCommentConfirmModalVisible} | ||
onConfirm={this.confirmDeleteAndHideModal} | ||
onCancel={this.hideDeleteConfirmModal} | ||
prompt={this.props.translate('reportActionContextMenu.deleteConfirmation')} | ||
confirmText={this.props.translate('common.delete')} | ||
cancelText={this.props.translate('common.cancel')} | ||
/> | ||
</View> | ||
); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -248,7 +248,14 @@ class ReportActionsView extends React.Component { | |
updateSortedReportActions(reportActions) { | ||
this.sortedReportActions = _.chain(reportActions) | ||
.sortBy('sequenceNumber') | ||
.filter(action => action.actionName === 'ADDCOMMENT' || action.actionName === 'IOU') | ||
.filter((action) => { | ||
// Only show non-empty ADDCOMMENT actions or IOU actions | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment does a good job of explaining "what" the code is doing, but not "why" it's doing it. The comment could be improved to explain why it's not showing any other actions. |
||
// Empty ADDCOMMENT actions typically mean they have been deleted and should not be shown | ||
const message = _.first(lodashGet(action, 'message', null)); | ||
This comment was marked as resolved.
Sorry, something went wrong. |
||
const html = lodashGet(message, 'html', ''); | ||
return action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU | ||
|| (action.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT && html !== ''); | ||
}) | ||
.map((item, index) => ({action: item, index})) | ||
.value() | ||
.reverse(); | ||
|
@@ -291,7 +298,7 @@ class ReportActionsView extends React.Component { | |
updateMostRecentIOUReportActionNumber(reportActions) { | ||
this.mostRecentIOUReportSequenceNumber = _.chain(reportActions) | ||
.sortBy('sequenceNumber') | ||
.filter(action => action.actionName === 'IOU') | ||
.filter(action => action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU) | ||
.max(action => action.sequenceNumber) | ||
.value().sequenceNumber; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is only the second time that something is making optimistic changes, other than adding a report comment. I think it's a cool pattern, and probably something that we should try to abstract at some point that would be easier for other actions to use as well. Nothing actionable here, but I wanted to note down the thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that sounds great, there is a certain amount of overhead to do it, so abstraction would be great, specially before we bring the expenses and reports into .cash