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

[Payment due Oct 7][$250] [Support CSV] Categories - File contains errors when downloading CSV with no category #49022

Closed
6 tasks done
IuliiaHerets opened this issue Sep 11, 2024 · 49 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Sep 11, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 9.0.32-3
Reproducible in staging?: Y
Reproducible in production?: N/A - new feature, doesn't exist in prod
Email or phone of affected tester (no customers): applausetester+kh010901@applause.expensifail.com
Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to Workspace settings > Categories.
  3. Delete all the categories.
  4. Tap 3-dot menu.
  5. Tap Download CSV.
  6. Open the downloaded file.

Expected Result:

The file should not contain error when downloading CSV with no categories.

Actual Result:

The file contains errors when downloading CSV with no category.

Workaround:

Unknown

Platforms:

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6600256_1726080845912.Screen_Recording_20240912_024951_Sheets.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021833956684433969336
  • Upwork Job ID: 1833956684433969336
  • Last Price Increase: 2024-09-11
  • Automatic offers:
    • brunovjk | Reviewer | 103982747
    • cretadn22 | Contributor | 103982748
Issue OwnerCurrent Issue Owner: @brunovjk
@IuliiaHerets IuliiaHerets added DeployBlockerCash This issue or pull request should block deployment Bug Something is broken. Auto assigns a BugZero manager. DeployBlocker Indicates it should block deploying the API labels Sep 11, 2024
Copy link

melvin-bot bot commented Sep 11, 2024

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

Copy link

melvin-bot bot commented Sep 11, 2024

