Skip to content
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

Refactor Edit and Delete Report Comment #9532

Merged
merged 89 commits into from
Aug 31, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
89 commits
Select commit Hold shift + click to select a range
827e0cf
Add new API call to edit and delete comment endpoints
Jun 22, 2022
47f54ac
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Jun 23, 2022
82309d7
Use optimisc data on comment edit and delete methods
Jun 23, 2022
5cd5c15
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Jun 27, 2022
3246ff7
Fix reportActionID error for deleteComment
Jun 27, 2022
8f23138
Send reportAction optimistic data on edit
Jun 28, 2022
30991b5
Update report last message if user edits it
Jun 28, 2022
483b78b
Lint and add missing commas and semi-colons
Jun 28, 2022
0da2900
Add ONYX MERGE to const file
Jun 28, 2022
f8bf959
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Jul 7, 2022
e4d3b9f
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Jul 11, 2022
4c8f633
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Jul 12, 2022
8de9a89
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Jul 14, 2022
63219b8
Refactor update and delete with main changes
Jul 14, 2022
52d6960
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Jul 15, 2022
39c06b6
Fix optimistic report data last message
Jul 15, 2022
f42ae84
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Jul 18, 2022
554e247
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Jul 20, 2022
818be57
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Jul 27, 2022
769d43e
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 4, 2022
1ee29e6
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 8, 2022
53ae16a
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 9, 2022
2c808ce
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 9, 2022
62401ee
Display offline pending actions to update/delete comment
Aug 9, 2022
afc8fc6
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 10, 2022
c12fcd1
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 10, 2022
1888216
Remove OfflineWithFeedback from ReportActionList
Aug 10, 2022
22f5e1b
Remove unused reportID from reportActions method
Aug 10, 2022
27c5bc9
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 11, 2022
3b3d43d
Update edit message with success and fail data
Aug 11, 2022
6afbf4a
Send clientID on message update
Aug 11, 2022
b633c47
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 11, 2022
bfc8584
Remove deprecatedAPI editComment method
Aug 11, 2022
2e70d47
Send clientID on message delete
Aug 11, 2022
fc0803b
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 12, 2022
c000554
Keep message content on deletion until success
Aug 12, 2022
d57dd14
Add strike-trough to message on delete while offline
Aug 12, 2022
27361a7
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 15, 2022
f06eac4
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 15, 2022
7dc9e3f
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 16, 2022
bb0f552
Clear sequenceNumber errors on offlineIndicator close btn
Aug 16, 2022
4bce6bf
Check if sequenceNumber and clientID are supplied before clearing
Aug 16, 2022
5f713be
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 17, 2022
ee002be
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 17, 2022
03115f8
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 18, 2022
b715aed
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 18, 2022
bcc2b66
Hide message context menu when pending deletion
Aug 18, 2022
69ba9f2
Remove comments from Report.js
Aug 18, 2022
9a5b5bd
Remove clientID from update and delete comment
Aug 18, 2022
70ef475
Remove duplicated text message variable
Aug 18, 2022
545977e
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 19, 2022
dd41dad
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 22, 2022
ab9772f
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 23, 2022
e2bbed7
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 24, 2022
26d2704
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 25, 2022
b20e330
Fix sequenceNumber type for deleteOptimisticReportAction after merge
Aug 25, 2022
c9cc3b8
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 25, 2022
98c3ed3
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 25, 2022
90ae734
Merge remote-tracking branch 'origin' into paulogasparsv-report-edit-…
marcaaron Aug 25, 2022
d0784e4
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 26, 2022
82c150b
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 29, 2022
6535d05
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 30, 2022
68a32c7
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 30, 2022
f8146dd
Fix typos and refactor Report.js
Aug 30, 2022
70b383f
Clean up deleteReportComment logic
Aug 30, 2022
219dae4
Update unreadActionCount on deletion
Aug 30, 2022
493768a
Remove old editComment subscription logic
Aug 30, 2022
c335b0a
Remove old editComment subscription logic reference
Aug 30, 2022
c498a35
Remove remaining references to old pusher subscriber for edit comment
Aug 30, 2022
e0cc84b
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 30, 2022
7c5a71a
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 31, 2022
b6a179e
Spread reportActionItemMessage styles property
Aug 31, 2022
d2f86c2
Only hide delete function instead of entire contextMenu when pending …
Aug 31, 2022
a41fa84
Remove edit option for pending deleted comments
Aug 31, 2022
2fb375c
Add getLastVisibleReportAction utility
Aug 31, 2022
765b0d8
Update lastMessageText if latest visible message is edited
Aug 31, 2022
0c3e9ce
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-re…
Aug 31, 2022
91841dc
Merge branch 'paulogasparsv-report-edit-comment-refactor' of github.c…
marcaaron Aug 31, 2022
827ad50
Remove unreadActionCount rollback
Aug 31, 2022
0e0376c
Fix reference typo for getLastVisibleReportAction
Aug 31, 2022
e076529
Fix underscore findLast non-existing reference
Aug 31, 2022
fb063f0
Dismiss errors for edit and delete comment
Aug 31, 2022
70f6d27
Clear errors after deletion success for edge case
Aug 31, 2022
093bc57
Merge branch 'paulogasparsv-report-edit-comment-refactor' of github.c…
marcaaron Aug 31, 2022
c39972a
Removing unneeded things
marcaaron Aug 31, 2022
542d0e1
remove some other changes
marcaaron Aug 31, 2022
2f9de3d
Remove lastVisibleReportAction export
marcaaron Aug 31, 2022
764a569
fix typo
marcaaron Aug 31, 2022
b5b7fb3
this is actually needed
marcaaron Aug 31, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/libs/Navigation/AppNavigator/AuthScreens.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import * as StyleUtils from '../../../styles/StyleUtils';
import withWindowDimensions, {windowDimensionsPropTypes} from '../../../components/withWindowDimensions';
import CONST from '../../../CONST';
import compose from '../../compose';
import * as Report from '../../actions/Report';
import * as PersonalDetails from '../../actions/PersonalDetails';
import * as Pusher from '../../Pusher/pusher';
import PusherConnectionManager from '../../PusherConnectionManager';
Expand Down Expand Up @@ -110,7 +109,6 @@ class AuthScreens extends React.Component {
cluster: CONFIG.PUSHER.CLUSTER,
authEndpoint: `${CONFIG.EXPENSIFY.URL_API_ROOT}api?command=AuthenticatePusher`,
}).then(() => {
Report.subscribeToUserEvents();
User.subscribeToUserEvents();
Policy.subscribeToPolicyEvents();
});
Expand Down
1 change: 0 additions & 1 deletion src/libs/Pusher/EventType.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
*/
export default {
REPORT_COMMENT: 'reportComment',
REPORT_COMMENT_EDIT: 'reportCommentEdit',
PREFERRED_LOCALE: 'preferredLocale',
EXPENSIFY_CARD_UPDATE: 'expensifyCardUpdate',
SCREEN_SHARE_REQUEST: 'screenshareRequest',
Expand Down
6 changes: 4 additions & 2 deletions src/libs/ReportUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,8 @@ function canEditReportAction(reportAction) {
return reportAction.actorEmail === sessionEmail
&& reportAction.reportActionID
&& reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT
&& !isReportMessageAttachment(lodashGet(reportAction, ['message', 0], {}));
&& !isReportMessageAttachment(lodashGet(reportAction, ['message', 0], {}))
&& reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE;
}

/**
Expand All @@ -113,7 +114,8 @@ function canEditReportAction(reportAction) {
function canDeleteReportAction(reportAction) {
return reportAction.actorEmail === sessionEmail
&& reportAction.reportActionID
&& reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT;
&& reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT
&& reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE;
}

/**
Expand Down
266 changes: 132 additions & 134 deletions src/libs/actions/Report.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import * as ReportUtils from '../ReportUtils';
import * as ReportActions from './ReportActions';
import Growl from '../Growl';
import * as Localize from '../Localize';
import PusherUtils from '../PusherUtils';
import DateUtils from '../DateUtils';
import * as ReportActionsUtils from '../ReportActionsUtils';
import * as NumberUtils from '../NumberUtils';
Expand Down Expand Up @@ -436,48 +435,6 @@ function getReportChannelName(reportID) {
return `${CONST.PUSHER.PRIVATE_REPORT_CHANNEL_PREFIX}${reportID}${CONFIG.PUSHER.SUFFIX}`;
}

/**
* Initialize our pusher subscriptions to listen for new report comments and pin toggles
*/
function subscribeToUserEvents() {
// If we don't have the user's accountID yet we can't subscribe so return early
if (!currentUserAccountID) {
return;
}

const pusherChannelName = `${CONST.PUSHER.PRIVATE_USER_CHANNEL_PREFIX}${currentUserAccountID}${CONFIG.PUSHER.SUFFIX}`;
if (Pusher.isSubscribed(pusherChannelName) || Pusher.isAlreadySubscribing(pusherChannelName)) {
return;
}

// Live-update a report's actions when an 'edit comment' event is received.
PusherUtils.subscribeToPrivateUserChannelEvent(Pusher.TYPE.REPORT_COMMENT_EDIT,
currentUserAccountID,
({reportID, sequenceNumber, message}) => {
// We only want the active client to process these events once otherwise multiple tabs would decrement the 'unreadActionCount'
if (!ActiveClientManager.isClientTheLeader()) {
return;
}

const actionsToMerge = {};
actionsToMerge[sequenceNumber] = {message: [message]};

// If someone besides the current user deleted an action and the sequenceNumber is greater than our last read we will decrement the unread count
// we skip this for the current user because we should already have decremented the count optimistically when they deleted the comment.
const isFromCurrentUser = ReportActions.isFromCurrentUser(reportID, sequenceNumber, currentUserAccountID, actionsToMerge);
if (!message.html && !isFromCurrentUser && sequenceNumber > getLastReadSequenceNumber(reportID)) {
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
unreadActionCount: Math.max(getUnreadActionCount(reportID) - 1, 0),
});
}

Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
lastMessageText: ReportActions.getLastVisibleMessageText(reportID, actionsToMerge),
});
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, actionsToMerge);
});
}

