Skip to content

Replace Report_AddComment with AddComment (Part 2) #9350

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.

Sign up for GitHub

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

Merged
merged 21 commits into from
Jul 5, 2022
Merged

Replace Report_AddComment with AddComment (Part 2) #9350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
aac44cb
Add SystemInlineMessage and remove error handlers
marcaaron Jun 7, 2022
ae0e541
Fix up optimisticReportActions
marcaaron Jun 8, 2022
984f41e
revert updateReportWithNewAction change for now
marcaaron Jun 8, 2022
4b4bceb
use const
marcaaron Jun 8, 2022
0ea7e2d
use Onyx.update() for Airship
marcaaron Jun 8, 2022
5759035
Fix jest tests
marcaaron Jun 8, 2022
05d540d
Fix styles
marcaaron Jun 8, 2022
631932f
Update everything to use isPending instead of loading
marcaaron Jun 8, 2022
665b93b
remove unused spacing
marcaaron Jun 9, 2022
bf90793
fix conflicts
marcaaron Jun 9, 2022
33eb904
remove unused stuff
marcaaron Jun 9, 2022
d798541
Remove throttled timezone update call
marcaaron Jun 10, 2022
a7174be
Update both my details and personal details
marcaaron Jun 10, 2022
7ba89c1
stringify
marcaaron Jun 13, 2022
950ea10
Just keep CreateComment and CreateAttachment separate for now
marcaaron Jun 13, 2022
a6ad485
Keep the name save the refactor for later
marcaaron Jun 13, 2022
99306f1
Fix test
marcaaron Jun 13, 2022
de49280
fix conflicts
marcaaron Jun 15, 2022
d5131e5
Update command names
marcaaron Jun 16, 2022
3686169
Fix conflicts
marcaaron Jun 21, 2022
8761aa6
Update to reflect recent standards and Web-E changes
marcaaron Jun 23, 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
6 changes: 6 additions & 0 deletions src/CONST.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,12 @@ const CONST = {

// There's a limit of 60k characters in Auth - https://github.com/Expensify/Auth/blob/198d59547f71fdee8121325e8bc9241fc9c3236a/auth/lib/Request.h#L28
MAX_COMMENT_LENGTH: 60000,

ONYX: {
METHOD: {
MERGE: 'merge',
},
},
};

export default CONST;
7 changes: 6 additions & 1 deletion src/components/InlineSystemMessage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@ import Icon from './Icon';

const propTypes = {
/** Error to display */
message: PropTypes.string.isRequired,
message: PropTypes.string,
};

const defaultProps = {
message: '',
};

const InlineSystemMessage = (props) => {
Expand All @@ -25,5 +29,6 @@ const InlineSystemMessage = (props) => {
};

InlineSystemMessage.propTypes = propTypes;
InlineSystemMessage.defaultProps = defaultProps;
InlineSystemMessage.displayName = 'InlineSystemMessage';
export default InlineSystemMessage;
35 changes: 27 additions & 8 deletions src/libs/DateUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,21 +115,38 @@ function startCurrentDateUpdater() {
});
}

