Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.03k stars 2.54k forks source link

[HOLD for payment 2024-02-20] [$1000] [Polish] Add limit on length of task names, room names, etc. #18647

Closed kbecciv closed 3 months ago

kbecciv commented 1 year ago

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


Action Performed:

  1. Go to URL https://staging.new.expensify.com/
  2. Login with any Expensifail account
  3. Go to FAB -> Assign task
  4. Start typing the title and note that there is no symbol limit

Expected Result:

After discussion in Slack, we want to make the following field limit updates:

Titles (100 characters):

Supporting messages (500 characters):

Actual Result:

There is no limit on the length of the title. Same behavior for Description

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.12.0

Reproducible in staging?: Yes

Reproducible in production?: yes

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

Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/05b9957a-b07e-4625-9e73-3b720dea6b93

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~018a42c5fcacc50150
  • Upwork Job ID: 1686014963280871424
  • Last Price Increase: 2023-07-31
  • Automatic offers:
    • dhairyasenjaliya | Contributor | 26010517
dhairyasenjaliya commented 9 months ago

@youssef-lr Also for WorkspaceName and RoomName we need to set the character limit to 100 as well currently both have 80 from the backend so this required backend changes as well

dhairyasenjaliya commented 9 months ago

@youssef-lr @mananjadhav I have created a draft PR but do we hold this issue until backend limit fix for WorkspaceName and RoomName ?

mananjadhav commented 9 months ago

@dhairyasenjaliya I think you can complete the PR.

@joekaufmanexpensify @youssef-lr Can you folks help with the backend change here?

joekaufmanexpensify commented 9 months ago

Sure! @youssef-lr since the auto-assigner selected you for the proposal review, do you want to handle the backend changes for the field limits too?

youssef-lr commented 9 months ago

Sure thing!

joekaufmanexpensify commented 9 months ago

Awesome, ty!

joekaufmanexpensify commented 9 months ago

Next step here is internal PR!

mananjadhav commented 9 months ago

@dhairyasenjaliya Still waiting for an update on this one.

dhairyasenjaliya commented 9 months ago

Hey @mananjadhav im waiting for backend fix once done will push code

mananjadhav commented 9 months ago

@dhairyasenjaliya As I've mentioned before we can finish the PR on the frontend. It isn't that this is introducing the bug currently? This anyway exists today, that backend has limit but we don't have on the frontend.

I don't see a need to block the App PR. Thanks.

dhairyasenjaliya commented 9 months ago

Hm @mananjadhav if we add limit of 100 then we want able to save it as it will throw invalid name an backend error

mananjadhav commented 9 months ago

and what happens today if we enter the text of length 100?

dhairyasenjaliya commented 9 months ago

On live there is limit of 80 on both backend and front end so we cant add 100

mananjadhav commented 9 months ago

Okay got it. I thought we didn't have the limit on the frontend. Thanks for correcting me. @joekaufmanexpensify can we put this on Hold until we have the backend fix up?

dhairyasenjaliya commented 9 months ago

also @youssef-lr can you check if we have a limit backend for the legal name since we also need to change the first/last legal names to support 50 characters each as per below thread https://expensify.slack.com/archives/C01GTK53T8Q/p1691756214835199?thread_ts=1686863510.616919&cid=C01GTK53T8Q

joekaufmanexpensify commented 9 months ago

@mananjadhav should it be on hold though? We're going to handle the internal PR in this issue too.

joekaufmanexpensify commented 9 months ago

@youssef-lr could you share an ETA for when you expect the internal PR to be up?

mananjadhav commented 9 months ago

Yeah it makes sense to put this on Hold. We could split them into separate PRs, but I don't think it's worth the effort as backend fix should be a small one. @youssef-lr You agree?

joekaufmanexpensify commented 9 months ago

Got it. I'd think the issue would not be on hold though. The frontend PR is held on the backend one. But the backend one is still being managed in this issue too. Could we just put that PR on hold?

I'm not sure it makes sense to hold the entire issue, as part of it can be worked on now.

mananjadhav commented 9 months ago

Okay sure I thought we create separate issues for backend. Thanks for correcting. Then it makes sense to put the PR on Hold. cc - @dhairyasenjaliya

dhairyasenjaliya commented 9 months ago

yup let's hold until the backend fix then I will adjust frontEnd PR here

joekaufmanexpensify commented 9 months ago

Chatted with @youssef-lr and he confirmed he's been resolving some issues with his dev environment, and PR will be up later today/tomorrow.

melvin-bot[bot] commented 9 months ago

@mananjadhav, @youssef-lr, @dhairyasenjaliya, @joekaufmanexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 9 months ago

@mananjadhav, @youssef-lr, @dhairyasenjaliya, @joekaufmanexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

joekaufmanexpensify commented 9 months ago

@youssef-lr could you share an ETA to the issue of when the internal PR will be up here?

daordonez11 commented 9 months ago

Hey guys coming from https://github.com/Expensify/App/issues/25494, please include in the polish the Description and the merchant of the request. As per discussed here: https://expensify.slack.com/archives/C01GTK53T8Q/p1692723237220349

cc @dhairyasenjaliya @mananjadhav @joekaufmanexpensify @mountiny

joekaufmanexpensify commented 9 months ago

@mananjadhav bumped the discussion here.

joekaufmanexpensify commented 9 months ago

Latest ETA for new PR is tomorrow!

melvin-bot[bot] commented 9 months ago

@mananjadhav, @youssef-lr, @dhairyasenjaliya, @joekaufmanexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

joekaufmanexpensify commented 9 months ago

Not overdue.

Santhosh-Sellavel commented 9 months ago

We closed https://github.com/Expensify/App/issues/26123 in favor of this issue. It makes sense to handle it here, please take of it.

cc: @mananjadhav

joekaufmanexpensify commented 9 months ago

I bumped the slack discussion again.

joekaufmanexpensify commented 9 months ago

Still held on internal PR.

joekaufmanexpensify commented 9 months ago

Same

joekaufmanexpensify commented 9 months ago

@youssef-lr could you please share an update on this when you have a chance? TY!

youssef-lr commented 9 months ago

@joekaufmanexpensify I'm gonna be able to get to this one after the launch of TU, which is in a couple of days, promise!

joekaufmanexpensify commented 9 months ago

Cool cool, sounds great, ty!

joekaufmanexpensify commented 9 months ago

We'll get to this one later this week, once TU is done

joekaufmanexpensify commented 8 months ago

Same

joekaufmanexpensify commented 8 months ago

We're discussing some error handling considerations here that will impact how we proceed with fixing this issue.

joekaufmanexpensify commented 8 months ago

@youssef-lr could you please confirm once you're ready to proceed with this one?

youssef-lr commented 8 months ago

Working on the PR @joekaufmanexpensify!

joekaufmanexpensify commented 8 months ago

Sounds good. We are discussing adding a character counter as an error message for when the user hits the limit, so they'd be able to see that they have exceeded the limit, but just not be cut off from typing.

That would be something to incorporate into the frontend PR though, right?

joekaufmanexpensify commented 8 months ago

Still pending further discussion on the above point.

youssef-lr commented 8 months ago

That would be something to incorporate into the frontend PR though, right?

Correct, this can be made external.

joekaufmanexpensify commented 8 months ago

Got it, ty!

joekaufmanexpensify commented 8 months ago

Not overdue

joekaufmanexpensify commented 8 months ago

Still ironing out specifics of character counter in the above linked issue.

joekaufmanexpensify commented 8 months ago

Same

joekaufmanexpensify commented 8 months ago

Still discussing the character limit error patterns in the above issue.