[HOLD for payment 2024-12-07] [$250] Categories - Error in Empty Category Not Dismissed

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: v9.0.64-0 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause Internal Team

Action Performed:

1. Go to Settings > Workspace > Categories.
2. Delete all categories and import a CSV file.
3. Select a field to map from the spreadsheet (e.g., Amount), enable it, and save
4. Observe the error message. Click the "Close" button to dismiss the error.

Expected Result:

The error message should be dismissed upon clicking the "Close" button.

Actual Result:

The error message remains displayed even after clicking the "Close" button.

Workaround:

Unknown

Platforms:

• [ ] Android: Standalone
• [ ] Android: HybridApp
• [ ] Android: mWeb Chrome
• [ ] iOS: Standalone
• [ ] iOS: HybridApp
• [ ] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/50ab8270-c0c4-4744-8cd2-e0694c6801b2

Bug6669395_1732017654330!Bulk_Export_id_DEFAULT_CSV__1_.csv

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021859068445047163468
• Upwork Job ID: 1859068445047163468
• Last Price Increase: 2024-11-20
• Automatic offers:
• brunovjk | Reviewer | 105006793
• abzokhattab | Contributor | 105006794

Issue Owner

Current Issue Owner: @kadiealexander

[melvin-bot[bot]]

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[Nodebrute]

Edited by proposal-police: This proposal was edited at 2024-11-19 13:20:58 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Error in Empty Category Not Dismissed

What is the root cause of that problem?

The categoryName will be undefined in this case, causing the function to return early. As a result, pressing the cross will have no effect.

https://github.com/Expensify/App/blob/main/src/libs/actions/Policy/Category.ts#L848-L850

What changes do you think we should make in order to solve the problem?