/**
* Setup reportComment push notification callbacks.
*/
Expand Down Expand Up @@ -880,73 +837,6 @@ function addComment(reportID, text) {
addActions(reportID, text);
}

/**
* Deletes a comment from the report, basically sets it as empty string
*
* @param {Number} reportID
* @param {Object} reportAction
*/
function deleteReportComment(reportID, reportAction) {
// Optimistic Response
const sequenceNumber = reportAction.sequenceNumber;
const reportActionsToMerge = {};
const oldMessage = {...reportAction.message};
reportActionsToMerge[sequenceNumber] = {
...reportAction,
message: [
{
type: CONST.REPORT.MESSAGE.TYPE.COMMENT,
html: '',
text: '',
},
],
};

// If the comment we are deleting is more recent than our last read comment we will update the unread count
if (sequenceNumber > getLastReadSequenceNumber(reportID)) {
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
unreadActionCount: Math.max(getUnreadActionCount(reportID) - 1, 0),
});
}

// Optimistically update the report and reportActions
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, reportActionsToMerge);
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
lastMessageText: ReportActions.getLastVisibleMessageText(reportID, reportActionsToMerge),
});

// Try to delete the comment by calling the API
DeprecatedAPI.Report_EditComment({
reportID,
reportActionID: reportAction.reportActionID,
reportComment: '',
sequenceNumber,
})
.then((response) => {
if (response.jsonCode === 200) {
return;
}

// Reverse Optimistic Response
reportActionsToMerge[sequenceNumber] = {
...reportAction,
message: oldMessage,
};

if (sequenceNumber > getLastReadSequenceNumber(reportID)) {
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
unreadActionCount: getUnreadActionCount(reportID) + 1,
});
}

Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
lastMessageText: ReportActions.getLastVisibleMessageText(reportID, reportActionsToMerge),
});

Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, reportActionsToMerge);
});
}

