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

[HOLD for payment 2024-12-17] [$250] mWeb - Category - Categories imported are disabled #52466

Closed
1 of 8 tasks
IuliiaHerets opened this issue Nov 13, 2024 · 31 comments
Closed
1 of 8 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Nov 13, 2024

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: 9.0.61-0
Reproducible in staging?: Y
Reproducible in production?: Y
Issue reported by: Applause Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/home
  2. Go to workspace settings
  3. Tap category
  4. Tap on 3 dots on top
  5. Import the below file

Expected Result:

Categories imported must be shown as enabled.

Actual Result:

Categories imported are shown as disabled.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6663366_1731473053062!Applausecard_s_Workspace_categories.csv

Bug6663366_1731472895825.Screenrecorder-2024-11-13-10-07-27-196.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021858671038820315324
  • Upwork Job ID: 1858671038820315324
  • Last Price Increase: 2024-11-26
  • Automatic offers:
    • huult | Contributor | 105110019
Issue OwnerCurrent Issue Owner: @alexpensify
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 13, 2024
Copy link

melvin-bot bot commented Nov 13, 2024

Triggered auto assignment to @alexpensify (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.

@NJ-2020
Copy link
Contributor

NJ-2020 commented Nov 13, 2024

Proposal

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

mWeb - Category - Categories imported are disabled

What is the root cause of that problem?

It's because after we upload the spreadsheet, the enabled value column return value boolean instead of string
So since we check if the enabled column is equal to true it will return false

enabled: categoriesEnabledColumn !== -1 ? categoriesEnabled?.[containsHeader ? index + 1 : index] === 'true' : true,

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

We should convert the enabled column value to string

return {
    name,
    // enabled: categoriesEnabledColumn !== -1 ? categoriesEnabled?.[containsHeader ? index + 1 : index] === 'true' : true,
    enabled: categoriesEnabledColumn !== -1 ? categoriesEnabled?.[containsHeader ? index + 1 : index]?.toString()  === 'true' : true,

Result

Screen.Recording.2024-11-13.at.04.35.23.mov

What alternative solutions did you explore? (Optional)

@huult
Copy link
Contributor

huult commented Nov 13, 2024

Edited by proposal-police: This proposal was edited at 2024-11-13 14:32:39 UTC.

Proposal

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

Categories imported are disabled

What is the root cause of that problem?

After we read the CSV spreadsheet into JSON, the type of the 'enable' field is returned as boolean data

const data = XLSX.utils.sheet_to_json(worksheet, {header: 1, blankrows: false});
setSpreadsheetData(data as string[][])
.then(() => {
Navigation.navigate(goTo);

The condition check for enable will be false because we are checking for the string 'true', which is a boolean as a string

enabled: categoriesEnabledColumn !== -1 ? categoriesEnabled?.[containsHeader ? index + 1 : index] === 'true' : true,

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

To fix this issue, we should convert the data in the cell to a string type to match the expected type and the current logic check. We should use this approach to fix any mismatches, and for the rest of the fields, we should force the return to be of type string[][].

// src/components/ImportSpreadsheet.tsx#L96
            .then((arrayBuffer) => {
                const workbook = XLSX.read(new Uint8Array(arrayBuffer), {type: 'buffer'});
                const worksheet = workbook.Sheets[workbook.SheetNames[0]];
                const data = XLSX.utils.sheet_to_json(worksheet, {header: 1, blankrows: false});
+                const convertedData = data.map((row) => row.map((cell) => (typeof cell === 'string' ? cell : String(cell))));

-                setSpreadsheetData(data as string[][])
+                setSpreadsheetData(convertedData as string[][])
                    .then(() => {
                        Navigation.navigate(goTo);
                    })
                    .catch(() => {
                        setUploadFileError(true, 'spreadsheet.importFailedTitle', 'spreadsheet.invalidFileMessage');
                    });
            })
POC
Screen.Recording.2024-11-13.at.21.24.41.mov

What alternative solutions did you explore? (Optional)

The main solution fixes the issue where the fields return the wrong data type for all entries. If we want to focus only on enable, it can be changed like this:

// src/pages/workspace/categories/ImportedCategoriesPage.tsx#L93
-      const categoriesEnabled = categoriesEnabledColumn !== -1 ? spreadsheet?.data[categoriesEnabledColumn].map((enabled) => enabled) : [];
+      const categoriesEnabled =
+                  categoriesEnabledColumn !== -1 ? spreadsheet?.data[categoriesEnabledColumn].map((enabled) => (typeof enabled === 'string' ? enabled : String(enabled))) : [];

@melvin-bot melvin-bot bot added the Overdue label Nov 18, 2024
@alexpensify alexpensify added the External Added to denote the issue can be worked on by a contributor label Nov 19, 2024
@melvin-bot melvin-bot bot changed the title mWeb - Category - Categories imported are disabled [$250] mWeb - Category - Categories imported are disabled Nov 19, 2024
Copy link

melvin-bot bot commented Nov 19, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 19, 2024
Copy link

melvin-bot bot commented Nov 19, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Nov 19, 2024
@alexpensify
Copy link
Contributor

@getusha, when you get a chance, please review these proposals and identify whether one will fix the issue. Thank you!

@cretadn22
Copy link
Contributor

Proposal

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

Category - Categories imported are disabled

What is the root cause of that problem?

It appears that the category file associated with this issue was exported from another source, resulting in the enable field being saved as a boolean type. Typically, when exporting categories on Expensify, all data in the file should be stored as string type.

enabled: categoriesEnabledColumn !== -1 ? categoriesEnabled?.[containsHeader ? index + 1 : index] === 'true' : true,

This discrepancy causes the above condition to not match and the enable field is stored as false.

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

When importing a spreadsheet, we should ensure that all data is converted to string type exclude null or undefined values

const data = XLSX.utils.sheet_to_json(worksheet, {header: 1, blankrows: false});
setSpreadsheetData(data as string[][])
.then(() => {
Navigation.navigate(goTo);

 const stringData = data.map(row => row.map(cell => (cell !== null && cell !== undefined ? String(cell) : cell)));

Please note: If we don’t exclude null and undefined values, they will be saved as strings, such as 'null' and 'undefined'. This could potentially break the UI and result in unexpected behavior. My approach is largely similar to the proposal above; however, I believe it offers significant improvements and mitigates many risks, particularly because ImportSpreadsheet is utilized in several other places.

What alternative solutions did you explore? (Optional)

@getusha
Copy link
Contributor

getusha commented Nov 20, 2024

Why convert it to string if it's always a boolean, can't we just use that?

@cretadn22
Copy link
Contributor

cretadn22 commented Nov 20, 2024

Typically, when exporting categories on Expensify, all data in the file should be stored as string type.

Therefore, when importing the category file, we handle all data to string type

cc @getusha

@NJ-2020
Copy link
Contributor

NJ-2020 commented Nov 21, 2024

Yes we can also check if the value is equal to boolean as well i.e ... === true but as you can see the type of spreadsheet data is number, string

@huult
Copy link
Contributor

huult commented Nov 21, 2024

@getusha We can handle boolean, number, or other types. However, with the current logic, we are handling string[][]. To align the data with the expected type, we should convert it accordingly to minimize changes and updates in the component logic. Therefore, I believe converting the data is a better approach for this ticket.

setSpreadsheetData(data as string[][])

Copy link

melvin-bot bot commented Nov 26, 2024

@alexpensify, @getusha Eep! 4 days overdue now. Issues have feelings too...

@melvin-bot melvin-bot bot added the Overdue label Nov 26, 2024
Copy link

melvin-bot bot commented Nov 26, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

Copy link

melvin-bot bot commented Nov 27, 2024

@alexpensify @getusha this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@alexpensify
Copy link
Contributor

@getusha when you get a chance, can you please review the recent feedback? Thanks!

Copy link

melvin-bot bot commented Nov 28, 2024

@alexpensify, @getusha Still overdue 6 days?! Let's take care of this!

@getusha
Copy link
Contributor

getusha commented Nov 28, 2024

Reviewing

@melvin-bot melvin-bot bot removed the Overdue label Nov 28, 2024
@getusha
Copy link
Contributor

getusha commented Nov 28, 2024

It makes sense to fix it in ImportSpreadsheet since there are other pages like ImportedTagsPage where the issue could possibly occur. @huult's proposal looks good to me
🎀 👀 🎀 C+ Reviewed.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 28, 2024
Copy link

melvin-bot bot commented Nov 28, 2024

📣 @huult 🎉 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 📖

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Nov 28, 2024
@huult
Copy link
Contributor

huult commented Dec 2, 2024

@getusha Could you review the pull request?

@alexpensify
Copy link
Contributor

@getusha keep us posted if you are unable to review this one. Thanks!

@huult
Copy link
Contributor

huult commented Dec 7, 2024

@alexpensify This pull request for this ticket has already been merged into the main branch.

@alexpensify
Copy link
Contributor

Whoops, waiting for this one to go into Production. Thanks for flagging!

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Dec 10, 2024
@melvin-bot melvin-bot bot changed the title [$250] mWeb - Category - Categories imported are disabled [HOLD for payment 2024-12-17] [$250] mWeb - Category - Categories imported are disabled Dec 10, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Dec 10, 2024
Copy link

melvin-bot bot commented Dec 10, 2024

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

Copy link

melvin-bot bot commented Dec 10, 2024

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

If no regressions arise, payment will be issued on 2024-12-17. 🎊

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

Copy link

melvin-bot bot commented Dec 10, 2024

@getusha @alexpensify @getusha 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]

@alexpensify
Copy link
Contributor

Payment Summary:

Contributor: @huult owed $250 via Upwork [LINK]
Contributor+: @getusha owed $250 via NewDot

Upwork Job: https://www.upwork.com/jobs/~021858671038820315324

@alexpensify
Copy link
Contributor

Heads up, I will be offline until Wednesday, December 18, 2024, and will not actively watch over this GitHub during that period.

If this GitHub requires an urgent update, please ask for help in the #expensify-open-source Slack Room. If the inquiry can wait, I'll review it when I return online.


I will complete the Upwork payment process when I'm back online.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Dec 17, 2024
@garrettmknight garrettmknight moved this to Bugs and Follow Up Issues in [#whatsnext] #expense Dec 17, 2024
@garrettmknight garrettmknight moved this from Bugs and Follow Up Issues to Hold for Payment in [#whatsnext] #expense Dec 17, 2024
@alexpensify
Copy link
Contributor

All set here, I've completed the payment process in Upwork for @huult.

@getusha - please submit a request via NewDot - thanks!

Payment Summary: #52466 (comment)

@github-project-automation github-project-automation bot moved this from Hold for Payment to Done in [#whatsnext] #expense Dec 18, 2024
@garrettmknight
Copy link
Contributor

$250 approved for @getusha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
Status: Done
Development

No branches or pull requests

8 participants