Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
004d79f
Order by most recent along with report icons
chiragsalian Nov 5, 2020
2afde47
code for chat report icons
chiragsalian Nov 6, 2020
4e73d87
icon fix
chiragsalian Nov 6, 2020
35918f7
Fixes to support add to group button
chiragsalian Nov 6, 2020
4a26675
Fixes to search and ordering
chiragsalian Nov 7, 2020
62a157d
record action properly and sorting improvements
chiragsalian Nov 10, 2020
2da2772
cleanups
chiragsalian Nov 10, 2020
2cdf9a4
remove console
chiragsalian Nov 10, 2020
226d702
Merge branch 'master' into chirag-show-most-recent
chiragsalian Nov 10, 2020
68b4d76
moving icon logic from switcherList to row
chiragsalian Nov 10, 2020
1746fc8
style fix
chiragsalian Nov 10, 2020
5f11d83
minor correction
chiragsalian Nov 10, 2020
78d43e8
Update as per discussion to not sort during search
chiragsalian Nov 11, 2020
8dc27d7
style fix
chiragsalian Nov 11, 2020
a259d7a
Merge branch 'master' into chirag-show-most-recent
chiragsalian Nov 11, 2020
6e87007
single icon
chiragsalian Nov 11, 2020
306d88a
Icon and search revert to regex with some improvements
chiragsalian Nov 11, 2020
32f5988
cleanup
chiragsalian Nov 11, 2020
10b7c7b
Merge branch 'master' into chirag-show-most-recent
chiragsalian Nov 11, 2020
40c04f5
correction to use Onyx
chiragsalian Nov 11, 2020
d0daf83
Addressing review comments
chiragsalian Nov 12, 2020
35e5876
Update to use lastRead
chiragsalian Nov 12, 2020
decebef
moar comments and some cleanup
chiragsalian Nov 12, 2020
ff11e79
comment update
chiragsalian Nov 12, 2020
31af299
Merge branch 'master' into chirag-show-most-recent
chiragsalian Nov 20, 2020
0e0683e
Merge branch 'master' into chirag-show-most-recent
chiragsalian Nov 25, 2020
85a5b8c
unneeded unread check
chiragsalian Nov 26, 2020
fd27e33
Correcting logic
chiragsalian Nov 26, 2020
0d2a024
Some corrections around the group chat and sorting logic
chiragsalian Nov 26, 2020
9a5cf51
revert accidently pushed change
chiragsalian Nov 27, 2020
8a7c43c
Some readability improvements
chiragsalian Nov 27, 2020
69bf6ab
addressing comments
chiragsalian Nov 30, 2020
f2d9f35
updating variable name
chiragsalian Dec 1, 2020
47c8f95
updating key to sequenceNumber to match web-e
chiragsalian Dec 1, 2020
f9a2f9b
Merge branch 'master' into chirag-show-most-recent
chiragsalian Dec 4, 2020
dd483ac
corrections and a bug fix
chiragsalian Dec 4, 2020
ecb15e7
Merge branch 'master' into chirag-show-most-recent
chiragsalian Dec 8, 2020
5b1d9a9
Update to match code style
chiragsalian Dec 8, 2020
d437b3e
cleaning up some flows
chiragsalian Dec 8, 2020
f5406fb
moar cleanup
chiragsalian Dec 8, 2020
a9ebd92
not calling updateSearch twice since the same happens in focus
chiragsalian Dec 9, 2020
5161b3e
comment updates
chiragsalian Dec 10, 2020
94dcd24
Merge branch 'master' into chirag-show-most-recent
chiragsalian Dec 10, 2020
4c52b72
comment correction
chiragsalian Dec 10, 2020
3469012
updating language
chiragsalian Dec 10, 2020
5bff415
correcting comment
chiragsalian Dec 10, 2020
625097f
cleanup
chiragsalian Dec 10, 2020
2f5447d
adding back formatPersonalDetails that i mistakenly removed
chiragsalian Dec 10, 2020
d1d5329
Fix for empty avatar icon
chiragsalian Dec 10, 2020
fdb673e
missed colon
chiragsalian Dec 10, 2020
22104c9
Added comment
chiragsalian Dec 10, 2020
022ca53
correction
chiragsalian Dec 10, 2020
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
4 changes: 4 additions & 0 deletions src/CONST.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ const CONST = {
CLOUDFRONT_URL,
PDF_VIEWER_URL: '/pdf/web/viewer.html',
EXPENSIFY_ICON_URL: `${CLOUDFRONT_URL}/images/favicon-2019.png`,
REPORT: {
SINGLE_USER_DM: 'singleUserDM',
GROUP_USERS_DM: 'groupUsersDM',
},
};