/*
* Updates user's timezone, if their timezone is set to automatic and
* is different from current timezone
/**
* @returns {Object}
*/
function updateTimezone() {
function getCurrentTimezone() {
const currentTimezone = moment.tz.guess(true);
if (timezone.automatic && timezone.selected !== currentTimezone) {
PersonalDetails.setPersonalDetails({timezone: {...timezone, selected: currentTimezone}});
return {...timezone, selected: currentTimezone};
}
return timezone;
}

/*
* Returns a version of updateTimezone function throttled by 5 minutes
* Updates user's timezone, if their timezone is set to automatic and
* is different from current timezone
*/
function updateTimezone() {
PersonalDetails.setPersonalDetails({timezone: getCurrentTimezone()});
}

// Used to throttle updates to the timezone when necessary
let lastUpdatedTimezoneTime = moment();

/**
* @returns {Boolean}
*/
const throttledUpdateTimezone = _.throttle(() => updateTimezone(), 1000 * 60 * 5);
function canUpdateTimezone() {
return lastUpdatedTimezoneTime.isBefore(moment().subtract(5, 'minutes'));
}

Comment on lines +143 to +146
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is just a refactor but why do we try to update every 5 minutes? feels like way too often, it probably only matters when a user is traveling but even then I don't really need my timezone updated every step of the way, just like when I arrive to my destination.

actually thinking about this more (i know this is all one comment but time passed between the first paragraph and this one)... why put a time limit on it at all? why not just check if the current timezone is different than the one in onyx and only attempt to set it if they are different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That could work. I'm not sure why we chose every 5 minutes or whether it would be easier to check if the timezone has changed. I think there is some kind of cost associated with trying to "guess" the timezone and maybe that's why this is throttled, but not really focused on improving this particular feature.

function setTimezoneUpdated() {
lastUpdatedTimezoneTime = moment();
}

/**
* @namespace DateUtils
Expand All @@ -139,8 +156,10 @@ const DateUtils = {
timestampToDateTime,
startCurrentDateUpdater,
updateTimezone,
throttledUpdateTimezone,
getLocalMomentFromTimestamp,
getCurrentTimezone,
canUpdateTimezone,
setTimezoneUpdated,
};

export default DateUtils;
155 changes: 64 additions & 91 deletions src/libs/actions/Report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import * as Pusher from '../Pusher/pusher';
import LocalNotification from '../Notification/LocalNotification';
import PushNotification from '../Notification/PushNotification';
import * as PersonalDetails from './PersonalDetails';
import * as User from './User';
import Navigation from '../Navigation/Navigation';
import * as ActiveClientManager from '../ActiveClientManager';
import Visibility from '../Visibility';
Expand All @@ -27,6 +26,7 @@ import * as ReportActions from './ReportActions';
import Growl from '../Growl';
import * as Localize from '../Localize';
import PusherUtils from '../PusherUtils';
import DateUtils from '../DateUtils';

let currentUserEmail;
let currentUserAccountID;
Expand Down Expand Up @@ -534,54 +534,6 @@ function updateReportActionMessage(reportID, sequenceNumber, message) {
});
}

/**
* Updates a report in the store with a new report action
*
* @param {Number} reportID
* @param {Object} reportAction
* @param {String} [notificationPreference] On what cadence the user would like to be notified
*/
function updateReportWithNewAction(
reportID,
reportAction,
notificationPreference = CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS,
) {
const messageHtml = reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.RENAMED
? lodashGet(reportAction, 'originalMessage.html', '')
: lodashGet(reportAction, ['message', 0, 'html'], '');

const parser = new ExpensiMark();
const messageText = parser.htmlToText(messageHtml);

const updatedReportObject = {
// Always merge the reportID into Onyx. If the report doesn't exist in Onyx yet, then all the rest of the data will be filled out by handleReportChanged
reportID,
maxSequenceNumber: reportAction.sequenceNumber,
notificationPreference,
lastMessageTimestamp: reportAction.timestamp,
lastMessageText: ReportUtils.formatReportLastMessageText(messageText),
lastActorEmail: reportAction.actorEmail,
};

Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, updatedReportObject);

const reportActionsToMerge = {};
if (reportAction.clientID) {
// Remove the optimistic action from the report since we are about to replace it
// with the real one (which has the true sequenceNumber)
reportActionsToMerge[reportAction.clientID] = null;
}

// Add the action into Onyx
reportActionsToMerge[reportAction.sequenceNumber] = {
...reportAction,
isAttachment: ReportUtils.isReportMessageAttachment(lodashGet(reportAction, ['message', 0], {})),
loading: false,
};

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

/**
* Get the private pusher channel name for a Report.
*
Expand All @@ -606,13 +558,6 @@ function subscribeToUserEvents() {
return;
}

// Live-update a report's actions when a 'report comment' event is received.
PusherUtils.subscribeToPrivateUserChannelEvent(
Pusher.TYPE.REPORT_COMMENT,
currentUserAccountID,
pushJSON => updateReportWithNewAction(pushJSON.reportID, pushJSON.reportAction, pushJSON.notificationPreference),
);

// Live-update a report's actions when an 'edit comment' event is received.
PusherUtils.subscribeToPrivateUserChannelEvent(Pusher.TYPE.REPORT_COMMENT_EDIT,
currentUserAccountID,
Expand All @@ -623,9 +568,9 @@ function subscribeToUserEvents() {
* Setup reportComment push notification callbacks.
*/
function subscribeToReportCommentPushNotifications() {
PushNotification.onReceived(PushNotification.TYPE.REPORT_COMMENT, ({reportID, reportAction}) => {
PushNotification.onReceived(PushNotification.TYPE.REPORT_COMMENT, ({reportID, onyxData}) => {
Log.info('[Report] Handled event sent by Airship', false, {reportID});
updateReportWithNewAction(reportID, reportAction);
Onyx.update(onyxData);
});

// Open correct report when push notification is clicked
Expand Down Expand Up @@ -903,7 +848,7 @@ function fetchAllReports(
* @param {String} text
* @param {File} [file]
*/
function addAction(reportID, text, file) {
function addComment(reportID, text, file) {
// For comments shorter than 10k chars, convert the comment from MD into HTML because that's how it is stored in the database
// For longer comments, skip parsing and display plaintext for performance reasons. It takes over 40s to parse a 100k long string!!
const parser = new ExpensiMark();
Expand All @@ -921,12 +866,12 @@ function addAction(reportID, text, file) {
: parser.htmlToText(htmlForNewComment);

// Update the report in Onyx to have the new sequence number
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
const optimisticReport = {
maxSequenceNumber: newSequenceNumber,
lastMessageTimestamp: moment().unix(),
lastMessageText: ReportUtils.formatReportLastMessageText(textForNewComment),
lastActorEmail: currentUserEmail,
});
};

// Generate a clientID so we can save the optimistic action to storage with the clientID as key. Later, we will
// remove the optimistic action when we add the real action created in the server. We do this because it's not
Expand All @@ -942,12 +887,10 @@ function addAction(reportID, text, file) {
// Store the optimistic action ID on the report the comment was added to. It will be removed later when refetching
// report actions in order to clear out any stuck actions (i.e. actions where the client never received a Pusher
// event, for whatever reason, from the server with the new action data
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
optimisticReportActionIDs: [...(optimisticReportActionIDs[reportID] || []), optimisticReportActionID],
});
optimisticReport.optimisticReportActionIDs = [...(optimisticReportActionIDs[reportID] || []), optimisticReportActionID];

// Optimistically add the new comment to the store before waiting to save it to the server
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, {
const optimisticReportActions = {
[optimisticReportActionID]: {
actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT,
actorEmail: currentUserEmail,
Expand Down Expand Up @@ -976,41 +919,70 @@ function addAction(reportID, text, file) {
isFirstItem: false,
isAttachment,
attachmentInfo,
loading: true,
isLoading: true,
shouldShow: true,
},
});
};

DeprecatedAPI.Report_AddComment({
const parameters = {
reportID,
file,
reportComment: commentText,
clientID: optimisticReportActionID,
persist: true,
})
.then((response) => {
if (response.jsonCode === 408) {
Growl.error(Localize.translateLocal('reportActionCompose.fileUploadFailed'));
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, {
[optimisticReportActionID]: null,
});
console.error(response.message);
return;
}

if (response.jsonCode === 666 && reportID === conciergeChatReportID) {
Growl.error(Localize.translateLocal('reportActionCompose.blockedFromConcierge'));
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, {
[optimisticReportActionID]: null,
});
};

// The fact that the API is returning this error means the BLOCKED_FROM_CONCIERGE nvp in the user details has changed since the last time we checked, so let's update
User.getUserDetails();
return;
}
const optimisticData = [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: optimisticReport,
},
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
value: optimisticReportActions,
},
];

updateReportWithNewAction(reportID, response.reportAction);
// Update the timezone if it's been 5 minutes from the last time the user added a comment
if (DateUtils.canUpdateTimezone()) {
const timezone = DateUtils.getCurrentTimezone();
parameters.timezone = JSON.stringify(timezone);
optimisticData.push({
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: ONYXKEYS.MY_PERSONAL_DETAILS,
value: {timezone},
});
optimisticData.push({
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: ONYXKEYS.PERSONAL_DETAILS,
value: {[currentUserEmail]: timezone},
});
DateUtils.setTimezoneUpdated();
}

API.write(isAttachment ? 'AddAttachment' : 'AddComment', parameters, {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think here I might have preferred a separate variable like

const command = isAttachment ? 'AddAttachment' : 'AddComment';
API.write(command, parameters, ...);

or maybe even getting rid of isAttachment and renaming that command at the top of the method and instead forking logic if command is AddAttachment or AddComment though that might end up being way too verbose 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

eh i guess looking back we use isAttachment inside of the onxyData and a few other places so maybe this is the best route the way it's written.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't really have strong feelings one way or the other on this.

optimisticData,
failureData: [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
value: {
[optimisticReportActionID]: {
isLoading: false,
},
},
},
],
});
}

/**
* @param {Number} reportID
* @param {Object} file
*/
function addAttachment(reportID, file) {
addComment(reportID, '', file);
}

/**
Expand Down Expand Up @@ -1521,7 +1493,8 @@ export {
fetchChatReportsByIDs,
fetchIOUReportByID,
fetchIOUReportByIDAndUpdateChatReport,
addAction,
addComment,
addAttachment,
updateLastReadActionID,
updateNotificationPreference,
setNewMarkerPosition,
Expand Down
2 changes: 1 addition & 1 deletion src/libs/actions/ReportActions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ Onyx.connect({
const reportID = CollectionUtils.extractCollectionItemID(key);
const actionsArray = _.toArray(actions);
reportActions[reportID] = actionsArray;
const mostRecentNonLoadingActionIndex = _.findLastIndex(actionsArray, action => !action.loading);
const mostRecentNonLoadingActionIndex = _.findLastIndex(actionsArray, action => !action.isLoading);
const mostRecentAction = actionsArray[mostRecentNonLoadingActionIndex];
if (!mostRecentAction || _.isUndefined(mostRecentAction.sequenceNumber)) {
return;
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/ReportScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ class ReportScreen extends React.Component {
* @param {String} text
*/
onSubmitComment(text) {
Report.addAction(getReportID(this.props.route), text);
Report.addComment(getReportID(this.props.route), text);
}

/**
Expand Down
5 changes: 1 addition & 4 deletions src/pages/home/report/ReportActionCompose.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import ReportActionComposeFocusManager from '../../../libs/ReportActionComposeFo
import participantPropTypes from '../../../components/participantPropTypes';
import ParticipantLocalTime from './ParticipantLocalTime';
import {withPersonalDetails} from '../../../components/OnyxProvider';
import DateUtils from '../../../libs/DateUtils';
import * as User from '../../../libs/actions/User';
import Tooltip from '../../../components/Tooltip';
import EmojiPickerButton from '../../../components/EmojiPicker/EmojiPickerButton';
Expand Down Expand Up @@ -448,8 +447,6 @@ class ReportActionCompose extends React.Component {
return;
}

DateUtils.throttledUpdateTimezone();

this.props.onSubmit(trimmedComment);
this.updateComment('');
this.setTextInputShouldClear(true);
Expand Down Expand Up @@ -498,7 +495,7 @@ class ReportActionCompose extends React.Component {
headerTitle={this.props.translate('reportActionCompose.sendAttachment')}
onConfirm={(file) => {
this.submitForm();
Report.addAction(this.props.reportID, '', file);
Report.addAttachment(this.props.reportID, file);
this.setTextInputShouldClear(false);
}}
>
Expand Down
Loading