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 2023-12-21] [$500] [Wave 6: Categories] - Unexpected error when requesting money with a very long category in workspace chat #30683

Closed
6 tasks done
kbecciv opened this issue Nov 1, 2023 · 62 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Nov 1, 2023

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: 1.3.94.0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Issue found when executing PR #30264

Action Performed:

Precondition:
The following long category is set up on OldDot.

Meals: Level 2 nesting and a category name that could span two lines: Level 3 nesting and a category name that could span two lines: Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.

  1. Go to workspace chat > + Request money > Manual.
  2. Create a money request with no category.
  3. Note that the request is successful.
  4. Repeat Step 2 with a very long category name from the precondition.

Expected Result:

The request is successful.

Actual Result:

The request is not successful. It shows an unexpected error message.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

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

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Bug6259289_1698837857753.20231101_100027.mp4
MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~018cfdce1c85555976
  • Upwork Job ID: 1719689005334507520
  • Last Price Increase: 2023-11-01
  • Automatic offers:
    • Ollyws | Reviewer | 27576838
@kbecciv kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 1, 2023
@melvin-bot melvin-bot bot changed the title Category - Unexpected error when requesting money with a very long category in workspace chat [$500] Category - Unexpected error when requesting money with a very long category in workspace chat Nov 1, 2023
Copy link

melvin-bot bot commented Nov 1, 2023