export default CONST;
3 changes: 2 additions & 1 deletion src/components/TextInputWithFocusStyles.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const propTypes = {
style: PropTypes.any,

// A function to call when the input has been blurred
onBlur: PropTypes.func.isRequired,
onBlur: PropTypes.func,

// A function to call when the input has gotten focus
onFocus: PropTypes.func.isRequired,
Expand All @@ -29,6 +29,7 @@ const defaultProps = {
styleFocusIn: null,
styleFocusOut: null,
style: null,
onBlur: () => {},
};

class TextInputWithFocusStyles extends React.Component {
Expand Down
9 changes: 4 additions & 5 deletions src/libs/API.js
Original file line number Diff line number Diff line change
Expand Up @@ -387,10 +387,9 @@ function Report_TogglePinned(parameters) {
* @param {Number} parameters.sequenceNumber
* @returns {Promise}
*/
function Report_SetLastReadActionID(parameters) {
const commandName = 'Report_SetLastReadActionID';
requireParameters(['accountID', 'reportID', 'sequenceNumber'],
parameters, commandName);
function Report_UpdateLastRead(parameters) {
const commandName = 'Report_UpdateLastRead';
requireParameters(['accountID', 'reportID', 'sequenceNumber'], parameters, commandName);
return request(commandName, parameters);
}

Expand All @@ -407,5 +406,5 @@ export {
Report_AddComment,
Report_GetHistory,
Report_TogglePinned,
Report_SetLastReadActionID
Report_UpdateLastRead
};
53 changes: 43 additions & 10 deletions src/libs/actions/PersonalDetails.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,19 @@ Onyx.connect({
callback: val => personalDetails = val,
});

/**
* Helper method to return a default avatar
*
* @param {String} login
* @returns {String}
*/
function getDefaultAvatar(login) {
Comment thread
chiragsalian marked this conversation as resolved.
// There are 8 possible default avatars, so we choose which one this user has based
// on a simple hash of their login (which is converted from HEX to INT)
const loginHashBucket = (parseInt(md5(login).substring(0, 4), 16) % 8) + 1;
return `${CONST.CLOUDFRONT_URL}/images/avatars/avatar_${loginHashBucket}.png`;
}

/**
* Returns the URL for a user's avatar and handles someone not having any avatar at all
*
Expand All @@ -31,10 +44,7 @@ function getAvatar(personalDetail, login) {
return personalDetail.avatar.replace(/&d=404$/, '');
}

// There are 8 possible default avatars, so we choose which one this user has based
// on a simple hash of their login (which is converted from HEX to INT)
const loginHashBucket = (parseInt(md5(login).substring(0, 4), 16) % 8) + 1;
return `${CONST.CLOUDFRONT_URL}/images/avatars/avatar_${loginHashBucket}.png`;
return getDefaultAvatar(login);
}

/**
Expand Down Expand Up @@ -129,15 +139,38 @@ function fetch() {
}

/**
* Get personal details for a list of emails.
* Get personal details from report participants.
*
* @param {String} emailList
* @param {Object} reports
*/
function getForEmails(emailList) {
API.PersonalDetails_GetForEmails({emailList})
function getFromReportParticipants(reports) {
const participantEmails = _.chain(reports)
.pluck('participants')
.flatten()
.unique()
.value();

if (participantEmails.length === 0) {
return;
}

API.PersonalDetails_GetForEmails({emailList: participantEmails.join(',')})
.then((data) => {
const details = _.pick(data, emailList.split(','));
const details = _.pick(data, participantEmails);
Onyx.merge(ONYXKEYS.PERSONAL_DETAILS, formatPersonalDetails(details));
Comment thread
chiragsalian marked this conversation as resolved.

// The personalDetails of the participants contain their avatar images. Here we'll go over each
// report and based on the participants we'll link up their avatars to report icons.
_.each(reports, (report) => {
if (report.participants.length === 1) {
const dmParticipant = report.participants[0];
let icon = lodashGet(details, [dmParticipant, 'avatar'], '');
if (!icon) {
icon = getDefaultAvatar(dmParticipant);
}
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, {icon});
}
});
});
Comment thread
marcaaron marked this conversation as resolved.
}

Expand All @@ -147,6 +180,6 @@ NetworkConnection.onReconnect(fetch);
export {
fetch,
fetchTimezone,
getForEmails,
getFromReportParticipants,
getDisplayName,
};
66 changes: 37 additions & 29 deletions src/libs/actions/Report.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ const typingWatchTimers = {};
const reportMaxSequenceNumbers = {};

// Keeps track of the last read for each report
const lastReadActionIDs = {};
const lastReadSequenceNumbers = {};

/**
* Checks the report to see if there are any unread action items
Expand All @@ -62,25 +62,31 @@ const lastReadActionIDs = {};
* @returns {Boolean}
*/
function getUnreadActionCount(report) {
const usersLastReadActionID = lodashGet(report, [
// @todo remove the first check as part of cleanup https://github.com/Expensify/Expensify/issues/145243
Comment thread
chiragsalian marked this conversation as resolved.
// since we migrating our data from lastReadActionID_ value to lastRead_ object.
const lastReadSequenceNumber = lodashGet(report, [
'reportNameValuePairs',
`lastReadActionID_${currentUserAccountID}`,
]) || lodashGet(report, [
'reportNameValuePairs',
`lastRead_${currentUserAccountID}`,
'sequenceNumber',
]);

// Save the lastReadActionID locally so we can access this later
lastReadActionIDs[report.reportID] = usersLastReadActionID;
lastReadSequenceNumbers[report.reportID] = lastReadSequenceNumber;

if (report.reportActionList.length === 0) {
return 0;
}

if (!usersLastReadActionID) {
if (!lastReadSequenceNumber) {
return report.reportActionList.length;
}

// There are unread items if the last one the user has read is less
// than the highest sequence number we have
const unreadActionCount = report.reportActionList.length - usersLastReadActionID;
const unreadActionCount = report.reportActionList.length - lastReadSequenceNumber;
return Math.max(0, unreadActionCount);
}

Expand All @@ -107,11 +113,15 @@ function getSimplifiedReportObject(report) {
return {
reportID: report.reportID,
reportName: report.reportName,
reportNameValuePairs: report.reportNameValuePairs,
Comment thread
marcaaron marked this conversation as resolved.
unreadActionCount: getUnreadActionCount(report),
maxSequenceNumber: report.reportActionList.length,
participants: getParticipantEmailsFromReport(report),
isPinned: report.isPinned,
lastVisitedTimestamp: lodashGet(report, [
'reportNameValuePairs',
`lastRead_${currentUserAccountID}`,
'timestamp'
], 0)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NAB not sure if it's very valuable but a utility that let us do

const lastRead = getLastRead(report, accountID);
console.log(lastRead.sequenceNumber);
console.log(lastRead.timestamp);

might help us DRY things up a little instead of having to use lodashGet

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah i don't see much value in a utility function like so when it can be directly accessed in lodashGet. Like atm i don't see a point of returning an object than the required value directly.

};
}

Expand Down Expand Up @@ -148,15 +158,14 @@ function fetchChatReportsByIDs(chatList) {
.then(({reports}) => {
fetchedReports = reports;

// Build array of all participant emails so we can
// get the personal details.
let participantEmails = [];

// Process the reports and store them in Onyx
// Process the reports and store them in Onyx. At the same time we'll save the simplified reports in this
// variable called simplifiedReports which hold the participants (minus the current user) for each report.
// Using this simplifiedReport we can call PersonalDetails.getFromReportParticipants to get the
// personal details of all the participants and even link up their avatars to report icons.
const simplifiedReports = [];
_.each(fetchedReports, (report) => {
const newReport = getSimplifiedReportObject(report);

participantEmails.push(newReport.participants);
simplifiedReports.push(newReport);
Comment thread
chiragsalian marked this conversation as resolved.

if (lodashGet(report, 'reportNameValuePairs.type') === 'chat') {
newReport.reportName = getChatReportName(report.sharedReportList);
Expand All @@ -166,31 +175,26 @@ function fetchChatReportsByIDs(chatList) {
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, newReport);
});

// Fetch the person details if there are any
participantEmails = _.unique(participantEmails);
if (participantEmails && participantEmails.length !== 0) {
PersonalDetails.getForEmails(participantEmails.join(','));
}
// Fetch the personal details if there are any
PersonalDetails.getFromReportParticipants(simplifiedReports);

return _.map(fetchedReports, report => report.reportID);
});
}

/**
* Update the lastReadActionID in Onyx and local memory.
* Update the lastRead actionID and timestamp in local memory and Onyx
*
* @param {Number} reportID
* @param {Number} sequenceNumber
*/
function setLocalLastReadActionID(reportID, sequenceNumber) {
lastReadActionIDs[reportID] = sequenceNumber;
function setLocalLastRead(reportID, sequenceNumber) {
lastReadSequenceNumbers[reportID] = sequenceNumber;

// Update the lastReadActionID on the report optimistically
// Update the report optimistically
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
unreadActionCount: 0,
reportNameValuePairs: {
[`lastReadActionID_${currentUserAccountID}`]: sequenceNumber,
}
lastVisitedTimestamp: Date.now(),
});
}

Expand All @@ -208,15 +212,15 @@ function updateReportWithNewAction(reportID, reportAction) {
// last read actionID has been updated in the server but not necessarily reflected
// locally so we must first update it and then calculate the unread (which should be 0)
if (isFromCurrentUser) {
setLocalLastReadActionID(reportID, newMaxSequenceNumber);
setLocalLastRead(reportID, newMaxSequenceNumber);
}

// 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
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
reportID,
unreadActionCount: newMaxSequenceNumber - (lastReadActionIDs[reportID] || 0),
unreadActionCount: newMaxSequenceNumber - (lastReadSequenceNumbers[reportID] || 0),
maxSequenceNumber: reportAction.sequenceNumber,
});

Expand Down Expand Up @@ -451,6 +455,10 @@ function fetchOrCreateChatReport(participants) {
const newReport = getSimplifiedReportObject(report);
newReport.reportName = getChatReportName(report.sharedReportList);

// Optimistically update the last visited timestamp such that if the user immediately switches to another
// report the last visited order is still maintained.
newReport.lastVisitedTimestamp = Date.now();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NAB/possible follow up... one thought I'm having is that we could also update the lastRead_ NVP timestamp for the user who created this report when calling CreateChatReport. Then we would not need to do Date.now() but perhaps it doesn't matter. I find it slightly confusing that we would need to do this here in the client instead of refer to the NVP that we fetch immediately after creating the report.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Technically we don't even need this code because Report_UpdateLastRead is called shortly after which will set the NVP but this code is just faster so that if the user switches out of a new report quickly they'll see the change immediately. I don't think CreateChatReport needs to be updated because Report_UpdateLastRead is called shortly after though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That makes sense to me I think. Except this part...

if the user switches out of a new report quickly they'll see the change immediately

Not sure it will be obvious to anyone scanning this code that this is the reason we are setting this on the report. How much faster does it make things? If we really want to keep it maybe we can add some context here for why it's being updated immediately.


// Merge the data into Onyx. Don't use set() here or multiSet() because then that would
// overwrite any existing data (like if they have unread messages)
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, newReport);
Expand Down Expand Up @@ -536,10 +544,10 @@ function updateLastReadActionID(reportID, sequenceNumber) {
return;
}

setLocalLastReadActionID(reportID, sequenceNumber);
setLocalLastRead(reportID, sequenceNumber);

// Mark the report as not having any unread items
API.Report_SetLastReadActionID({
API.Report_UpdateLastRead({
accountID: currentUserAccountID,
reportID,
sequenceNumber,
Expand Down
1 change: 1 addition & 0 deletions src/pages/home/report/ReportActionsView.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ class ReportActionsView extends React.Component {
});
if (this.props.isActiveReport) {
this.keyboardEvent = Keyboard.addListener('keyboardDidShow', this.scrollToListBottom);
this.recordMaxAction();
Comment thread
chiragsalian marked this conversation as resolved.
}

fetchActions(this.props.reportID);
Expand Down
5 changes: 3 additions & 2 deletions src/pages/home/sidebar/ChatLinkRow.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import ChatSwitcherOptionPropTypes from './ChatSwitcherOptionPropTypes';
import ROUTES from '../../../ROUTES';
import pencilIcon from '../../../../assets/images/icon-pencil.png';
import PressableLink from '../../../components/PressableLink';
import CONST from '../../../CONST';

const propTypes = {
// Option to allow the user to choose from can be type 'report' or 'user'
Expand Down Expand Up @@ -43,7 +44,7 @@ const ChatLinkRow = ({
onAddToGroup,
isChatSwitcher,
}) => {
const isUserRow = option.type === 'user';
const isSingleUserDM = option.type === CONST.REPORT.SINGLE_USER_DM;
const textStyle = optionIsFocused
? styles.sidebarLinkActiveText
: styles.sidebarLinkText;
Expand Down Expand Up @@ -108,7 +109,7 @@ const ChatLinkRow = ({
</View>
</View>
</PressableLink>
{isUserRow && isChatSwitcher && (
{isSingleUserDM && isChatSwitcher && (
<View>
<TouchableOpacity
style={[styles.chatSwitcherItemButton]}
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/sidebar/ChatSwitcherList.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const ChatSwitcherList = ({
keyboardShouldPersistTaps="always"
showsVerticalScrollIndicator={false}
data={options}
keyExtractor={option => (option.type === 'user' ? option.alternateText : String(option.reportID))}
keyExtractor={option => option.keyForList}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NAB key is probably fine as we will know what it's for based on how it gets used

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

went with the suggestion mentioned here

renderItem={({item, index}) => (
<ChatLinkRow
option={item}
Expand Down
3 changes: 3 additions & 0 deletions src/pages/home/sidebar/ChatSwitcherOptionPropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,7 @@ export default PropTypes.shape({

// The type of option we have e.g. user or report
type: PropTypes.string,

// The option key provided to FlatList keyExtractor
keyForList: PropTypes.string,
});
Loading