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 4 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 = policyTag.oldName;
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
4 changes: 2 additions & 2 deletions src/pages/workspace/tags/EditTagPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,12 @@ function EditTagPage({route, policyTags}: EditTagPageProps) {
const tagName = values.tagName.trim();
// Do not call the API if the edited tag name is the same as the current tag name
if (currentTagName !== tagName) {
Policy.renamePolicyTag(route.params.policyID, {oldName: currentTagName, newName: values.tagName.trim()});
Policy.renamePolicyTag(route.params.policyID, {oldName: route.params.tagName, newName: values.tagName.trim()});
}
Keyboard.dismiss();
Navigation.goBack();
},
[route.params.policyID, currentTagName],
[route.params.policyID, route.params.tagName, currentTagName],
);

return (
Expand Down
Loading