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-04-30][$500] Tag - Tag in Parent: Child format is displayed as Parent/: Child in Tag list #37847

Closed
6 tasks done
kbecciv opened this issue Mar 6, 2024 · 38 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Mar 6, 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: 1.4.48-0
Reproducible in staging?: y
Reproducible in production?: n
Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

  • User is an admin of Collect workspace with tags.
  • The tags include tag in Parent: Child format.
  1. Go to staging.new.expensify.com
  2. Go to Workspace settings > Collect workspace.
  3. Go to Tags.

Expected Result:

Tag in Parent: Child format is displayed correctly.

Actual Result:

Tag in Parent: Child format is displayed as Parent/: Child.

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

image

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01c364cca7b2db41ce
  • Upwork Job ID: 1767986259945525248
  • Last Price Increase: 2024-03-13
  • Automatic offers:
    • hoangzinh | Contributor | 0
@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Mar 6, 2024
Copy link
Contributor

github-actions bot commented Mar 6, 2024

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

Copy link

melvin-bot bot commented Mar 6, 2024

Triggered auto assignment to @marcaaron (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@kbecciv
Copy link
Author

kbecciv commented Mar 6, 2024

We think that this bug might be related to #wave8-collect-admins
@zanyrenney

@Tony-MK
Copy link
Contributor

Tony-MK commented Mar 6, 2024

Proposal

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

Tag - Tag in Parent: Child format is displayed as Parent/: Child in the Tag list

What is the root cause of that problem?

We are showing all types of policy tags even if they are multiple tags.

Object.values(policyTagList.tags).map((value) => ({
value: value.name,

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

We should filter out the policy multi-level tags from the policyTagList.tags by checking if the value.name equals the string result from the PolicyUtils.getCleanTagName function.

Object.values(
  policyTagList.tags.filter(
     (value) => value.name == getCleanTagName(value.name)).map((value) => ({
      ...

What alternative solutions did you explore? (Optional)

Create a regex to test if the tag is multi-level by checking if the Parent: Child format exits in the value.name string.

The regex will be similar to the regex the PolicyUtils.getCleanTagName function uses.

App/src/libs/PolicyUtils.ts

Lines 208 to 213 in 13f8bc0

/**
* Cleans up escaping of colons (used to create multi-level tags, e.g. "Parent: Child") in the tag name we receive from the backend
*/
function getCleanedTagName(tag: string) {
return tag?.replace(/\\{1,2}:/g, CONST.COLON);
}

Optional

Implement one of the above solutions in the PolicyUtils.getTagLists function and create an argument called shouldExcudeMultiTags, defaulted to false, to exclude the policy's multi-level tags from the result array.

Additional

To handle possible regressions, in this line, let's check the tagList.length instead of policyTags.

@allgandalf
Copy link
Contributor

allgandalf commented Mar 6, 2024

Proposal

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

Child format is displayed as Parent/: Child in Tag list

What is the root cause of that problem?

We flatten out the array of tags:

policyTagLists
.map((policyTagList) =>
Object.values(policyTagList.tags).map((value) => ({
value: value.name,
text: value.name,
keyForList: value.name,
isSelected: !!selectedTags[value.name],
rightElement: (
<View style={styles.flexRow}>
<Text style={[styles.disabledText, styles.alignSelfCenter]}>
{value.enabled ? translate('workspace.common.enabled') : translate('workspace.common.disabled')}
</Text>
<View style={[styles.p1, styles.pl2]}>
<Icon
src={Expensicons.ArrowRight}
fill={theme.icon}
/>
</View>
</View>
),
})),
)
.flat(),

See at the end of the code above

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

We should not do this, we should follow similar pattern as we do with categories and remove the flat() funciton

Considering that both tags and categories are on the same option menu, it would be good to remove flat() function to maintain consistency in codebase.

What alternative solutions did you explore? (Optional)

Considering the comments, the updated solution is as follows:

As this is a temporary fix, i suggest that we shouldn't update the util function, instead we should modify the FE as follows, we should update the mapping:

 const tagList = useMemo<PolicyForList[]>(
     () =>
         policyTagLists
             .map((policyTagList) =>
                 Object.values(policyTagList.tags || []).map((value) => {
+                    let cleanValue = value.name;
+                    const colonIndex = cleanValue.indexOf('\\:');
+                    if (colonIndex !== -1 && colonIndex > 0) {
+                        cleanValue = cleanValue.substring(0, colonIndex);
+                    }
                     return {
+                        value: cleanValue,
+                        text: cleanValue,
+                        keyForList: cleanValue,
                         isSelected: !!selectedTags[value.name],
                         rightElement: (
                             <View style={styles.flexRow}>
                                 <Text style={[styles.textSupporting, styles.alignSelfCenter, styles.pl2, styles.label]}>
                                     {value.enabled ? translate('workspace.common.enabled') : translate('workspace.common.disabled')}
                                 </Text>
                                 <View style={[styles.p1, styles.pl2]}>
                                     <Icon
                                         src={Expensicons.ArrowRight}
                                         fill={theme.icon}
                                     />
                                 </View>
                             </View>
                         ),
                     };
                 })
             )
             .flat(),
     [policyTagLists, selectedTags, styles.alignSelfCenter, styles.flexRow, styles.label, styles.p1, styles.pl2, styles.textSupporting, theme.icon, translate],
 );

@marcaaron
Copy link
Contributor

cc @luacmartins @waterim might be able to triage this faster than me. Will take a look soon if not.

@luacmartins
Copy link
Contributor

we're discussing this internally since multi-level tags were out of scope for the doc

@luacmartins luacmartins assigned luacmartins and unassigned marcaaron Mar 7, 2024
@luacmartins
Copy link
Contributor

We've discussed this internally and the solution we're going with is to just hide any tags that are on level 2 or above, so we only want to display the root level for now.

@luacmartins
Copy link
Contributor

We're still discussing some nuances for this case. I'll circle back once we made a decision on the expected behavior

@luacmartins
Copy link
Contributor

@kbecciv how did you set up that Parent: Child tag?

@roryabraham
Copy link
Contributor

@luacmartins is this a blocker? Tags are a collect policy feature and thus in order to use them in NewDot you have to run a JS snippet in OldDot. Right?

@luacmartins
Copy link
Contributor

Yea, let's demote this.

@luacmartins luacmartins added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Mar 7, 2024
@luacmartins
Copy link
Contributor

luacmartins commented Mar 7, 2024

@kbecciv bump on this comment

how did you set up that Parent: Child tag?

@melvin-bot melvin-bot bot added the Overdue label Mar 11, 2024
Copy link

melvin-bot bot commented Mar 11, 2024

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

@luacmartins
Copy link
Contributor

This is a polish item, will make external

@melvin-bot melvin-bot bot removed the Overdue label Mar 13, 2024
@luacmartins luacmartins added the Bug Something is broken. Auto assigns a BugZero manager. label Mar 13, 2024
Copy link

melvin-bot bot commented Mar 18, 2024

❌ There was an error making the offer to @Tony-MK for the Contributor role. The BZ member will need to manually hire the contributor.

Copy link

melvin-bot bot commented Mar 20, 2024

@luacmartins @Tony-MK @allroundexperts @adelekennedy this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@melvin-bot melvin-bot bot added the Overdue label Mar 20, 2024
@adelekennedy
Copy link

@Tony-MK manually invited you to the job on Upwork!

@melvin-bot melvin-bot bot removed the Overdue label Mar 20, 2024
@allroundexperts
Copy link
Contributor

@luacmartins I think this issue would solve #38683

@luacmartins
Copy link
Contributor

Makes sense. I'll put that on hold

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Mar 21, 2024
@Tony-MK
Copy link
Contributor

Tony-MK commented Mar 22, 2024

@allroundexperts, the PR is ready review.

@luacmartins
Copy link
Contributor

Copy link

melvin-bot bot commented Mar 29, 2024

📣 @hoangzinh 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Apr 22, 2024
Copy link

melvin-bot bot commented Apr 22, 2024

This issue has not been updated in over 15 days. @hoangzinh, @luacmartins, @Tony-MK, @adelekennedy 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!

@hoangzinh
Copy link
Contributor

The fix has been landed in Prod #38759 (comment)

@luacmartins luacmartins changed the title [$500] Tag - Tag in Parent: Child format is displayed as Parent/: Child in Tag list [HOLD for payment 2024-04-30][$500] Tag - Tag in Parent: Child format is displayed as Parent/: Child in Tag list Apr 23, 2024
@luacmartins luacmartins added Awaiting Payment Auto-added when associated PR is deployed to production Weekly KSv2 and removed Reviewing Has a PR in review Monthly KSv2 labels Apr 23, 2024
@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 29, 2024
@adelekennedy
Copy link

payments made - @luacmartins are we good to close?

@luacmartins
Copy link
Contributor

luacmartins commented Apr 30, 2024

Yes, should be good to close. Thanks!

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. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
Archived in project
Development

No branches or pull requests

9 participants