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.

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

Fix crash after leaving group as the last member #48096

Merged
merged 3 commits into from
Aug 27, 2024
Merged

Fix crash after leaving group as the last member #48096

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b6faa55
fallback using double pipe
bernhardoj Aug 27, 2024
ddbe832
supress lint
bernhardoj Aug 27, 2024
c7d65ab
use double pipe
bernhardoj Aug 27, 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
3 changes: 2 additions & 1 deletion src/libs/actions/ReportActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ function clearReportActionErrors(reportID: string, reportAction: ReportAction, k
});

// If there's a linked transaction, delete that too
const linkedTransactionID = ReportActionUtils.getLinkedTransactionID(reportAction.reportActionID, originalReportID ?? '-1');
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
const linkedTransactionID = ReportActionUtils.getLinkedTransactionID(reportAction.reportActionID, originalReportID || '-1');
if (linkedTransactionID) {
Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION}${linkedTransactionID}`, null);
}
Expand Down
3 changes: 2 additions & 1 deletion src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,8 @@ function PopoverReportActionContextMenu(_props: unknown, ref: ForwardedRef<Repor
typeRef.current = type;
reportIDRef.current = reportID ?? '-1';
reportActionIDRef.current = reportActionID ?? '-1';
originalReportIDRef.current = originalReportID ?? '-1';
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
originalReportIDRef.current = originalReportID || '-1';
selectionRef.current = selection;
setIsPopoverVisible(true);
reportActionDraftMessageRef.current = draftMessage;
Expand Down
3 changes: 2 additions & 1 deletion src/pages/home/report/ReportActionItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,8 @@ function ReportActionItem({
const {translate} = useLocalize();
const {shouldUseNarrowLayout} = useResponsiveLayout();
const blockedFromConcierge = useBlockedFromConcierge();
const originalReportID = useMemo(() => ReportUtils.getOriginalReportID(report.reportID, action) ?? '-1', [report.reportID, action]);
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
const originalReportID = useMemo(() => ReportUtils.getOriginalReportID(report.reportID, action) || '-1', [report.reportID, action]);
Copy link
Contributor

Choose a reason for hiding this comment

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

There are few other places that we're using originalReportID ?? '-1', do you think we should update them as well?

App/src/libs/actions/ReportActions.ts

Line 35 in 6ca91cd

const linkedTransactionID = ReportActionUtils.getLinkedTransactionID(reportAction.reportActionID, originalReportID ?? '-1');

App/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.tsx

Line 214 in 6ca91cd

originalReportIDRef.current = originalReportID ?? '-1';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be safe to do it, but I think it's best that we have a guideline to use || instead of ?? for string and update them all in one issue (break in several PRs perhaps).

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just update these 2 places here? And then we can discuss about the second part later.

cc @cristipaval

Copy link
Contributor

Choose a reason for hiding this comment

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

OR, can we update getOriginalReportID to return undefined instead empty string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think returning reportID || undefined looks unclean.

I have updated to use || for now. We can revert it if @cristipaval disagrees

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw, I'm currently having trouble running the native app.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here :/ I can't run native too

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we raise this for discussion somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah there was a thread here: https://expensify.slack.com/archives/C01GTK53T8Q/p1724842994690509

const [draftMessage] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${originalReportID}`, {
selector: (draftMessagesForReport) => {
const matchingDraftMessage = draftMessagesForReport?.[action.reportActionID];
Expand Down
Loading