If the category has an error like 'Empty category name', we can clear the category from Onyx. Here's an example of how we can do this:

 const hasEmptyCategoryNameError = Object.values(item.errors).some(error => error === 'Empty category name');
    if(hasEmptyCategoryNameError){
        Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}${policyID}`, {
            [""]: null
        });
    }

Above, we are removing the entire category from Onyx, effectively clearing the empty category from the category list.

https://github.com/user-attachments/assets/d486a411-b096-4d26-be28-d8e3e0905cce

Optionally: We can also choose to clear only the errors when pressing x. This will remove the error, but the empty category will still remain.

   [""]: {
            errors: null,
        },

Note: We can achieve the same result with different checks, such as !item.keyForList && item.errors?.length, but the core logic remains the same — which is to remove the empty category from the list.

What alternative solutions did you explore? (Optional)

Alternatively, we can implement the same approach as my main solution but omit the hasEmptyCategoryNameError check.

[abzokhattab]

Edited by proposal-police: This proposal was edited at 2024-11-19 13:44:54 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Error in Empty Category Not Dismissed

What is the root cause of that problem?

The issue occurs because keyForList is undefined for empty category names in the Name column of the uploaded CSV. https://github.com/Expensify/App/blob/8a80de01ccfaf9f3890459f721a3f0b61810bbd3/src/pages/workspace/categories/WorkspaceCategoriesPage.tsx#L111-L122

When attempting to clear the error, the function uses keyForList, which is undefined, resulting in no action being performed.

https://github.com/Expensify/App/blob/8a80de01ccfaf9f3890459f721a3f0b61810bbd3/src/pages/workspace/categories/WorkspaceCategoriesPage.tsx#L169-L171

This happens because the current validation allows empty names in the Name column during CSV upload.

Since we dont allow empty names when creating or updating a category, we should not allow this when uploading a csv as well, to acheive that, we should enhance the validate function to throw an error if any of the names is empty

What changes do you think we should make in order to solve the problem?

Enhance the validate function here to detect and throw an error if any of the names in the Name column are empty.

Code Changes:

     const validate = useCallback(() => {
        const columns = Object.values(spreadsheet?.columns ?? {});
        const errors: Errors = {};

        // Check if required columns are mapped
        if (!requiredColumns.every((requiredColumn) => columns.includes(requiredColumn.value))) {
            requiredColumns.forEach((requiredColumn) => {
                if (columns.includes(requiredColumn.value)) {
                    return;
                }
                errors.required = translate('spreadsheet.fieldNotMapped', {fieldName: requiredColumn.text});
            });
        } else {
            // Check for duplicate mappings
            const duplicate = findDuplicate(columns);
            const duplicateColumn = columnRoles.find((role) => role.value === duplicate);
            if (duplicateColumn) {
                errors.duplicates = translate('spreadsheet.singleFieldMultipleColumns', {fieldName: duplicateColumn.text});
            } else {
                // Check for empty mapped names
                const categoriesNamesColumn = columns.findIndex((column) => column === CONST.CSV_IMPORT_COLUMNS.NAME);
                const categoriesNames = categoriesNamesColumn !== -1 ? spreadsheet?.data[categoriesNamesColumn] : [];
                if (categoriesNames?.some((name, index) => (containsHeader ? index !== 0 : true) && !name.trim())) {
                    errors.emptyNames = translate('spreadsheet.emptyMappedField', {fieldName: CONST.CSV_IMPORT_COLUMNS.NAME});
                }
            }
        }
        return errors;
    }, [spreadsheet?.columns, spreadsheet?.data, requiredColumns, translate, columnRoles, containsHeader]);

we should also add a new translation for spreadsheet.emptyMappedField in the translation files.

Result:

https://github.com/user-attachments/assets/4894c19e-a02e-491a-9275-fc7fcde79890

What alternative solutions did you explore? (Optional)

[Nodebrute]

Proposal Updated Added a note

[huult]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Error in Empty Category Not Dismissed

What is the root cause of that problem?

We are unable to close this error because we can't find the category. As a result, we return early and cannot set the error to null to hide the error message.

https://github.com/Expensify/App/blob/faa48b605d708d7e8814e54fbf2b88a883c9c0c0/src/libs/actions/Policy/Category.ts#L845-L857

https://github.com/Expensify/App/blob/be6acac6f051b290d96e14353704065e4928b53a/src/components/OfflineWithFeedback.tsx#L147-L154

We can't find the category because we don't have a category name. The reason we don't have a category name is that the imported CSV does not include a name field.

What changes do you think we should make in order to solve the problem?

To fix this issue, we shouldn't accept a CSV containing invalid data. We should check whether the CSV includes a name field or not. If the CSV does not include a name field, we should return early and display an error message. Something like this:

// src/components/ImportSpreadsheet.tsx#L96
+    const rowTitle = data.at(0) as string[];

+    if (!rowTitle.includes(CONST.CSV_IMPORT_COLUMNS.NAME)) {
+        setUploadFileError(true, 'spreadsheet.importFailedTitle', 'spreadsheet.invalidFileMessage');
+        setIsReadingFIle(false);
+        if (fileURI && !file.uri) {
+            URL.revokeObjectURL(fileURI);
+        }
+        return;
+    }

Test branch

POC

https://github.com/user-attachments/assets/4f62c871-dc02-40ac-96a5-2c60d2b7c3b7

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~021859068445047163468

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @brunovjk (External)

[brunovjk]

Reviewing proposals soon.

[nkdengineer]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The error message remains displayed even after clicking the "Close" button.

What is the root cause of that problem?

We import categories with name as empty string.

https://github.com/Expensify/App/blob/7bfb34527508b47732cab6d4280a943ffab48066/src/pages/workspace/categories/ImportedCategoriesPage.tsx#L92

Then BE returns an error with the key as an empty string. We cannot remove this error because the category here is empty string and we return early if !category is true.

https://github.com/Expensify/App/blob/faa48b605d708d7e8814e54fbf2b88a883c9c0c0/src/libs/actions/Policy/Category.ts#L845-L857

What changes do you think we should make in order to solve the problem?

Because the category name cannot be an empty string, we have some ideas to resolve this problem:

1. We can filter out the categories with name is empty here

const categories = categoriesNames
    ?.slice(containsHeader ? 1 : 0)
    .filter((name) => !name)
    .map((name, index) => {
        const categoryAlreadyExists = policyCategories?.[name];
        const existingGLCodeOrDefault = categoryAlreadyExists?.['GL Code'] ?? '';
        return {
            name,
            enabled: categoriesEnabledColumn !== -1 ? categoriesEnabled?.[containsHeader ? index + 1 : index] === 'true' : true,
            // eslint-disable-next-line @typescript-eslint/naming-convention
            'GL Code': categoriesGLCodeColumn !== -1 ? categoriesGLCode?.[containsHeader ? index + 1 : index] ?? '' : existingGLCodeOrDefault,
        };
    });

https://github.com/Expensify/App/blob/7bfb34527508b47732cab6d4280a943ffab48066/src/pages/workspace/categories/ImportedCategoriesPage.tsx#L95

OPTIONAL: if categoriesNames after filtering is an empty array we can display an error modal with content like The name column cannot be an empty string, please select another column that doesn't contain empty string to map the category name. To display this modal we can reuse ConfirmModal here.

https://github.com/Expensify/App/blob/7bfb34527508b47732cab6d4280a943ffab48066/src/pages/workspace/categories/ImportedCategoriesPage.tsx#L149

2. We can immediately display the modal that I mentioned above when we click on the submit button. To do that we can check if categoriesNames array contains an empty string we can show this modal and return it early.

https://github.com/Expensify/App/blob/7bfb34527508b47732cab6d4280a943ffab48066/src/pages/workspace/categories/ImportedCategoriesPage.tsx#L92

What alternative solutions did you explore? (Optional)

[brunovjk]

Thank you all for the proposals.

@Nodebrute, I think your proposal alone is not enough as it doesn’t address the root cause of allowing empty categories during CSV import. @Abzokhattab, @huult, and @nkdengineer, correct me if I’m wrong, but your proposals seem similar in that they focus on updating the CSV import validation.

With that, I think we should go with @Abzokhattab's proposal, as it is the first robust solution addressing the root cause effectively while maintaining user clarity.

🎀👀🎀 C+ reviewed

[melvin-bot[bot]]

Triggered auto assignment to @neil-marcellini, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[huult]

@brunovjk Yes, thank you for your feedback. For my proposal, I suggest notifying the user about invalid steps during the CSV import process to enhance the user experience. Regarding the proposal you voted for, validation occurs after the import. However, since editing the data fields is not possible at that step and validation is always shown but cannot proceed, it becomes redundant.

[abzokhattab]

but cannot proceed, it becomes redundant.

Hi Huult, i think this is not completely accurate. The user can still choose a different column and select it as the "name" column (as shown in the POC). Additionally, it provides visibility for the user to identify which columns have issues and gives them the opportunity to make changes

[neil-marcellini]

Thank you all for the proposals.

@Nodebrute, I think your proposal alone is not enough as it doesn’t address the root cause of allowing empty categories during CSV import. @abzokhattab, @huult, and @nkdengineer, correct me if I’m wrong, but your proposals seem similar in that they focus on updating the CSV import validation.

With that, I think we should go with @abzokhattab's proposal, as it is the first robust solution addressing the root cause effectively while maintaining user clarity.

🎀👀🎀 C+ reviewed

I agree, it's a good proposal and resulting UX. Hiring.

[melvin-bot[bot]]

📣 @brunovjk 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[melvin-bot[bot]]

📣 @abzokhattab 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[brunovjk]

Hey @abzokhattab :D just checking, would you happen to have an ETA for this PR? Thanks.

[abzokhattab]

Thanks @brunovjk the PR is ready!

[brunovjk]

Update: PR still under review.

[melvin-bot[bot]]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.68-7 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

• https://github.com/Expensify/App/pull/52992

If no regressions arise, payment will be issued on 2024-12-07. :confetti_ball:

For reference, here are some details about the assignees on this issue:

• @abzokhattab requires payment automatic offer (Contributor)
• @brunovjk requires payment automatic offer (Reviewer)

[melvin-bot[bot]]

@brunovjk @kadiealexander @brunovjk The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[brunovjk]

BugZero Checklist:

• [x] [Contributor] Classify the bug:

Bug classification

Source of bug:

• [ ] 1a. Result of the original design (eg. a case wasn't considered)
• [x] 1b. Mistake during implementation
• [ ] 1c. Backend bug
• [ ] 1z. Other:

Where bug was reported:

• [x] 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
• [ ] 2b. Reported on staging (eg. found during regression or PR testing)
• [ ] 2d. Reported on a PR
• [ ] 2z. Other:

Who reported the bug:

• [ ] 3a. Expensify user
• [ ] 3b. Expensify employee
• [ ] 3c. Contributor
• [ ] 3d. QA
• [x] 3z. Other: Applause Internal Team

• [x] [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

  Link to comment: No PR directly introduced the bug. This happens because the current validation allows empty names in the Name column during CSV upload.

• [ ] [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

  Link to discussion:

• [x] [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

Regression Test Proposal

Test:

1. Navigate to Settings Workspace Categories .
2. Click on the three-dot menu and select Import a Spreadsheet .
3. Use a spreadsheet file where one column contains one or more empty cells (e.g., the attached example file).
4. Select the column with empty values as the Name field.
5. Choose another column as the Required field.
6. Click Submit and verify that the following error message appears: Oops! The field ("Name") contains one or more empty values. Please review and try again.
7. Update the Name field to a column that does not contain empty values and retry the process.
8. Confirm that the categories are imported successfully without any errors.

Do we agree 👍 or 👎

[kadiealexander]

Test GH: https://github.com/Expensify/Expensify/issues/451500

[kadiealexander]

Payments due:

• [x] Contributor: @abzokhattab paid $250 via Upwork (Offer link)
• [x] Reviewer: @brunovjk owed $250 via Upwork (Offer link)