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.

Sign up for GitHub

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

Jump to bottom

Fix opening tag with colon shows not found page while opening for the first time #42381

Merged
merged 5 commits into from
May 29, 2024
Merged

Fix opening tag with colon shows not found page while opening for the first time #42381

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4b9c913
escape colon from tag name
bernhardoj May 20, 2024
e073da2
lint
bernhardoj May 20, 2024
8b0abd5
pass the escaped tag name
bernhardoj May 22, 2024
aafc055
add missing deps
bernhardoj May 22, 2024
6e97610
Merge branch 'main' into fix/41083-escape-colon-from-tag-name
bernhardoj May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/libs/PolicyUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,13 @@ function getCleanedTagName(tag: string) {
return tag?.replace(/\\{1,2}:/g, CONST.COLON);
}

/**
* Escape colon from tag name
*/
function escapeTagName(tag: string) {
return tag?.replaceAll(CONST.COLON, '\\:');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-05-20 at 17 20 27

@bernhardoj I see there are many cases the BE replace : by \:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify what you are trying to request?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You assume that the BE will replace : with \\: and I see there are some exceptional

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bernhardoj If you enter the tag name is a:b the BE will return a\:b and the optimistic data in your change will return a\\:b

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you see any pattern in how the BE replaces the colon?

cc @amyevans

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we need double \\ to escape the \. You can expand the object and see the name contains 2 \.
image

If you do 'a:b'.replaceAll(':', '\:'), nothing is changed.

}

/**
* Gets a count of enabled tags of a policy
*/
Expand Down Expand Up @@ -411,6 +418,7 @@ function getCurrentXeroOrganizationName(policy: Policy | undefined): string | un
export {
canEditTaxRate,
extractPolicyIDFromPath,
escapeTagName,
getActivePolicies,
getAdminEmployees,
getCleanedTagName,
Expand Down
33 changes: 18 additions & 15 deletions src/libs/actions/Policy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3405,6 +3405,7 @@ function renamePolicyCategory(policyID: string, policyCategory: {oldName: string

function createPolicyTag(policyID: string, tagName: string) {
const policyTag = PolicyUtils.getTagLists(allPolicyTags?.[`${ONYXKEYS.COLLECTION.POLICY_TAGS}${policyID}`] ?? {})?.[0] ?? {};
const newTagName = PolicyUtils.escapeTagName(tagName);

const onyxData: OnyxData = {
optimisticData: [
Expand All @@ -3414,8 +3415,8 @@ function createPolicyTag(policyID: string, tagName: string) {
value: {
[policyTag.name]: {
tags: {
[tagName]: {
name: tagName,
[newTagName]: {
name: newTagName,
enabled: true,
errors: null,
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
Expand All @@ -3432,7 +3433,7 @@ function createPolicyTag(policyID: string, tagName: string) {
value: {
[policyTag.name]: {
tags: {
[tagName]: {
[newTagName]: {
errors: null,
pendingAction: null,
},
Expand All @@ -3448,7 +3449,7 @@ function createPolicyTag(policyID: string, tagName: string) {
value: {
[policyTag.name]: {
tags: {
[tagName]: {
[newTagName]: {
errors: ErrorUtils.getMicroSecondOnyxError('workspace.tags.genericFailureMessage'),
},
},
Expand All @@ -3460,7 +3461,7 @@ function createPolicyTag(policyID: string, tagName: string) {

const parameters = {
policyID,
tags: JSON.stringify([{name: tagName}]),
tags: JSON.stringify([{name: newTagName}]),
};

API.write(WRITE_COMMANDS.CREATE_POLICY_TAG, parameters, onyxData);
Expand Down Expand Up @@ -3649,7 +3650,9 @@ function clearPolicyTagErrors(policyID: string, tagName: string) {

function renamePolicyTag(policyID: string, policyTag: {oldName: string; newName: string}) {
const tagListName = Object.keys(allPolicyTags?.[`${ONYXKEYS.COLLECTION.POLICY_TAGS}${policyID}`] ?? {})[0];
const oldTag = allPolicyTags?.[`${ONYXKEYS.COLLECTION.POLICY_TAGS}${policyID}`]?.[tagListName]?.tags?.[policyTag.oldName] ?? {};
const oldTagName = PolicyUtils.escapeTagName(policyTag.oldName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bernhardoj Why do we need to escape the old tag?

Copy link
Contributor

@DylanDylann DylanDylann May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because we escaped the tag name when creating, It means that the old tag is escaped. Unless you want to cover tags created before this PR was merged, in this case, I think it isn't a good way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tag name that we pass to the old name is the "cleaned" tag name.

App/src/pages/workspace/tags/EditTagPage.tsx

Line 38 in e8ae3c5

const currentTagName = PolicyUtils.getCleanedTagName(route.params.tagName);

App/src/pages/workspace/tags/EditTagPage.tsx

Line 61 in e8ae3c5

Policy.renamePolicyTag(route.params.policyID, {oldName: currentTagName, newName: values.tagName.trim()});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bernhardoj Should we pass route.params.tagName instead of currentTagName

App/src/pages/workspace/tags/EditTagPage.tsx

Line 61 in e8ae3c5

Policy.renamePolicyTag(route.params.policyID, {oldName: currentTagName, newName: values.tagName.trim()});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to keep it as it is. Having 2 different sources of the tag name, route.params.tagName and currentTagName makes it more confusing, IMO.

Copy link
Contributor

@DylanDylann DylanDylann May 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we shouldn't make a redundant logic. Currently, we clean tag name and escape again

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, we only need to escape new tag

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I have updated it.

const newTagName = PolicyUtils.escapeTagName(policyTag.newName);
const oldTag = allPolicyTags?.[`${ONYXKEYS.COLLECTION.POLICY_TAGS}${policyID}`]?.[tagListName]?.tags?.[oldTagName] ?? {};
const onyxData: OnyxData = {
optimisticData: [
{
Expand All @@ -3658,15 +3661,15 @@ function renamePolicyTag(policyID: string, policyTag: {oldName: string; newName:
value: {
[tagListName]: {
tags: {
[policyTag.oldName]: null,
[policyTag.newName]: {
[oldTagName]: null,
[newTagName]: {
...oldTag,
name: policyTag.newName,
name: newTagName,
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
pendingFields: {
name: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
},
previousTagName: policyTag.oldName,
previousTagName: oldTagName,
},
},
},
Expand All @@ -3680,7 +3683,7 @@ function renamePolicyTag(policyID: string, policyTag: {oldName: string; newName:
value: {
[tagListName]: {
tags: {
[policyTag.newName]: {
[newTagName]: {
errors: null,
pendingAction: null,
pendingFields: {
Expand All @@ -3699,8 +3702,8 @@ function renamePolicyTag(policyID: string, policyTag: {oldName: string; newName:
value: {
[tagListName]: {
tags: {
[policyTag.newName]: null,
[policyTag.oldName]: {
[newTagName]: null,
[oldTagName]: {
...oldTag,
errors: ErrorUtils.getMicroSecondOnyxError('workspace.tags.genericFailureMessage'),
},
Expand All @@ -3713,8 +3716,8 @@ function renamePolicyTag(policyID: string, policyTag: {oldName: string; newName:

const parameters = {
policyID,
oldName: policyTag.oldName,
newName: policyTag.newName,
oldName: oldTagName,
newName: newTagName,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit hesitant when changing the param but everything seems to still work well

};

API.write(WRITE_COMMANDS.RENAME_POLICY_TAG, parameters, onyxData);
Expand Down
Loading