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

Fixed LHN Ordering #35907

Merged
merged 26 commits into from
Mar 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
9b040c5
Test LHN fix
shubham1206agra Feb 6, 2024
6333dcd
revert unintentional change
shubham1206agra Feb 6, 2024
87826ab
Merge branch 'main' into test-lhn-fix
shubham1206agra Feb 17, 2024
06f5775
Optimization attempt 1
shubham1206agra Feb 17, 2024
f556253
Fixed perf test
shubham1206agra Feb 17, 2024
f8ab7cc
Fixed display name filter
shubham1206agra Feb 17, 2024
90343b9
Added violation for RBR in sorting
shubham1206agra Feb 18, 2024
6b19fd2
Optimization attempt 2
shubham1206agra Feb 19, 2024
c61adb1
Revert "Optimization attempt 2"
shubham1206agra Feb 19, 2024
cf3147f
Merge branch 'Expensify:main' into test-lhn-fix
shubham1206agra Feb 19, 2024
6649bb9
Minor fixes
shubham1206agra Feb 19, 2024
40e7a05
Merge branch 'main' into test-lhn-fix
shubham1206agra Feb 21, 2024
89a56b2
Optimization Attempt 3
shubham1206agra Feb 21, 2024
102bbda
Fixed perf test
shubham1206agra Feb 21, 2024
1eca3e1
code adjustment according to recommendations
shubham1206agra Feb 21, 2024
1314854
Merge branch 'main' into test-lhn-fix
shubham1206agra Feb 24, 2024
4e35908
Fixed lint
shubham1206agra Feb 24, 2024
e15075f
Merge branch 'Expensify:main' into test-lhn-fix
shubham1206agra Feb 27, 2024
651e372
Revert "Merge branch 'Expensify:main' into test-lhn-fix"
shubham1206agra Feb 27, 2024
70ef3fc
fixing name display
shubham1206agra Feb 27, 2024
91cea78
Revert "Revert "Merge branch 'Expensify:main' into test-lhn-fix""
shubham1206agra Feb 27, 2024
e2f769a
adding small comment
shubham1206agra Feb 27, 2024
8495e60
Rename variable
shubham1206agra Feb 28, 2024
29216c2
Fixed violation to be used properly for all permissions users
shubham1206agra Feb 28, 2024
888195b
Merge branch 'Expensify:main' into test-lhn-fix
shubham1206agra Mar 4, 2024
f14090e
Added transaction as dependency
shubham1206agra Mar 4, 2024
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: 2 additions & 0 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1472,6 +1472,8 @@ const CONST = {
ALPHABETIC_AND_LATIN_CHARS: /^[\p{Script=Latin} ]*$/u,
NON_ALPHABETIC_AND_NON_LATIN_CHARS: /[^\p{Script=Latin}]/gu,
ACCENT_LATIN_CHARS: /[\u00C0-\u017F]/g,
INVALID_DISPLAY_NAME_LHN: /[^\p{L}\p{N}\u00C0-\u017F\s-]/gu,
INVALID_DISPLAY_NAME_ONLY_LHN: /^[^\p{L}\p{N}\u00C0-\u017F]$/gu,
POSITIVE_INTEGER: /^\d+$/,
PO_BOX: /\b[P|p]?(OST|ost)?\.?\s*[O|o|0]?(ffice|FFICE)?\.?\s*[B|b][O|o|0]?[X|x]?\.?\s+[#]?(\d+)\b/,
ANY_VALUE: /^.+$/,
Expand Down
4 changes: 4 additions & 0 deletions src/components/LHNOptionsList/LHNOptionsList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ function LHNOptionsList({
draftComments = {},
transactionViolations = {},
onFirstItemRendered = () => {},
reportIDsWithErrors = {},
}: LHNOptionsListProps) {
const styles = useThemeStyles();
const {canUseViolations} = usePermissions();
Expand Down Expand Up @@ -63,6 +64,7 @@ function LHNOptionsList({
const itemComment = draftComments?.[`${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`] ?? '';
const sortedReportActions = ReportActionsUtils.getSortedReportActionsForDisplay(itemReportActions);
const lastReportAction = sortedReportActions[0];
const reportErrors = reportIDsWithErrors[reportID] ?? {};

// Get the transaction for the last report action
let lastReportActionTransactionID = '';
Expand Down Expand Up @@ -91,6 +93,7 @@ function LHNOptionsList({
transactionViolations={transactionViolations}
canUseViolations={canUseViolations}
onLayout={onLayoutItem}
reportErrors={reportErrors}
/>
);
},
Expand All @@ -109,6 +112,7 @@ function LHNOptionsList({
transactionViolations,
canUseViolations,
onLayoutItem,
reportIDsWithErrors,
],
);

Expand Down
4 changes: 3 additions & 1 deletion src/components/LHNOptionsList/OptionRowLHNData.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ function OptionRowLHNData({
lastReportActionTransaction = {},
transactionViolations,
canUseViolations,
reportErrors,
...propsToForward
}: OptionRowLHNDataProps) {
const reportID = propsToForward.reportID;
Expand All @@ -40,11 +41,11 @@ function OptionRowLHNData({
// Note: ideally we'd have this as a dependent selector in onyx!
const item = SidebarUtils.getOptionData({
report: fullReport,
reportActions,
personalDetails,
preferredLocale: preferredLocale ?? CONST.LOCALES.DEFAULT,
policy,
parentReportAction,
reportErrors,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not added to the dependencies of useMemo

Copy link
Contributor

Choose a reason for hiding this comment

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

reportErrors seems to be an object, if we add it as dependency we should memoize it also on the parent component

parent:

const reportErrors = useMemo(() => reportIDsWithErrors[reportID] ?? {}, [reportID]);

and not sure about reportIDsWithErrors
cc: @youssef-lr

hasViolations: !!hasViolations,
});
if (deepEqual(item, optionItemRef.current)) {
Expand All @@ -69,6 +70,7 @@ function OptionRowLHNData({
transactionViolations,
canUseViolations,
receiptTransactions,
reportErrors,
]);

useEffect(() => {
Expand Down
7 changes: 7 additions & 0 deletions src/components/LHNOptionsList/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import type {CurrentReportIDContextValue} from '@components/withCurrentReportID'
import type CONST from '@src/CONST';
import type {OptionData} from '@src/libs/ReportUtils';
import type {Locale, PersonalDetailsList, Policy, Report, ReportAction, ReportActions, Transaction, TransactionViolation} from '@src/types/onyx';
import type * as OnyxCommon from '@src/types/onyx/OnyxCommon';
import type {EmptyObject} from '@src/types/utils/EmptyObject';

type OptionMode = ValueOf<typeof CONST.OPTION_MODE>;
Expand Down Expand Up @@ -58,6 +59,9 @@ type CustomLHNOptionsListProps = {

/** Callback to fire when the list is laid out */
onFirstItemRendered: () => void;

/** Report IDs with errors mapping to their corresponding error objects */
reportIDsWithErrors: Record<string, OnyxCommon.Errors>;
};

type LHNOptionsListProps = CustomLHNOptionsListProps & CurrentReportIDContextValue & LHNOptionsListOnyxProps;
Expand Down Expand Up @@ -113,6 +117,9 @@ type OptionRowLHNDataProps = {

/** Callback to execute when the OptionList lays out */
onLayout?: (event: LayoutChangeEvent) => void;

/** The report errors */
reportErrors: OnyxCommon.Errors | undefined;
};

type OptionRowLHNProps = {
Expand Down
4 changes: 2 additions & 2 deletions src/libs/OptionsListUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ function getSearchText(
/**
* Get an object of error messages keyed by microtime by combining all error objects related to the report.
*/
function getAllReportErrors(report: OnyxEntry<Report>, reportActions: OnyxEntry<ReportActions>): OnyxCommon.Errors {
function getAllReportErrors(report: OnyxEntry<Report>, reportActions: OnyxEntry<ReportActions>, transactions: OnyxCollection<Transaction> = allTransactions): OnyxCommon.Errors {
const reportErrors = report?.errors ?? {};
const reportErrorFields = report?.errorFields ?? {};
const reportActionErrors: OnyxCommon.ErrorFields = Object.values(reportActions ?? {}).reduce(
Expand All @@ -493,7 +493,7 @@ function getAllReportErrors(report: OnyxEntry<Report>, reportActions: OnyxEntry<

if (parentReportAction?.actorAccountID === currentUserAccountID && ReportActionUtils.isTransactionThread(parentReportAction)) {
const transactionID = parentReportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU ? parentReportAction?.originalMessage?.IOUTransactionID : null;
const transaction = allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`];
const transaction = transactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`];
if (TransactionUtils.hasMissingSmartscanFields(transaction ?? null) && !ReportUtils.isSettled(transaction?.reportID)) {
reportActionErrors.smartscan = ErrorUtils.getMicroSecondOnyxError('report.genericSmartscanFailureMessage');
}
Expand Down
45 changes: 30 additions & 15 deletions src/libs/SidebarUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {PersonalDetails, PersonalDetailsList, TransactionViolation} from '@src/types/onyx';
import type Beta from '@src/types/onyx/Beta';
import type * as OnyxCommon from '@src/types/onyx/OnyxCommon';
import type Policy from '@src/types/onyx/Policy';
import type Report from '@src/types/onyx/Report';
import type {ReportActions} from '@src/types/onyx/ReportAction';
Expand Down Expand Up @@ -59,6 +60,13 @@ function compareStringDates(a: string, b: string): 0 | 1 | -1 {
return 0;
}

function filterDisplayName(displayName: string): string {
if (CONST.REGEX.INVALID_DISPLAY_NAME_ONLY_LHN.test(displayName)) {
return displayName;
}
return displayName.replace(CONST.REGEX.INVALID_DISPLAY_NAME_LHN, '').trim();
}

/**
* @returns An array of reportIDs sorted in the proper order
*/
Expand All @@ -68,22 +76,27 @@ function getOrderedReportIDs(
betas: Beta[],
policies: Record<string, Policy>,
priorityMode: ValueOf<typeof CONST.PRIORITY_MODE>,
allReportActions: OnyxCollection<ReportAction[]>,
allReportActions: OnyxCollection<ReportActions>,
transactionViolations: OnyxCollection<TransactionViolation[]>,
currentPolicyID = '',
policyMemberAccountIDs: number[] = [],
reportIDsWithErrors: Record<string, OnyxCommon.Errors> = {},
canUseViolations = false,
): string[] {
const isInGSDMode = priorityMode === CONST.PRIORITY_MODE.GSD;
const isInDefaultMode = !isInGSDMode;
const allReportsDictValues = Object.values(allReports);

const reportIDsWithViolations = new Set<string>();
Copy link
Contributor

@fedirjh fedirjh Feb 28, 2024

Choose a reason for hiding this comment

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

Couldn't this be moved to SidebarLinksData.js similar to reportIDsWithErrors ? I think this will give some perf boost?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. It didn't give any.


// Filter out all the reports that shouldn't be displayed
let reportsToDisplay = allReportsDictValues.filter((report) => {
const parentReportActionsKey = `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report?.parentReportID}`;
const parentReportActions = allReportActions?.[parentReportActionsKey];
const parentReportAction = parentReportActions?.find((action) => action && report && action?.reportActionID === report?.parentReportActionID);
const doesReportHaveViolations =
betas.includes(CONST.BETAS.VIOLATIONS) && !!parentReportAction && ReportUtils.doesTransactionThreadHaveViolations(report, transactionViolations, parentReportAction);
const parentReportAction = allReportActions?.[parentReportActionsKey]?.[report.parentReportActionID ?? ''];
const doesReportHaveViolations = canUseViolations && !!parentReportAction && ReportUtils.doesTransactionThreadHaveViolations(report, transactionViolations, parentReportAction);
if (doesReportHaveViolations) {
reportIDsWithViolations.add(report.reportID);
}
return ReportUtils.shouldReportBeInOptionList({
report,
currentReportId: currentReportId ?? '',
Expand All @@ -105,15 +118,15 @@ function getOrderedReportIDs(
}

// The LHN is split into four distinct groups, and each group is sorted a little differently. The groups will ALWAYS be in this order:
// 1. Pinned/GBR - Always sorted by reportDisplayName
// 1. Pinned/GBR/RBR - Always sorted by reportDisplayName
// 2. Drafts - Always sorted by reportDisplayName
// 3. Non-archived reports and settled IOUs
// - Sorted by lastVisibleActionCreated in default (most recent) view mode
// - Sorted by reportDisplayName in GSD (focus) view mode
// 4. Archived reports
// - Sorted by lastVisibleActionCreated in default (most recent) view mode
// - Sorted by reportDisplayName in GSD (focus) view mode
const pinnedAndGBRReports: Report[] = [];
const pinnedAndBrickRoadReports: Report[] = [];
const draftReports: Report[] = [];
const nonArchivedReports: Report[] = [];
const archivedReports: Report[] = [];
Expand All @@ -127,12 +140,14 @@ function getOrderedReportIDs(
// However, this code needs to be very performant to handle thousands of reports, so in the interest of speed, we're just going to disable this lint rule and add
// the reportDisplayName property to the report object directly.
// eslint-disable-next-line no-param-reassign
report.displayName = ReportUtils.getReportName(report);
report.displayName = filterDisplayName(ReportUtils.getReportName(report));

shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved
const hasRBR = report.reportID in reportIDsWithErrors || reportIDsWithViolations.has(report.reportID);

const isPinned = report.isPinned ?? false;
const reportAction = ReportActionsUtils.getReportAction(report.parentReportID ?? '', report.parentReportActionID ?? '');
if (isPinned || ReportUtils.requiresAttentionFromCurrentUser(report, reportAction)) {
pinnedAndGBRReports.push(report);
if (isPinned || hasRBR || ReportUtils.requiresAttentionFromCurrentUser(report, reportAction)) {
pinnedAndBrickRoadReports.push(report);
} else if (report.hasDraft) {
draftReports.push(report);
} else if (ReportUtils.isArchivedRoom(report)) {
Expand All @@ -143,7 +158,7 @@ function getOrderedReportIDs(
});

// Sort each group of reports accordingly
pinnedAndGBRReports.sort((a, b) => (a?.displayName && b?.displayName ? localeCompare(a.displayName, b.displayName) : 0));
pinnedAndBrickRoadReports.sort((a, b) => (a?.displayName && b?.displayName ? localeCompare(a.displayName, b.displayName) : 0));
draftReports.sort((a, b) => (a?.displayName && b?.displayName ? localeCompare(a.displayName, b.displayName) : 0));

if (isInDefaultMode) {
Expand All @@ -161,7 +176,7 @@ function getOrderedReportIDs(

// Now that we have all the reports grouped and sorted, they must be flattened into an array and only return the reportID.
// The order the arrays are concatenated in matters and will determine the order that the groups are displayed in the sidebar.
const LHNReports = [...pinnedAndGBRReports, ...draftReports, ...nonArchivedReports, ...archivedReports].map((report) => report.reportID);
const LHNReports = [...pinnedAndBrickRoadReports, ...draftReports, ...nonArchivedReports, ...archivedReports].map((report) => report.reportID);
return LHNReports;
}

Expand All @@ -170,19 +185,19 @@ function getOrderedReportIDs(
*/
function getOptionData({
report,
reportActions,
personalDetails,
preferredLocale,
policy,
parentReportAction,
reportErrors,
hasViolations,
}: {
report: OnyxEntry<Report>;
reportActions: OnyxEntry<ReportActions>;
personalDetails: OnyxEntry<PersonalDetailsList>;
preferredLocale: DeepValueOf<typeof CONST.LOCALES>;
policy: OnyxEntry<Policy> | undefined;
parentReportAction: OnyxEntry<ReportAction> | undefined;
reportErrors: OnyxCommon.Errors | undefined;
hasViolations: boolean;
}): ReportUtils.OptionData | undefined {
// When a user signs out, Onyx is cleared. Due to the lazy rendering with a virtual list, it's possible for
Expand All @@ -195,7 +210,7 @@ function getOptionData({
const result: ReportUtils.OptionData = {
text: '',
alternateText: null,
allReportErrors: OptionsListUtils.getAllReportErrors(report, reportActions),
allReportErrors: reportErrors,
brickRoadIndicator: null,
tooltipText: null,
subtitle: null,
Expand Down
3 changes: 2 additions & 1 deletion src/pages/home/sidebar/SidebarLinks.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ const propTypes = {
isActiveReport: PropTypes.func.isRequired,
};

function SidebarLinks({onLinkClick, insets, optionListItems, isLoading, priorityMode = CONST.PRIORITY_MODE.DEFAULT, isActiveReport, isCreateMenuOpen, activePolicy}) {
function SidebarLinks({onLinkClick, insets, optionListItems, isLoading, priorityMode = CONST.PRIORITY_MODE.DEFAULT, isActiveReport, isCreateMenuOpen, activePolicy, reportIDsWithErrors}) {
const styles = useThemeStyles();
const StyleUtils = useStyleUtils();
const modal = useRef({});
Expand Down Expand Up @@ -154,6 +154,7 @@ function SidebarLinks({onLinkClick, insets, optionListItems, isLoading, priority
shouldDisableFocusOptions={isSmallScreenWidth}
optionMode={viewMode}
onFirstItemRendered={App.setSidebarLoaded}
reportIDsWithErrors={reportIDsWithErrors}
/>
{isLoading && optionListItems.length === 0 && (
<View style={[StyleSheet.absoluteFillObject, styles.appBG]}>
Expand Down
Loading
Loading