Triggered auto assignment to @blimpich (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@melvin-bot melvin-bot bot added the Daily KSv2 label Sep 11, 2024
@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Sep 11, 2024
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@blimpich
Copy link
Contributor

Looking into this.

@blimpich blimpich changed the title Categories - File contains errors when downloading CSV with no category [Support CSV] Categories - File contains errors when downloading CSV with no category Sep 11, 2024
@blimpich
Copy link
Contributor

Probably related to #48687

@mountiny do you consider this a deploy blocker?

@blimpich blimpich removed DeployBlocker Indicates it should block deploying the API DeployBlockerCash This issue or pull request should block deployment labels Sep 11, 2024
@blimpich
Copy link
Contributor

Talked about it in Slack, decided its not a deploy blocker. Doesn't prevent them from using the app and is also pretty edge case.

@blimpich blimpich added Daily KSv2 External Added to denote the issue can be worked on by a contributor and removed Hourly KSv2 labels Sep 11, 2024
@melvin-bot melvin-bot bot changed the title [Support CSV] Categories - File contains errors when downloading CSV with no category [$250] [Support CSV] Categories - File contains errors when downloading CSV with no category Sep 11, 2024
Copy link

melvin-bot bot commented Sep 11, 2024

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

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

melvin-bot bot commented Sep 11, 2024

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

@blimpich
Copy link
Contributor

Discussed expected behavior in this #expensify-open-source thread. Probably should just not be showing the "download csv" button if there are no categories. @rlinoz can you confirm that?

@rlinoz
Copy link
Contributor

rlinoz commented Sep 11, 2024

Yeah, I think we can go with that.

Another thing that we should do is make sure that if a download CSV request throws an error we don't write it in the CSV and instead show a modal.

Copy link

melvin-bot bot commented Sep 16, 2024

Current assignee @rlinoz is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

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

melvin-bot bot commented Sep 16, 2024

📣 @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

Copy link

melvin-bot bot commented Sep 16, 2024

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

@cretadn22
Copy link
Contributor

cretadn22 commented Sep 17, 2024

@brunovjk @rlinoz Could you please ask the design team to provide the correct message for the download failure modal?

26

@rlinoz
Copy link
Contributor

rlinoz commented Sep 17, 2024

@cretadn22 you can use the ones in common

common.downloadFailedTitle
common.downloadFailedDescription

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 17, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Sep 23, 2024
@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Sep 24, 2024

heads up, I'm going to be ooo until 9/30. Since we're working on a PR I'm not going to invite a BZ teammate to monitor this while I'm away. If you need anything from BZ please reach out in slack for a volunteer. Thanks!

@cretadn22
Copy link
Contributor

cretadn22 commented Sep 30, 2024

Request to increase payment amount

The fix broadens the scope of the original bug and the additional PR represents a significant modification. Below is my summary:

  • The original fix: Incorporating an error handling mechanism for importing the Category and modifying the display of the download button

  • The additional fix (the issue description does not mention this and It is required after my original PR is merged): Also apply the fix to Tag and Member page

@rlinoz @brunovjk I welcome your thoughts on this matter

@Christinadobrzyn Christinadobrzyn changed the title [$250] [Support CSV] Categories - File contains errors when downloading CSV with no category [Payment due Oct 3][$250] [Support CSV] Categories - File contains errors when downloading CSV with no category Sep 30, 2024
@brunovjk
Copy link
Contributor

brunovjk commented Sep 30, 2024

@cretadn22 I welcome your thoughts on this matter.

I initially thought it was a kind of regression as now I felt obliged to raise the problem during PR testing, but I'm not sure. In favor of @cretadn22, they did an excellent job with both PRs. They were proactive in discussing solutions with us here and quickly created the PR after the issue was raised. However, I don't think much beyond that, it's up to you @Christinadobrzyn and @rlinoz :D

Either way I will return here to complete the BZ checklist before payment.

@Christinadobrzyn Christinadobrzyn changed the title [Payment due Oct 3][$250] [Support CSV] Categories - File contains errors when downloading CSV with no category [Payment due Oct 7][$250] [Support CSV] Categories - File contains errors when downloading CSV with no category Oct 2, 2024
@Christinadobrzyn
Copy link
Contributor

Thanks for reaching out @cretadn22 about the bonus request! Also thanks for the context and thoughts @brunovjk.

I'm discussing this with the team and will get back to you as soon as possible!

Note, I pushed out the payment date to Oct 7th because this PR is still in the 7 day window.

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Oct 3, 2024

Okay payment summary, let me know how this looks!

Payouts due:

Do we need a regression test for this?

@brunovjk
Copy link
Contributor

brunovjk commented Oct 3, 2024

Thank you very much @Christinadobrzyn.

Do we need a regression test for this?

Yes, I believe we can use one, I will do it before payday, I will keep an eye out for regressions.

@Christinadobrzyn Christinadobrzyn added Daily KSv2 and removed Weekly KSv2 labels Oct 7, 2024
@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Oct 7, 2024

Hi @brunovjk would you mind writing up a regression test for us? Thank you!

Moving to daily to pay out asap

@brunovjk
Copy link
Contributor

brunovjk commented Oct 7, 2024

  • [@brunovjk] The PR that introduced the bug has been identified. Link to the PR: #47927: Export Categories CSV #48687 Add import and export CSV flow for Members #48876 Add import and export CSV flow for Tags #48877
  • [@brunovjk] 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: I dont think this should be classified as a regression, as it seems more like an edge case that wasn’t considered across the three related PRs.
  • [@brunovjk] A discussion in #expensify-bugs 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:
  • [@brunovjk] Determine if we should create a regression test for this bug. Yes
  • [@brunovjk] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal:

  1. Open the members page as a non-admin.
  2. Click on the three-dot menu and select "Download CSV."
  3. Verify that the error modal appears.

  1. Have the owner and admin accounts open in different tabs.
  2. [Admin] Open the tags page of the workspace.
  3. [Owner] Update the admin to be just a member.
  4. [Admin] While still in the tags page, click on the three-dot menu and select "Download CSV."
  5. Verify that the error modal appears for the admin.

  1. Navigate to Workspace settings > Categories.
  2. Delete all categories.
  3. Tap the 3-dot menu.
  4. Verify that the "Download CSV" button does not appear.

  1. Navigate to Workspace settings > Tags.
  2. Delete all tags.
  3. Tap the 3-dot menu.
  4. Verify that the "Download CSV" button does not appear.

Do we agree 👍 or 👎?

cc: @Christinadobrzyn

@rlinoz
Copy link
Contributor

rlinoz commented Oct 7, 2024

I think we don't need this, we didn't have the tests for Applause when this happened and we have now, so it should be mostly covered.

@brunovjk
Copy link
Contributor

brunovjk commented Oct 7, 2024

Thanks for clarifying @rlinoz

@Christinadobrzyn
Copy link
Contributor

Thanks for the insight @rlinoz! It's payment day so I paid out based on this summary - #49022 (comment)

I'll not create the regression test based on #49022 (comment)

Also, @brunovjk and @cretadn22 there was a discussion about some payments through Upwork not going through - if you happen to notice this payment is missing will you please let me know. I assume it will take a week or so for the payment to show up but if you happen to notice anything strange, please let me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
Status: Done
Development

No branches or pull requests

10 participants