/**
* Gets the latest page of report actions and updates the last read message
*
Expand Down Expand Up @@ -1216,6 +1106,88 @@ Onyx.connect({
callback: handleReportChanged,
});

/**
* Deletes a comment from the report, basically sets it as empty string
*
* @param {Number} reportID
* @param {Object} reportAction
*/
function deleteReportComment(reportID, reportAction) {
const sequenceNumber = reportAction.sequenceNumber;
const optimisticReportActions = {
[sequenceNumber]: {
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
},
PauloGasparSv marked this conversation as resolved.
Show resolved Hide resolved
};

// If we are deleting the last visible message, let's find the previous visible one
// and update the lastMessageText in the chat preview.
const optimisticReport = {
lastMessageText: ReportActions.getLastVisibleMessageText(reportID, {
[sequenceNumber]: {
message: [{
html: '',
text: '',
}],
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe could use a comment here to explain why we need to update the lastMessageText here? Is it only necessary when we are deleting the last visible message?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I think this should be passing in the modified message.. like:

    const optimisticReport = {
        lastMessageText: ReportActions.getLastVisibleMessageText(reportID, {
            [sequenceNumber]: {
                ...reportAction,
                message: [{
                    html: '',
                    text: '',
                }],
            },

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not that sure we need all that anymore except for the unreadActionCount bits! Things weren't so clear for me when I started working on the app :p

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, I think we do need. Not sure why though, I thought the API would update lastMessageText for the sender and recipient. Need some more time to understand everything

}),
};

// If the API call fails we must show the original message again, so we revert the message content back to how it was
// and and remove the pendingAction so the strike-through clears
const failureData = [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
value: {
[sequenceNumber]: {
message: reportAction.message,
pendingAction: null,
},
},
},
];

const successData = [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
value: {
[sequenceNumber]: {
pendingAction: null,
},
},
},
];

// If we are deleting an unread message that is greater than our last read we decrease the unreadActionCount
// since the message we are deleting is an unread
if (sequenceNumber > getLastReadSequenceNumber(reportID)) {
const unreadActionCount = getUnreadActionCount(reportID);
optimisticReport.unreadActionCount = Math.max(unreadActionCount - 1, 0);
}

const optimisticData = [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
value: optimisticReportActions,
},
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: optimisticReport,
},
];

const parameters = {
reportID,
sequenceNumber,
reportActionID: reportAction.reportActionID,
};
API.write('DeleteComment', parameters, {optimisticData, successData, failureData});
}

