Skip to content

Fix: Add red dot to one transaction reports #44810

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
robertjchen merged 11 commits into from
Jul 11, 2024
Merged

Fix: Add red dot to one transaction reports #44810

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
7da324b
Fix: Add red dot to one transaction reports
truph01 Jul 4, 2024
7ab06d9
merge main
truph01 Jul 8, 2024
39f8fed
Fix: Lint
truph01 Jul 8, 2024
1bef82d
Fix: Typescript
truph01 Jul 8, 2024
805f6ae
Fix: Remove allReports
truph01 Jul 8, 2024
20f24c7
Fix: Conflict
truph01 Jul 8, 2024
d8e6c25
Fix: Typecheck
truph01 Jul 8, 2024
4e1d2de
Fix: Merge main
truph01 Jul 10, 2024
9c8ef36
Fix: Create getReport function in ReportUtils
truph01 Jul 10, 2024
9392dc9
Fix: Lint
truph01 Jul 10, 2024
c35d1e5
Fix: Resolve conflict
truph01 Jul 11, 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
1 change: 1 addition & 0 deletions src/components/LHNOptionsList/OptionRowLHNData.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ function OptionRowLHNData({
policy,
parentReportAction,
hasViolations: !!shouldDisplayViolations,
transactionViolations,
});
if (deepEqual(item, optionItemRef.current)) {
return optionItemRef.current;
Expand Down
8 changes: 8 additions & 0 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,6 +608,13 @@ function isDraftReport(reportID: string | undefined): boolean {
return !!draftReport;
}

/**
* Returns the report
*/
function getReport(reportID: string): OnyxEntry<Report> {
return ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`];
}

Copy link
Copy Markdown
Contributor

@bernhardoj bernhardoj Jul 16, 2024

Choose a reason for hiding this comment

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

FYI, I don't think we should be exporting getReport anymore. It was previously removed in #43632. They added a test rule to prevent the export, but they got the name wrong.

App/tests/actions/EnforceActionExportRestrictions.ts

Lines 32 to 35 in 7d58aa6

it('does not export getReport', () => {
// @ts-expect-error the test is asserting that it's undefined, so the TS error is normal
expect(ReportUtils.getReportOrDraftReport).toBeUndefined();
});

Copy link
Copy Markdown
Contributor Author

@truph01 truph01 Jul 16, 2024

Choose a reason for hiding this comment

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

@bernhardoj Thanks. I will note it in the future PR.

Copy link
Copy Markdown
Contributor

@Pujan92 Pujan92 Jul 16, 2024

Choose a reason for hiding this comment

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

Thanks @bernhardoj.
To contrary, ReportConnection.ts is created to keep track of all the reports instead of subscribing it from all the components for the performance improvement in #44068. Maybe we can move getReport there, though it is the same.
Thoughts @robertjchen ?

/**
* Returns the parentReport if the given report is a thread
*/
Expand Down Expand Up @@ -7375,6 +7382,7 @@ export {
findPolicyExpenseChatByPolicyID,
hasOnlyNonReimbursableTransactions,
getMostRecentlyVisitedReport,
getReport,
};

export type {
Expand Down
35 changes: 34 additions & 1 deletion src/libs/SidebarUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,18 @@ import localeCompare from './LocaleCompare';
import * as LocalePhoneNumber from './LocalePhoneNumber';
import * as Localize from './Localize';
import * as OptionsListUtils from './OptionsListUtils';
import Permissions from './Permissions';
import * as PolicyUtils from './PolicyUtils';
import * as ReportActionsUtils from './ReportActionsUtils';
import * as ReportUtils from './ReportUtils';
import * as TaskUtils from './TaskUtils';

let allBetas: OnyxEntry<Beta[]>;
Onyx.connect({
key: ONYXKEYS.BETAS,
callback: (value) => (allBetas = value),
});

const visibleReportActionItems: ReportActions = {};
Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
Expand Down Expand Up @@ -94,8 +101,17 @@ function getOrderedReportIDs(
const isHidden = report.notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;
const isFocused = report.reportID === currentReportId;
const allReportErrors = OptionsListUtils.getAllReportErrors(report, reportActions) ?? {};
const transactionReportActions = ReportActionsUtils.getAllReportActions(report.reportID);
const oneTransactionThreadReportID = ReportActionsUtils.getOneTransactionThreadReportID(report.reportID, transactionReportActions, undefined);
let doesTransactionThreadReportHasViolations = false;
if (oneTransactionThreadReportID) {
const transactionReport = ReportUtils.getReport(oneTransactionThreadReportID);
doesTransactionThreadReportHasViolations = !!transactionReport && OptionsListUtils.shouldShowViolations(transactionReport, betas ?? [], transactionViolations);
}
const hasErrorsOtherThanFailedReceipt =
doesReportHaveViolations || Object.values(allReportErrors).some((error) => error?.[0] !== Localize.translateLocal('iou.error.genericSmartscanFailureMessage'));
doesTransactionThreadReportHasViolations ||
doesReportHaveViolations ||
Object.values(allReportErrors).some((error) => error?.[0] !== Localize.translateLocal('iou.error.genericSmartscanFailureMessage'));
if (ReportUtils.isOneTransactionThread(report.reportID, report.parentReportID ?? '0', parentReportAction)) {
return;
}
Expand Down Expand Up @@ -214,6 +230,7 @@ function getOptionData({
policy,
parentReportAction,
hasViolations,
transactionViolations,
}: {
report: OnyxEntry<Report>;
reportActions: OnyxEntry<ReportActions>;
Expand All @@ -222,6 +239,7 @@ function getOptionData({
policy: OnyxEntry<Policy> | undefined;
parentReportAction: OnyxEntry<ReportAction> | undefined;
hasViolations: boolean;
transactionViolations?: OnyxCollection<TransactionViolation[]>;
}): ReportUtils.OptionData | undefined {
// 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
Expand Down Expand Up @@ -281,6 +299,21 @@ function getOptionData({
result.shouldShowSubscript = ReportUtils.shouldReportShowSubscript(report);
result.pendingAction = report.pendingFields?.addWorkspaceRoom ?? report.pendingFields?.createChat;
result.brickRoadIndicator = hasErrors || hasViolations ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : '';
const oneTransactionThreadReportID = ReportActionsUtils.getOneTransactionThreadReportID(report.reportID, ReportActionsUtils.getAllReportActions(report.reportID));
Copy link
Copy Markdown
Contributor

@Pujan92 Pujan92 Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of putting this logic here, how about placing it with the parent report OptionRowLHNData? I am suggesting this to keep the shouldDisplayViolations in a single place.

App/src/components/LHNOptionsList/OptionRowLHNData.tsx

Lines 37 to 39 in dd96852

const shouldDisplayViolations = canUseViolations && ReportUtils.shouldDisplayTransactionThreadViolations(fullReport, transactionViolations, parentReportAction); 

const oneTransactionThreadReportID = ReportActionsUtils.getOneTransactionThreadReportID(reportID, ReportActionsUtils.getAllReportActions(reportID));
let shouldDisplayOneTransactionThreadViolation = false;
if (oneTransactionThreadReportID) {
  const oneTransactionThreadReport = getReport(oneTransactionThreadReportID);

  if (
      ReportUtils.shouldDisplayTransactionThreadViolations(
          oneTransactionThreadReport,
          transactionViolations,
          ReportActionsUtils.getAllReportActions(reportID)[oneTransactionThreadReport?.parentReportActionID ?? '-1'],
      )
  ) {
    shouldDisplayOneTransactionThreadViolation = true;
  }
}

const shouldDisplayViolations = canUseViolations && (ReportUtils.shouldDisplayTransactionThreadViolations(fullReport, transactionViolations, parentReportAction) || shouldDisplayOneTransactionThreadViolation);

Copy link
Copy Markdown
Contributor Author

@truph01 truph01 Jul 10, 2024

Choose a reason for hiding this comment

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

I think we should let the SizebarUtils handle the logic to display the RBR in case one transaction report has violations because:

  1. We have already handled the logic to pin the one transaction report if it has a violation in SizebarUtils. So the logic to display RBR should be placed here for easier management.

  2. Reduce the complexity of the OptionRowLHNData file.

cc @robertjchen What do you think?

Copy link
Copy Markdown
Contributor

@Pujan92 Pujan92 Jul 10, 2024

Choose a reason for hiding this comment

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

Ok 👍

if (oneTransactionThreadReportID) {
const oneTransactionThreadReport = ReportUtils.getReport(oneTransactionThreadReportID);

if (
Permissions.canUseViolations(allBetas) &&
ReportUtils.shouldDisplayTransactionThreadViolations(
oneTransactionThreadReport,
transactionViolations,
ReportActionsUtils.getAllReportActions(report.reportID)[oneTransactionThreadReport?.parentReportActionID ?? '-1'],
)
) {
result.brickRoadIndicator = CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR;
}
}
result.ownerAccountID = report.ownerAccountID;
result.managerID = report.managerID;
result.reportID = report.reportID;
Expand Down
9 changes: 1 addition & 8 deletions src/libs/actions/Task.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ function createTaskAndNavigate(

const currentTime = DateUtils.getDBTimeWithSkew();
const lastCommentText = ReportUtils.formatReportLastMessageText(ReportActionsUtils.getReportActionText(optimisticAddCommentReport.reportAction));
const parentReport = getReport(parentReportID);
const parentReport = ReportUtils.getReport(parentReportID);
const optimisticParentReport = {
lastVisibleActionCreated: optimisticAddCommentReport.reportAction.created,
lastMessageText: lastCommentText,
Expand Down Expand Up @@ -906,13 +906,6 @@ function getParentReport(report: OnyxEntry<OnyxTypes.Report>): OnyxEntry<OnyxTyp
return ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${report.parentReportID}`];
}

/**
* Returns the report
*/
function getReport(reportID: string): OnyxEntry<OnyxTypes.Report> {
return ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`];
}

/**
* Cancels a task by setting the report state to SUBMITTED and status to CLOSED
*/
Expand Down