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

Jump to bottom

clear policy errors #44130

Merged
merged 12 commits into from
Jul 22, 2024
Merged

clear policy errors #44130

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f2efad0
clear policy errors
Nodebrute Jun 21, 2024
143dd13
Merge branch 'Expensify:main' into policyErrors
Nodebrute Jun 25, 2024
196f16f
remove errors
Nodebrute Jun 25, 2024
4a02aee
fix lint
Nodebrute Jun 25, 2024
e875e3f
Merge branch 'Expensify:main' into policyErrors
Nodebrute Jun 27, 2024
a0202fe
Merge branch 'main' into policyErrors
Nodebrute Jul 15, 2024
72518c1
fix reportName error
Nodebrute Jul 15, 2024
e340e43
remove rbr from settings
Nodebrute Jul 15, 2024
0befef8
fix lint errors
Nodebrute Jul 15, 2024
2ff95e0
Merge branch 'Expensify:main' into policyErrors
Nodebrute Jul 22, 2024
671f6cd
Merge branch 'Expensify:main' into policyErrors
Nodebrute Jul 22, 2024
f049a8b
import localize
Nodebrute Jul 22, 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
4 changes: 0 additions & 4 deletions src/libs/actions/Report.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ import * as EmojiUtils from '@libs/EmojiUtils';
import * as Environment from '@libs/Environment/Environment';
import * as ErrorUtils from '@libs/ErrorUtils';
import isPublicScreenRoute from '@libs/isPublicScreenRoute';
import * as Localize from '@libs/Localize';
import Log from '@libs/Log';
import {registerPaginationConfig} from '@libs/Middleware/Pagination';
import Navigation from '@libs/Navigation/Navigation';
Expand Down Expand Up @@ -654,9 +653,6 @@ function updateGroupChatName(reportID: string, reportName: string) {
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
reportName: ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.reportName ?? null,
errors: {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure how our problem gets solved by not setting errors here. The problem mentioned here still exists. When openReport is called, errorFields comes back as follows with null (as shown below) and, strangely, FE resets the errorFields in report in Onyx. Don’t we need to get to the bottom of this?

Screenshot 2024-07-20 at 11 17 08 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Investigating

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rojiphil Hey, I'm still unable to reproduce the issue you're mentioning with RBR. You can check the video below. Even after the openReport calls, the RBR isn't disappearing. I've also tried reloading the page, but the RBR remains until we manually remove it by pressing the cross. Please let me know if I am missing any step

Screen.Recording.2024-07-22.at.1.54.12.PM.1.1.1.mov

Copy link
Contributor Author

@Nodebrute Nodebrute Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure how our problem gets solved by not setting errors here. The problem mentioned here still exists. When openReport is called, errorFields comes back as follows with null (as shown below) and, strangely, FE resets the errorFields in report in Onyx. Don’t we need to get to the bottom of this?

We don't need to set errors here. The backend returns errorFields in case of failure, which is sufficient to display errors here. We don't even set errors when updating the room name. I don't think we need to include errors in the failure data when updating the group name. Removing errors seems to address the main issue of removing RBR after pressing the cross. Let me know your thoughts.

App/src/libs/actions/Report.ts

Lines 2293 to 2305 in eec4d6a

const failureData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
reportName: previousName,
},
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
value: {[optimisticRenamedAction.reportActionID]: null},
},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rojiphil Additionally, the backend is returning the merge method, so it is not affecting the existing errorFields in the Onyx.

Screen.Recording.2024-07-22.at.2.41.36.PM.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to set errors here. The backend returns errorFields in case of failure, which is sufficient to display errors here.

This is a good cleanup. Earlier as mentioned here settings also had a provision to update room name. But, now, since editing the room name is no longer present within settings page, we can avoid setting GBR for settings menu.

reportName: Localize.translateLocal('common.genericErrorMessage'),
},
pendingFields: {
reportName: null,
},
Expand Down
28 changes: 12 additions & 16 deletions src/pages/ReportDetailsPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -696,22 +696,18 @@ function ReportDetailsPage({policies, report, session, personalDetails}: ReportD
promotedActions={promotedActions}
/>

{menuItems.map((item) => {
const brickRoadIndicator =
ReportUtils.hasReportNameError(report) && item.key === CONST.REPORT_DETAILS_MENU_ITEM.SETTINGS ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined;
return (
<MenuItem
key={item.key}
title={translate(item.translationKey)}
subtitle={item.subtitle}
icon={item.icon}
onPress={item.action}
isAnonymousAction={item.isAnonymousAction}
shouldShowRightIcon={item.shouldShowRightIcon}
brickRoadIndicator={brickRoadIndicator ?? item.brickRoadIndicator}
/>
);
})}
{menuItems.map((item) => (
<MenuItem
key={item.key}
title={translate(item.translationKey)}
subtitle={item.subtitle}
icon={item.icon}
onPress={item.action}
isAnonymousAction={item.isAnonymousAction}
shouldShowRightIcon={item.shouldShowRightIcon}
brickRoadIndicator={item.brickRoadIndicator}
/>
))}

{shouldShowDeleteButton && (
<MenuItem
Expand Down
Loading