/**
* Saves a new message for a comment. Marks the comment as edited, which will be reflected in the UI.
*
Expand All @@ -1241,32 +1213,59 @@ function editReportComment(reportID, originalReportAction, textForNewComment) {
return;
}

// Optimistically update the report action with the new message
// Optimistically update the reportAction with the new message
const sequenceNumber = originalReportAction.sequenceNumber;
const newReportAction = {...originalReportAction};
const actionToMerge = {};
newReportAction.message[0].isEdited = true;
newReportAction.message[0].html = htmlForNewComment;
newReportAction.message[0].text = parser.htmlToText(htmlForNewComment);
actionToMerge[sequenceNumber] = newReportAction;
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, actionToMerge);

// Persist the updated report comment
DeprecatedAPI.Report_EditComment({
const optimisticReportActions = {
[sequenceNumber]: {
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
message: [{
isEdited: true,
html: htmlForNewComment,
text: textForNewComment,
}],
},
};

const optimisticData = [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
value: optimisticReportActions,
},
];

const failureData = [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
value: {
[sequenceNumber]: {
...originalReportAction,
pendingAction: null,
},
},
},
];

const successData = [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
value: {
[sequenceNumber]: {
pendingAction: null,
},
},
},
];

const parameters = {
reportID,
reportActionID: originalReportAction.reportActionID,
reportComment: htmlForNewComment,
sequenceNumber,
})
.then((response) => {
if (response.jsonCode === 200) {
return;
}

// If it fails, reset Onyx
actionToMerge[sequenceNumber] = originalReportAction;
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, actionToMerge);
});
reportComment: htmlForNewComment,
reportActionID: originalReportAction.reportActionID,
};
API.write('UpdateComment', parameters, {optimisticData, successData, failureData});
}

/**
Expand Down Expand Up @@ -1582,7 +1581,6 @@ export {
updateNotificationPreference,
setNewMarkerPosition,
subscribeToReportTypingEvents,
subscribeToUserEvents,
subscribeToReportCommentPushNotifications,
unsubscribeFromReportChannel,
saveReportComment,
Expand Down
13 changes: 13 additions & 0 deletions src/libs/actions/ReportActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,22 @@ function deleteOptimisticReportAction(reportID, sequenceNumber) {
});
}

/**
* @param {Number} reportID
* @param {String} sequenceNumber
*/
function clearReportActionErrors(reportID, sequenceNumber) {
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, {
[sequenceNumber]: {
errors: null,
},
});
}

export {
getDeletedCommentsCount,
getLastVisibleMessageText,
clearReportActionErrors,
isFromCurrentUser,
deleteOptimisticReportAction,
};
Loading