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-02-20] [$1000] [Polish] Add limit on length of task names, room names, etc. #18647

Closed
6 tasks done
kbecciv opened this issue May 9, 2023 · 193 comments
Closed
6 tasks done
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.

Comments

@kbecciv
Copy link

kbecciv commented May 9, 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!


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):

  • Workspace name
  • Task name
  • Room name

Supporting messages (500 characters):

  • Request description
  • Room welcome message
  • Task description

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?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

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

Bug6048269_Recording__4480.mp4

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
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 9, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 9, 2023

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

@melvin-bot

This comment was marked as off-topic.

@joekaufmanexpensify

This comment was marked as outdated.

@allroundexperts
Copy link
Contributor

Proposal

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

Assign Task field does not have limit on length

What is the root cause of that problem?

We don't have maxLength defined for the input here.

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

We should add a maxLength attribute with the required length that we want to restrict the users to here.

What alternative solutions did you explore? (Optional)

None

@dhairyasenjaliya
Copy link
Contributor

Proposal

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

  • Assign Task - There is no limit on the length of the title 

What is the root cause of that problem?

  • The root cause is we have not add any maxLength or any validation to check before form submission

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

  • The problem not only exists on New task page we need to modify this all files NewTaskDetailsPage.js , NewTaskTitlePage.js, NewTaskTitlePage.js , TaskTitlePage.js , NewTaskDescriptionPage.js , TaskDescriptionPage.js

  • To fix the problem we have 2 ways to solve 


solution 1

  • We can add maxLength (need to define limit) to both the field title and description which are inputID="taskTitle” and inputID="description" on all the above mentioned pages that are affected by infinite character limit

solution 2

  • Since we are using <Form> on validate() function we can just check length of the both field and based on that we can direct display error

What alternative solutions did you explore? (Optional)

  • None

@joekaufmanexpensify

This comment was marked as outdated.

@joekaufmanexpensify
Copy link
Contributor

Okay, we discussed this in Slack here, and I confirmed this is not a bug. We just didn't build this.

While it prob makes sense to add a limit at some point, we decided we don't need to prioritize this and it can be polish. Particularly because:

  • We also don't have a limit for room names, and this has caused no issues so far.
  • We don't allow multiple characters by holding down a key in the app. So if someone wants to make a task that's 100 characters long, they need to type 100 keys. It doesn't feel super realistic that a user would intentionally make a room or task name that's longer than any limit we decided to set.

When we do ultimately decide to do this, we'll want to consider all of the areas that don't have a limit, like task name, room name, etc. and add a limit to all of them.

@joekaufmanexpensify joekaufmanexpensify changed the title Assign Task - There is no limit on the length of the title [Polish] Add limit on length of task names, room names, etc. May 11, 2023
@joekaufmanexpensify joekaufmanexpensify added NewFeature Something to build that is a new item. and removed Bug Something is broken. Auto assigns a BugZero manager. labels May 11, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 11, 2023

Current assignee @joekaufmanexpensify is eligible for the NewFeature assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels May 11, 2023
@joekaufmanexpensify
Copy link
Contributor

Next steps here are to:

  1. identify anywhere in the app that needs a name limit.
  2. Figure out what we think that limit should be.
  3. Implement the limit in all of those places.

This is a minor polish issue, so will get to it in the next few weeks.

@dhairyasenjaliya
Copy link
Contributor

@joekaufmanexpensify we can add this one here also #18891

@joekaufmanexpensify
Copy link
Contributor

Sounds good!

@joekaufmanexpensify
Copy link
Contributor

Will prioritize this soon.

@joekaufmanexpensify
Copy link
Contributor

OOO next week, but then will pick this back up.

@dhairyasenjaliya
Copy link
Contributor

This can be added here #19821 (comment)

@dhairyasenjaliya
Copy link
Contributor

another potential GH to add it here #20137 @joekaufmanexpensify

@melvin-bot melvin-bot bot added the Overdue label Jun 5, 2023
@joekaufmanexpensify
Copy link
Contributor

Was OOO last week, going to look into this soon

@melvin-bot melvin-bot bot removed the Overdue label Jun 5, 2023
@melvin-bot melvin-bot bot added the Weekly KSv2 label Feb 12, 2024
@dhairyasenjaliya
Copy link
Contributor

@youssef-lr created PR above please check

@mananjadhav
Copy link
Collaborator

@dhairyasenjaliya Posted comment on the PR.

@dhairyasenjaliya
Copy link
Contributor

@mananjadhav all done

@youssef-lr
Copy link
Contributor

Merged, thanks folks. @joekaufmanexpensify for the payments, I don't think this should count as a regression.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Feb 13, 2024
@melvin-bot melvin-bot bot changed the title [$1000] [Polish] Add limit on length of task names, room names, etc. [HOLD for payment 2024-02-20] [$1000] [Polish] Add limit on length of task names, room names, etc. Feb 13, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Feb 13, 2024
Copy link

melvin-bot bot commented Feb 13, 2024

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

Copy link

melvin-bot bot commented Feb 13, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.40-5 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-02-20. 🎊

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

Copy link

melvin-bot bot commented Feb 13, 2024

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

@joekaufmanexpensify
Copy link
Contributor

@mananjadhav could you please propose regression test so we can prep to issue payment next week?

@joekaufmanexpensify joekaufmanexpensify added Daily KSv2 and removed Weekly KSv2 labels Feb 19, 2024
@melvin-bot melvin-bot bot added the Overdue label Feb 19, 2024
@joekaufmanexpensify
Copy link
Contributor

Payment due soon

@melvin-bot melvin-bot bot removed the Overdue label Feb 19, 2024
@mananjadhav
Copy link
Collaborator

@joekaufmanexpensify We have got multiple test cases for this one. I think we can reuse the ones mentioned in the Test steps of the PR.

There won't be any offending PR as this was more of a feature request.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Daily KSv2 labels Feb 20, 2024
@joekaufmanexpensify
Copy link
Contributor

Sounds good! Regression test issue shared above.

@joekaufmanexpensify
Copy link
Contributor

All set to issue payment here. We need to issue the following payments for this one:

@joekaufmanexpensify
Copy link
Contributor

@dhairyasenjaliya $1000 sent and contract ended!

@joekaufmanexpensify
Copy link
Contributor

@mananjadhav could you please request $1000 via NewDot and confirm once complete?

@mananjadhav
Copy link
Collaborator

Done @joekaufmanexpensify. Thanks for the bump.

@joekaufmanexpensify
Copy link
Contributor

Awesome. TY! Closing as this is all set. Thanks everyone!

@JmillsExpensify
Copy link

$1,000 approved for @mananjadhav based on summary above.

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 Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.
Projects
None yet
Development

No branches or pull requests