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.34k stars 2.77k forks source link

[$250] Tags - Existing tags are restructured when custom tag name is 0 and adding 0: 0 as tag #48300

Closed IuliiaHerets closed 23 hours ago

IuliiaHerets commented 3 weeks 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!


Version Number: v9.0.26-3 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Exp https://expensify.testrail.io/index.php?/tests/view/4902717 Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to workspace settings > Tags.
  3. Add a few tags.
  4. Click Settings > Custom tag name.
  5. Enter 0 as custom tag name.
  6. Add 0: 0 as tag.

Expected Result:

Nothing will happen to the existing tags.

Actual Result:

The existing tags are duplicated and restructured - one set falls under 0: 0 as multi tags, the other set is regular tag.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/704508e7-484d-430e-a4b3-fb5955f30e4f

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021831485723893012269
  • Upwork Job ID: 1831485723893012269
  • Last Price Increase: 2024-09-05
Issue OwnerCurrent Issue Owner: @mananjadhav
melvin-bot[bot] commented 3 weeks ago

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

IuliiaHerets commented 3 weeks ago

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

Nodebrute commented 3 weeks ago

Edited by proposal-police: This proposal was edited at 2024-08-29 21:30:38 UTC.

Proposal

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

Existing tags are restructured when custom tag name is 0 and adding 0: 0 as tag

What is the root cause of that problem?

We don't check when user add 0 as a custom tag name. It's a similar issue to this https://github.com/Expensify/App/issues/45016. The backend response is null


key
: 
"policyTags_8FE3CA458B14BEF9"
onyxMethod
: 
"merge"
value
: 
[null]
0
: 
null

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

We should stop user from adding 0 as a custom tag name. We can go for similar approach that we do here https://github.com/Expensify/App/blob/8c0386a562ac8ecdc0aa8e0496c8ed24971a5089/src/pages/workspace/tags/WorkspaceCreateTagPage.tsx#L48 We can throw error when user tries to enter 0 in custom tag. We can also add translations in en.ts and es.ts files for the error.

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 2 weeks ago

@adelekennedy Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 2 weeks ago

📣 @nihatuzlaci! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
nihatuzlaci commented 2 weeks ago

Contributor details Your Expensify account email: senius.33@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~011416d6c42e4e4d8c

The account belongs to my partner and I have consent to hire him for the payments.

melvin-bot[bot] commented 2 weeks ago

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

nihatuzlaci commented 2 weeks ago

Proposal

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

Adding "0" as a custom tag name restructuring the existing tags.

What is the root cause of that problem?

Adding "0" as a custom tag causes restructuring tag list.

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

We should add an if block to check adding "0" as a custom tag name and throw an error when the user tries to add it.

https://github.com/Expensify/App/blob/562d7b48226247253625b9ac1fd45266cb53eda8/src/pages/workspace/tags/WorkspaceEditTagsPage.tsx#L39-L51

What alternative solutions did you explore? (Optional)

eucool commented 2 weeks ago

Proposal

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

Allowed to save custom Tag name as 0.

What is the root cause of that problem?



We do not have any validation in place to check for custom tag name here, this is why we do not get any validation errors here 



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

Like we do for regular tag names, we can add a validation for custom tag names as well:

https://github.com/Expensify/App/blob/f890c73877b5b5c8b524793cb09695bf0be904c5/src/pages/workspace/tags/EditTagPage.tsx#L48-L49

So we have to update the code here to:

const validateTagName = useCallback(
        (values: FormOnyxValues<typeof ONYXKEYS.FORMS.POLICY_TAG_NAME_FORM>) => {
            const errors: FormInputErrors<typeof ONYXKEYS.FORMS.POLICY_TAG_NAME_FORM> = {};
            .
            .
            if (values[INPUT_IDS.POLICY_TAGS_NAME].trim() === '0'){
              errors[INPUT_IDS.POLICY_TAGS_NAME] = translate('workspace.tags.invalidTagNameError');
            }

           .
           .
           .
            return errors;
        },
        [translate, policyTags, route.params.orderWeight],
    );

We can look at any other parts of the code where these changes are needed and also small improvements can be done during PR phase

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

adelekennedy commented 2 weeks ago

Something else I see is that even after renaming the custom tag it still shows up as the old name

https://github.com/user-attachments/assets/1ce347d3-bd2b-46f3-a6ca-59ab3b309443

trjExpensify commented 2 weeks ago

https://github.com/Expensify/App/issues/45016#issuecomment-2331356711 - I think this is getting a bit ridiculous with all these issues related to setting a tag name to 0. Asking to consolidate and close these out.

danieldoglas commented 23 hours ago

Done as part of https://github.com/Expensify/App/issues/45016