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

[$500] Web - It's possible to create a tag name "0", the app will crash when it's deleted #45016

Closed
2 of 6 tasks
lanitochka17 opened this issue Jul 8, 2024 · 60 comments
Closed
2 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Jul 8, 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.5-4
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:
Slack conversation:

Issue found when executing PR #44391

Action Performed:

  1. Go to www.staging.chat.expensify.com
  2. Create a new workspace and enable Tags
  3. Create tag with "0" name
  4. Try to delete this Tag

Expected Result:

Not possible to create tag with "0" name, when it fails, verify the tag name reverts to the previous name
Verify the error is cleared if close button is pressed

Actual Result:

It's possible to create a tag name "0", no error appears.
The app will crash when it's deleted.

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

Add any screenshot/video evidence

Bug6536264_1720468537392.Screen_Recording_2024-07-08_at_22.51.31.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~012c39ad7db952ea6b
  • Upwork Job ID: 1811206201231781305
  • Last Price Increase: 2024-09-09
  • Automatic offers:
    • ahmedGaber93 | Reviewer | 103558323
    • Krishna2323 | Contributor | 103558324
Issue OwnerCurrent Issue Owner: @isabelastisser
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 8, 2024
Copy link

melvin-bot bot commented Jul 8, 2024

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

@lanitochka17
Copy link
Author

@isabelastisser FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@lanitochka17
Copy link
Author

We think that this bug might be related to #wave-collect - Release 1

@Krishna2323
Copy link
Contributor

Krishna2323 commented Jul 8, 2024

Proposal

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

Web - It's possible to create a tag name "0", the app will crash when it's deleted

What is the root cause of that problem?

We don't prevent users from adding tag with name 0

const validate = useCallback(
(values: FormOnyxValues<typeof ONYXKEYS.FORMS.WORKSPACE_TAG_FORM>) => {
const errors: FormInputErrors<typeof ONYXKEYS.FORMS.WORKSPACE_TAG_FORM> = {};
const tagName = values.tagName.trim();
const {tags} = PolicyUtils.getTagList(policyTags, 0);
if (!ValidationUtils.isRequiredFulfilled(tagName)) {
errors.tagName = translate('workspace.tags.tagRequiredError');
} else if (tags?.[tagName]) {
errors.tagName = translate('workspace.tags.existingTagError');
} else if ([...tagName].length > CONST.TAG_NAME_LIMIT) {
// Uses the spread syntax to count the number of Unicode code points instead of the number of UTF-16 code units.
ErrorUtils.addErrorMessage(errors, 'tagName', translate('common.error.characterLimitExceedCounter', {length: [...tagName].length, limit: CONST.TAG_NAME_LIMIT}));
}
return errors;
},
[policyTags, translate],
);

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

We can check if the tag name is 0 and show an error. We can create new translation for that. We should also do that when editing the tag.

What alternative solutions did you explore? (Optional)

Result

Monosnap.screencast.2024-07-09.04-08-52.mp4

@bernhardoj
Copy link
Contributor

BE issue, the API response returns the tags as an array of null.

Screenshot 2024-07-09 at 11 43 47

cc: @danieldoglas

@danieldoglas
Copy link
Contributor

hm, I think the backend changes didn't go out yet, so we might need to retest this after we deploy the beckend

@isabelastisser isabelastisser added Internal Requires API changes or must be handled by Expensify staff retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause labels Jul 11, 2024
@melvin-bot melvin-bot bot added the Overdue label Jul 11, 2024
@isabelastisser isabelastisser added Weekly KSv2 and removed Daily KSv2 labels Jul 11, 2024
@melvin-bot melvin-bot bot removed the Overdue label Jul 11, 2024
@isabelastisser
Copy link
Contributor

It's not overdue; waiting to retest this!

@isabelastisser isabelastisser added External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors and removed Internal Requires API changes or must be handled by Expensify staff labels Jul 11, 2024
@melvin-bot melvin-bot bot changed the title Web - It's possible to create a tag name "0", the app will crash when it's deleted [$250] Web - It's possible to create a tag name "0", the app will crash when it's deleted Jul 11, 2024
Copy link