Triggered auto assignment to @stephanieelliott (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

Copy link

melvin-bot bot commented Nov 1, 2023

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

Copy link

melvin-bot bot commented Nov 1, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

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

melvin-bot bot commented Nov 1, 2023

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

@ikevin127
Copy link
Contributor

I was able to reproduce this and I confirm that the RequestMoney call fails with the following error:
Screenshot 2023-11-01 at 21 53 58

Which means that the long category created on OldDot turns out to be exceeding maximum size when the NewDot's RequestMoney is called with that category. I think this is a BE related issue.

@NisargDeveloper
Copy link

Hey there shouldn't be any proper handling for requesting money with category. We can use if and AND condition to solve this issue. Let me know if you need a help.

Contributor details
Your Expensify account email: nisarg.k.amin@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01149c9659573bccce

Copy link

melvin-bot bot commented Nov 2, 2023

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@melvin-bot melvin-bot bot added the Overdue label Nov 3, 2023
Copy link

melvin-bot bot commented Nov 6, 2023

@Ollyws, @stephanieelliott Huh... This is 4 days overdue. Who can take care of this?

@NisargDeveloper
Copy link

Sure... I can help you

@stephanieelliott
Copy link
Contributor

Hey @NisargDeveloper if you'd like to work on this, can you please post a proposal per the instructions in https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md?

@melvin-bot melvin-bot bot removed the Overdue label Nov 6, 2023
@NisargDeveloper
Copy link

NisargDeveloper commented Nov 7, 2023

hey! @stephanieelliott I tried to reproduce it. I'm thinking I'm missing step. May I know how can I setup Old Dot?.

@stephanieelliott
Copy link
Contributor

stephanieelliott commented Nov 8, 2023

@NisargDeveloper can you lend context into which step you are getting hung up on? Perhaps @kbecciv can help clarify

@rezkiy37
Copy link
Contributor

rezkiy37 commented Nov 8, 2023

Hi, I’m Michael (Mykhailo) from Callstack and I would like to work on this issue.

@kbecciv
Copy link
Author

kbecciv commented Nov 8, 2023

@NisargDeveloper Here is instruction how to set up Category in Olddot
image

@NisargDeveloper
Copy link

Hey I cannot see the Create Policy on the left side? Please see the attached ss.

image

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

melvin-bot bot commented Nov 8, 2023

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

@mountiny
Copy link
Contributor

mountiny commented Nov 8, 2023

Assigning @rezkiy37 as they worked on these features and will handle this update too

@rezkiy37
Copy link
Contributor

rezkiy37 commented Nov 8, 2023

So, guys, looks like we have a collision regarding the maximum length of name for a category.

We can add a category up to 256 symbols on the OD, 257 symbols are too long. However, when we create a money request on ND, the backend does not accept a category with 256 symbols name.

ND
OD.mp4

Based on this, looks like we need to unify backend restrictions for name's length.

cc @mountiny, @puneetlath

@melvin-bot melvin-bot bot added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor labels Nov 29, 2023
Copy link

melvin-bot bot commented Nov 29, 2023

Current assignee @Ollyws is eligible for the Internal assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added the Overdue label Dec 1, 2023
@aldo-expensify
Copy link
Contributor

I noticed that this is also broken for OldDot. I'm asking in slack to confirm the path we want to take to fix this: https://expensify.slack.com/archives/C02MW39LT9N/p1701396084292819

@melvin-bot melvin-bot bot removed the Overdue label Dec 1, 2023
@aldo-expensify
Copy link
Contributor

We can't increase the category max length beyond 255, so we are going with the solution of improving the error feedback.

Working on improving the error returned by the backend, it feels a bit junky the experience because the modal closes and then an error appears in the chat. I think a better and less confusing UX is to have the limit as a form validation that won't allow you to submit the expense.

@aldo-expensify
Copy link
Contributor

I put up a draft PR. The solution looks like this:

Screen.Recording.2023-12-01.at.4.56.24.PM.mov

Please let me know if you think that is fine to finish the PR (description/videos).

@melvin-bot melvin-bot bot added the Overdue label Dec 4, 2023
@rezkiy37
Copy link
Contributor

rezkiy37 commented Dec 4, 2023

@aldo-expensify, so, isn't there a way to align the limitation on the backend side? I looks odd that user can add a category, but cannot use. However, if really no way, then I agree with the detailed message.

@melvin-bot melvin-bot bot removed the Overdue label Dec 4, 2023
@aldo-expensify
Copy link
Contributor

isn't there a way to align the limitation on the backend side?

I'm guessing you are referring to rejecting in the backend (and frontend) a user creating a category longer than 255 because they won't be able to use it anyway.

I think both things can be done independently: the validation proposed above and disallow creating long categories.

The problem with implementing the second part is that categories can be added manually and also automatically from integrations with external services. The part of the integrations can get complicated if we want to provide some feedback about why some categories didn't load. Also, we would have to think about what would we do with long categories that are already in the database.

Considering the complexity of that second part, and that this is an edge case (someone very very very rarely will add such long category), I don't think it is worth it to try the second part and we should only do the proposed front end validation when using the category.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Dec 5, 2023
@aldo-expensify
Copy link
Contributor

Made PR ready for review: #32394

@rezkiy37
Copy link
Contributor

rezkiy37 commented Dec 5, 2023

Got you, makes sense. Thank you 🙂

@stephanieelliott
Copy link
Contributor

This issue didn't update but the PR was pushed to prod on 12/14. Adjusting title to remember to issue payment after 7 day hold.

@stephanieelliott stephanieelliott changed the title [$500] [Wave 6: Categories] - Unexpected error when requesting money with a very long category in workspace chat [HOLD for payment 2023-12-21] [$500] [Wave 6: Categories] - Unexpected error when requesting money with a very long category in workspace chat Dec 19, 2023
@stephanieelliott stephanieelliott added the Awaiting Payment Auto-added when associated PR is deployed to production label Dec 19, 2023
Copy link

melvin-bot bot commented Dec 21, 2023

Issue is ready for payment but no BZ is assigned. @dylanexpensify you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

@stephanieelliott
Copy link
Contributor

Summarizing payment on this issue:

  • Contributor: no payment, @rezkiy37 with Callstack
  • Contributor+: $500, to pay via Upwork - @Ollyws I've extended the offer to you on Upwork, can you please accept?

Upwork job is here: https://www.upwork.com/jobs/~018cfdce1c85555976

@Ollyws
Copy link
Contributor

Ollyws commented Dec 21, 2023

Accepted, thanks!

@stephanieelliott
Copy link
Contributor

All paid, thank you!

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. Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2
Projects
No open projects
Development

No branches or pull requests