Skip to content

[CP Staging] Fix regression #23189 #23240

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
mountiny merged 11 commits into from
Jul 20, 2023
Merged

[CP Staging] Fix regression #23189 #23240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5521d4b
wip: add policy as dependency to function
hannojg Jul 20, 2023
77801b4
revert changes on OptionsListUtils
hannojg Jul 20, 2023
41051a4
pass param correctly
hannojg Jul 20, 2023
66d74ae
add comment
hannojg Jul 20, 2023
2b6f5cd
Update src/libs/SidebarUtils.js
hannojg Jul 20, 2023
fd7ff20
remove from getReportPreviewMessage
hannojg Jul 20, 2023
2a5d4e2
Merge branch 'fix/23189-regression-of-21022' of github.com:margelo/ex…
hannojg Jul 20, 2023
44ccbd6
remove from getRootReportAndWorkspaceName
hannojg Jul 20, 2023
9cd441b
fix priority mode warning
hannojg Jul 20, 2023
c6f8e6e
Merge branch 'main' of github.com:Expensify/App into fix/23189-regres…
hannojg Jul 20, 2023
ad3ce9b
fixed wrong prop type
hannojg Jul 20, 2023
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
24 changes: 21 additions & 3 deletions src/components/LHNOptionsList/OptionRowLHNData.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,18 @@ const propTypes = {
// eslint-disable-next-line react/forbid-prop-types
fullReport: PropTypes.object,

/** The policies which the user has access to and which the report could be tied to */
policies: PropTypes.objectOf(
PropTypes.shape({
/** The ID of the policy */
id: PropTypes.string,
/** Name of the policy */
name: PropTypes.string,
/** Avatar of the policy */
avatar: PropTypes.string,
}),
),

...withCurrentReportIDPropTypes,
Comment thread
mountiny marked this conversation as resolved.
...basePropTypes,
};
Expand All @@ -37,6 +49,7 @@ const defaultProps = {
shouldDisableFocusOptions: false,
personalDetails: {},
fullReport: {},
policies: {},
preferredLocale: CONST.LOCALES.DEFAULT,
...withCurrentReportIDDefaultProps,
...baseDefaultProps,
Expand All @@ -48,22 +61,24 @@ const defaultProps = {
* The OptionRowLHN component is memoized, so it will only
* re-render if the data really changed.
*/
function OptionRowLHNData({shouldDisableFocusOptions, currentReportID, fullReport, personalDetails, preferredLocale, comment, ...propsToForward}) {
function OptionRowLHNData({shouldDisableFocusOptions, currentReportID, fullReport, personalDetails, preferredLocale, comment, policies, ...propsToForward}) {
const reportID = propsToForward.reportID;
// We only want to pass a boolean to the memoized component,
// instead of a changing number (so we prevent unnecessary re-renders).
const isFocused = !shouldDisableFocusOptions && currentReportID === reportID;

const policy = lodashGet(policies, [`${ONYXKEYS.COLLECTION.POLICY}${fullReport.policyID}`], '');

const optionItemRef = useRef();
const optionItem = useMemo(() => {
// Note: ideally we'd have this as a dependent selector in onyx!
const item = SidebarUtils.getOptionData(fullReport, personalDetails, preferredLocale);
const item = SidebarUtils.getOptionData(fullReport, personalDetails, preferredLocale, policy);
if (deepEqual(item, optionItemRef.current)) {
return optionItemRef.current;
}
optionItemRef.current = item;
return item;
}, [fullReport, preferredLocale, personalDetails]);
}, [fullReport, personalDetails, preferredLocale, policy]);

useEffect(() => {
if (!optionItem || optionItem.hasDraftComment || !comment || comment.length <= 0 || isFocused) {
Expand Down Expand Up @@ -137,6 +152,9 @@ export default React.memo(
preferredLocale: {
key: ONYXKEYS.NVP_PREFERRED_LOCALE,
},
policies: {
key: ONYXKEYS.COLLECTION.POLICY,
},
}),
)(OptionRowLHNData),
);
48 changes: 27 additions & 21 deletions src/libs/ReportUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,9 +505,10 @@ function isArchivedRoom(report) {
* @param {String} report.oldPolicyName
* @param {String} report.policyName
* @param {Boolean} [returnEmptyIfNotFound]
* @param {Object} [policy]
* @returns {String}
*/
function getPolicyName(report, returnEmptyIfNotFound = false) {
function getPolicyName(report, returnEmptyIfNotFound = false, policy = undefined) {
const noPolicyFound = returnEmptyIfNotFound ? '' : Localize.translateLocal('workspace.common.unavailable');
if (_.isEmpty(report)) {
return noPolicyFound;
Expand All @@ -516,12 +517,12 @@ function getPolicyName(report, returnEmptyIfNotFound = false) {
if ((!allPolicies || _.size(allPolicies) === 0) && !report.policyName) {
return Localize.translateLocal('workspace.common.unavailable');
}
const policy = _.get(allPolicies, `${ONYXKEYS.COLLECTION.POLICY}${report.policyID}`);
const finalPolicy = policy || _.get(allPolicies, `${ONYXKEYS.COLLECTION.POLICY}${report.policyID}`);

// // Public rooms send back the policy name with the reportSummary,
// // since they can also be accessed by people who aren't in the workspace
// Public rooms send back the policy name with the reportSummary,
// since they can also be accessed by people who aren't in the workspace

return lodashGet(policy, 'name') || report.policyName || report.oldPolicyName || noPolicyFound;
return lodashGet(finalPolicy, 'name') || report.policyName || report.oldPolicyName || noPolicyFound;
}

/**
Expand Down Expand Up @@ -785,10 +786,11 @@ function getIconsForParticipants(participants, personalDetails) {
* Given a report, return the associated workspace icon.
*
* @param {Object} report
* @param {Object} [policy]
* @returns {Object}
*/
function getWorkspaceIcon(report) {
const workspaceName = getPolicyName(report);
function getWorkspaceIcon(report, policy = undefined) {

@dummy-1111 dummy-1111 Jul 20, 2023

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.

I think policy = undefined has no meaning. We can safely remove the default value because default value is undefined in JS

@hannojg hannojg Jul 20, 2023

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.

while its true, we found in other places that its more expressive. It conveys the meaning, that you don't have to set policy.
See similar cases here: https://github.com/Expensify/App/pull/23206/files#r1268753442

App/.github/libs/ActionUtils.js

Line 28 in 450573e

function getStringInput(name, options, defaultValue = undefined) {

const workspaceName = getPolicyName(report, false, policy);
const policyExpenseChatAvatarSource = lodashGet(allPolicies, [`${ONYXKEYS.COLLECTION.POLICY}${report.policyID}`, 'avatar']) || getDefaultWorkspaceAvatar(workspaceName);
const workspaceIcon = {
source: policyExpenseChatAvatarSource,
Expand All @@ -809,9 +811,10 @@ function getWorkspaceIcon(report) {
* @param {Boolean} [isPayer]
* @param {String} [defaultName]
* @param {Number} [defaultAccountID]
* @param {Object} [policy]
* @returns {Array<*>}
*/
function getIcons(report, personalDetails, defaultIcon = null, isPayer = false, defaultName = '', defaultAccountID = -1) {
function getIcons(report, personalDetails, defaultIcon = null, isPayer = false, defaultName = '', defaultAccountID = -1, policy = undefined) {
if (_.isEmpty(report)) {
const fallbackIcon = {
source: defaultIcon || Expensicons.FallbackAvatar,
Expand All @@ -823,7 +826,7 @@ function getIcons(report, personalDetails, defaultIcon = null, isPayer = false,
}
if (isExpenseRequest(report)) {
const parentReportAction = ReportActionsUtils.getParentReportAction(report);
const workspaceIcon = getWorkspaceIcon(report);
const workspaceIcon = getWorkspaceIcon(report, policy);
const memberIcon = {
source: UserUtils.getAvatar(lodashGet(personalDetails, [parentReportAction.actorAccountID, 'avatar']), parentReportAction.actorAccountID),
id: parentReportAction.actorAccountID,
Expand All @@ -846,7 +849,7 @@ function getIcons(report, personalDetails, defaultIcon = null, isPayer = false,
};

if (isWorkspaceThread(report)) {
const workspaceIcon = getWorkspaceIcon(report);
const workspaceIcon = getWorkspaceIcon(report, policy);
return [actorIcon, workspaceIcon];
}
return [actorIcon];
Expand All @@ -860,7 +863,7 @@ function getIcons(report, personalDetails, defaultIcon = null, isPayer = false,
};

if (isWorkspaceTaskReport(report)) {
const workspaceIcon = getWorkspaceIcon(report);
const workspaceIcon = getWorkspaceIcon(report, policy);
return [ownerIcon, workspaceIcon];
}

Expand All @@ -879,11 +882,11 @@ function getIcons(report, personalDetails, defaultIcon = null, isPayer = false,
return [domainIcon];
}
if (isAdminRoom(report) || isAnnounceRoom(report) || isChatRoom(report) || isArchivedRoom(report)) {
const workspaceIcon = getWorkspaceIcon(report);
const workspaceIcon = getWorkspaceIcon(report, policy);
return [workspaceIcon];
}
if (isPolicyExpenseChat(report) || isExpenseReport(report)) {
const workspaceIcon = getWorkspaceIcon(report);
const workspaceIcon = getWorkspaceIcon(report, policy);
const memberIcon = {
source: UserUtils.getAvatar(lodashGet(personalDetails, [report.ownerAccountID, 'avatar']), report.ownerAccountID),
id: report.ownerAccountID,
Expand Down Expand Up @@ -1032,14 +1035,15 @@ function getMoneyRequestTotal(report, moneyRequestReports = {}) {
* Get the title for a policy expense chat which depends on the role of the policy member seeing this report
*
* @param {Object} report
* @param {Object} [policy]
* @returns {String}
*/
function getPolicyExpenseChatName(report) {
function getPolicyExpenseChatName(report, policy = undefined) {
const reportOwnerDisplayName = getDisplayNameForParticipant(report.ownerAccountID) || lodashGet(allPersonalDetails, [report.ownerAccountID, 'login']) || report.reportName;

// If the policy expense chat is owned by this user, use the name of the policy as the report name.
if (report.isOwnPolicyExpenseChat) {
return getPolicyName(report);
return getPolicyName(report, false, policy);
}

const policyExpenseChatRole = lodashGet(allPolicies, [`${ONYXKEYS.COLLECTION.POLICY}${report.policyID}`, 'role']) || 'user';
Expand All @@ -1050,7 +1054,7 @@ function getPolicyExpenseChatName(report) {
const lastAction = ReportActionsUtils.getLastVisibleAction(report.reportID);
const archiveReason = (lastAction && lastAction.originalMessage && lastAction.originalMessage.reason) || CONST.REPORT.ARCHIVE_REASON.DEFAULT;
if (archiveReason === CONST.REPORT.ARCHIVE_REASON.ACCOUNT_MERGED && policyExpenseChatRole !== CONST.POLICY.ROLE.ADMIN) {
return getPolicyName(report);
return getPolicyName(report, false, policy);
}
}

Expand All @@ -1062,11 +1066,12 @@ function getPolicyExpenseChatName(report) {
* Get the title for a IOU or expense chat which will be showing the payer and the amount
*
* @param {Object} report
* @param {Object} [policy]
* @returns {String}
*/
function getMoneyRequestReportName(report) {
function getMoneyRequestReportName(report, policy = undefined) {
const formattedAmount = CurrencyUtils.convertToDisplayString(getMoneyRequestTotal(report), report.currency);
const payerName = isExpenseReport(report) ? getPolicyName(report) : getDisplayNameForParticipant(report.managerID);
const payerName = isExpenseReport(report) ? getPolicyName(report, false, policy) : getDisplayNameForParticipant(report.managerID);

return Localize.translateLocal(report.hasOutstandingIOU ? 'iou.payerOwesAmount' : 'iou.payerPaidAmount', {payer: payerName, amount: formattedAmount});
}
Expand Down Expand Up @@ -1121,9 +1126,10 @@ function getReportPreviewMessage(report, reportAction = {}) {
* Get the title for a report.
*
* @param {Object} report
* @param {Object} [policy]
* @returns {String}
*/
function getReportName(report) {
function getReportName(report, policy = undefined) {
let formattedName;
if (isChatThread(report)) {
const parentReportAction = ReportActionsUtils.getParentReportAction(report);
Expand All @@ -1149,11 +1155,11 @@ function getReportName(report) {
}

if (isPolicyExpenseChat(report)) {
formattedName = getPolicyExpenseChatName(report);
formattedName = getPolicyExpenseChatName(report, policy);
}

if (isMoneyRequestReport(report)) {
formattedName = getMoneyRequestReportName(report);
formattedName = getMoneyRequestReportName(report, policy);
}

if (isArchivedRoom(report)) {
Expand Down
9 changes: 5 additions & 4 deletions src/libs/SidebarUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,9 +200,10 @@ function getOrderedReportIDs(currentReportId, allReportsDict, betas, policies, p
* @param {Object} report
* @param {Object} personalDetails
* @param {String} preferredLocale
* @param {Object} [policy]
* @returns {Object}
*/
function getOptionData(report, personalDetails, preferredLocale) {
function getOptionData(report, personalDetails, preferredLocale, policy) {
// When a user signs out, Onyx is cleared. Due to the lazy rendering with a virtual list, it's possible for
// this method to be called after the Onyx data has been cleared out. In that case, it's fine to do
// a null check here and return early.
Expand Down Expand Up @@ -306,7 +307,7 @@ function getOptionData(report, personalDetails, preferredLocale) {
CONST.REPORT.ARCHIVE_REASON.DEFAULT;
lastMessageText = Localize.translate(preferredLocale, `reportArchiveReasons.${archiveReason}`, {
displayName: archiveReason.displayName || PersonalDetailsUtils.getDisplayNameOrDefault(lastActorDetails, 'displayName'),
policyName: ReportUtils.getPolicyName(report),
policyName: ReportUtils.getPolicyName(report, false, policy),
});
}

Expand Down Expand Up @@ -356,13 +357,13 @@ function getOptionData(report, personalDetails, preferredLocale) {
result.payPalMeAddress = personalDetail.payPalMeAddress;
}

const reportName = ReportUtils.getReportName(report);
const reportName = ReportUtils.getReportName(report, policy);

result.text = reportName;
result.subtitle = subtitle;
result.participantsList = participantPersonalDetailList;

result.icons = ReportUtils.getIcons(report, personalDetails, UserUtils.getAvatar(personalDetail.avatar, personalDetail.accountID), true);
result.icons = ReportUtils.getIcons(report, personalDetails, UserUtils.getAvatar(personalDetail.avatar, personalDetail.accountID), true, '', -1, policy);
result.searchText = OptionsListUtils.getSearchText(report, reportName, participantPersonalDetailList, result.isChatRoom || result.isPolicyExpenseChat, result.isThread);
result.displayNamesWithTooltips = displayNamesWithTooltips;
result.isLastMessageDeletedParentAction = report.isLastMessageDeletedParentAction;
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/sidebar/SidebarLinks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ const propTypes = {

currentUserPersonalDetails: personalDetailsPropType,

priorityMode: PropTypes.oneOf(_.values(CONST.OPTION_MODE)),
priorityMode: PropTypes.oneOf(_.values(CONST.PRIORITY_MODE)),

@mountiny mountiny Jul 20, 2023

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.

Why is this changed?

@fedirjh fedirjh Jul 20, 2023

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.

It was causing a warning, please check #23240 (comment)

@hannojg hannojg Jul 20, 2023 •
edited
Loading

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.

well, the onyx value we read is the priority mode, it has been wrong in the first place to check for OPTION_MODE, which is not what we are working with (we changed it to PRIORITY_MODE)

@mountiny mountiny Jul 20, 2023

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.

Thanks!


...withLocalizePropTypes,
};
Expand Down