melvin-bot bot commented Jul 11, 2024

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

@isabelastisser isabelastisser moved this to Release 1: Spring 2024 (May) in [#whatsnext] #wave-collect Jul 11, 2024
Copy link

melvin-bot bot commented Jul 11, 2024

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 11, 2024
@ahmedGaber93
Copy link
Contributor

@danieldoglas I don't know if backend deployed or not, but I test this today and I see backend return incorrect data.

API CreatePolicyTag response for tag '0' (tags type is array).

Screenshot 2024-07-16 at 8 35 49 AM

API CreatePolicyTag response for tag '2' (tags type is object).

Screenshot 2024-07-16 at 8 35 28 AM

Copy link

melvin-bot bot commented Jul 18, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Jul 18, 2024
@trjExpensify
Copy link
Contributor

trjExpensify commented Sep 5, 2024

@danieldoglas @isabelastisser can we consolidate all of these "tag 0" issues into here? it seems crazy we have so many issues for this edge case that I highly doubt any real world customer will actually configure. Namely, these:

@danieldoglas
Copy link
Contributor

Yes, let's add them all here.

@Krishna2323 can you also add that validation for custom tags?

@Krishna2323
Copy link
Contributor

@danieldoglas, sure, but please allow me 2-3 days as I have some open PRs to work on. I think this is and edge case so we can wait a little.

@VictoriaExpensify
Copy link
Contributor

Here's another one - #48584

I'm going to go ahead and close that one out

@danieldoglas
Copy link
Contributor

bump @Krishna2323, let me know when you have a PR for those.

also, I understand that this is a different situation than what was initially proposed and (correctly) fixed, so I'll raise this to $500.

@danieldoglas danieldoglas changed the title [$250] Web - It's possible to create a tag name "0", the app will crash when it's deleted [$500] Web - It's possible to create a tag name "0", the app will crash when it's deleted Sep 9, 2024
Copy link

melvin-bot bot commented Sep 9, 2024

Upwork job price has been updated to $500

@Krishna2323
Copy link
Contributor

Thanks a lot for the raise🙏🏻, I will have the PR up within 12 hours.

@melvin-bot melvin-bot bot removed the Weekly KSv2 label Sep 9, 2024
Copy link

melvin-bot bot commented Sep 9, 2024

This issue has not been updated in over 15 days. @danieldoglas, @isabelastisser, @ahmedGaber93, @Krishna2323 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@melvin-bot melvin-bot bot added the Monthly KSv2 label Sep 9, 2024
@trjExpensify trjExpensify added Weekly KSv2 and removed Monthly KSv2 labels Sep 10, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Sep 10, 2024
@Krishna2323
Copy link
Contributor

@ahmedGaber93, this PR is ready for review.

@Krishna2323
Copy link
Contributor

@danieldoglas, should we use the same error message for custom tag name field also?

Tag name cannot be 0. Please choose a different value.

@Krishna2323
Copy link
Contributor

@danieldoglas @ahmedGaber93, do you have any idea if this PR has been deployed to production or not? These is no sign from melvin bot on the PR.

@danieldoglas
Copy link
Contributor

hmm that's strange. I think it was, since it was part of the checklist as well. @ahmedGaber93 can you test if it's working in production as it should? If yes I'll add the awaiting for payment label.

@ahmedGaber93
Copy link
Contributor

@danieldoglas, yes it works on production.

@danieldoglas danieldoglas added Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review labels Sep 20, 2024
@danieldoglas
Copy link
Contributor

@isabelastisser all yours for payment!

@isabelastisser
Copy link
Contributor

Paid in Upwork. All set!

@ahmedGaber93
Copy link
Contributor

@isabelastisser This issue is $500 price not $250

@isabelastisser
Copy link
Contributor

Hi @ahmedGaber93, I'm sorry for the confusion! I thought that the price increase was only applied to the C+ review. I will adjust the offer price in Upwork now.

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. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
No open projects
Status: Done
Development

No branches or pull requests

10 participants