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

Make renaming of tags works #43005

Merged
merged 16 commits into from
Jun 7, 2024
Merged

Make renaming of tags works #43005

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
5112fcd
fix: add additional parameter
Jun 3, 2024
543998d
fix: pass tagListIndex
Jun 3, 2024
979bfc3
modify renamePolicyTag
Jun 3, 2024
a4204fb
fix: pass orderWeight
Jun 3, 2024
1284fac
fix: get individual tag
Jun 3, 2024
462253b
chore: clear fields & move errors clearing location
Jun 3, 2024
eacbbb1
clear specific pending field
Jun 5, 2024
e59c007
pass tag list index
Jun 5, 2024
11469a3
fix: make api command parameter required
Jun 5, 2024
4667251
fix: make function parameter required
Jun 5, 2024
12a234c
fix: pass orderWeight (index) to the function
Jun 5, 2024
44334ff
chore: pass index in the tests
Jun 5, 2024
9f71d25
chore: add a missing dependency
Jun 5, 2024
08f85ef
chore: add param type when specifying the parameter
Jun 6, 2024
c7f6093
refactor: rename file and type name
Jun 6, 2024
5d728f3
refactor: update the existing code to use the new file and type names
Jun 6, 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
1 change: 1 addition & 0 deletions src/libs/API/parameters/RenamePolicyTaglist.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ type RenamePolicyTaglist = {
policyID: string;
oldName: string;
newName: string;
tagListIndex: number;
};

export default RenamePolicyTaglist;
1 change: 1 addition & 0 deletions src/libs/API/parameters/RenamePolicyTagsParams.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ type RenamePolicyTagsParams = {
policyID: string;
oldName: string;
newName: string;
tagListIndex: number;
};

export default RenamePolicyTagsParams;
27 changes: 17 additions & 10 deletions src/libs/actions/Policy/Tag.ts
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

We have problems in error handling

  • Tags

    • Enabled: This is missing error handling
    • Name: This is writing to errors but we should write to errorFields

  • TagLists:

    • Required: Being handled in the other PR
    • Name: This is writing to errors but we should write to errorFields

Do you think this should be handled separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible that you can open up a follow up PR to fix these issues? I think you have much better grasp on how we handle errors in the context of Onyx and optimistic data stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah sure, can you please create an issue and assign me

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @hayata-suenaga did you create an issue for this?

Original file line number Diff line number Diff line change
Expand Up @@ -359,28 +359,30 @@ 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];
function renamePolicyTag(policyID: string, policyTag: {oldName: string; newName: string}, tagListIndex: number) {
hayata-suenaga marked this conversation as resolved.
Show resolved Hide resolved
hayata-suenaga marked this conversation as resolved.
Show resolved Hide resolved
const tagList = PolicyUtils.getTagLists(allPolicyTags?.[`${ONYXKEYS.COLLECTION.POLICY_TAGS}${policyID}`] ?? {})?.[tagListIndex] ?? {};
const tag = tagList.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: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY_TAGS}${policyID}`,
value: {
[tagListName]: {
[tagList.name]: {
tags: {
[oldTagName]: null,
[newTagName]: {
...oldTag,
...tag,
name: newTagName,
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
pendingFields: {
name: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
},
previousTagName: oldTagName,
errors: null,
},
},
},
Expand All @@ -392,10 +394,9 @@ function renamePolicyTag(policyID: string, policyTag: {oldName: string; newName:
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY_TAGS}${policyID}`,
value: {
[tagListName]: {
[tagList.name]: {
tags: {
[newTagName]: {
errors: null,
pendingAction: null,
pendingFields: {
name: null,
Expand All @@ -411,11 +412,15 @@ function renamePolicyTag(policyID: string, policyTag: {oldName: string; newName:
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY_TAGS}${policyID}`,
value: {
[tagListName]: {
[tagList.name]: {
tags: {
[newTagName]: null,
[oldTagName]: {
...oldTag,
...tag,
pendingAction: null,
pendingFields: {
name: null,
},
errors: ErrorUtils.getMicroSecondOnyxError('workspace.tags.genericFailureMessage'),
},
},
Expand All @@ -429,6 +434,7 @@ function renamePolicyTag(policyID: string, policyTag: {oldName: string; newName:
policyID,
oldName: oldTagName,
newName: newTagName,
tagListIndex,
};

API.write(WRITE_COMMANDS.RENAME_POLICY_TAG, parameters, onyxData);
Expand Down Expand Up @@ -482,7 +488,7 @@ function enablePolicyTags(policyID: string, enabled: boolean) {
}
}

function renamePolicyTaglist(policyID: string, policyTagListName: {oldName: string; newName: string}, policyTags: OnyxEntry<PolicyTagList>) {
function renamePolicyTaglist(policyID: string, policyTagListName: {oldName: string; newName: string}, policyTags: OnyxEntry<PolicyTagList>, tagListIndex: number) {
hayata-suenaga marked this conversation as resolved.
Show resolved Hide resolved
const newName = policyTagListName.newName;
const oldName = policyTagListName.oldName;
const oldPolicyTags = policyTags?.[oldName] ?? {};
Expand Down Expand Up @@ -526,6 +532,7 @@ function renamePolicyTaglist(policyID: string, policyTagListName: {oldName: stri
policyID,
oldName,
newName,
tagListIndex,
};

API.write(WRITE_COMMANDS.RENAME_POLICY_TAG_LIST, 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) {
Tag.renamePolicyTag(route.params.policyID, {oldName: route.params.tagName, newName: values.tagName.trim()});
Tag.renamePolicyTag(route.params.policyID, {oldName: route.params.tagName, newName: values.tagName.trim()}, route.params.orderWeight);
}
Keyboard.dismiss();
Navigation.goBack();
},
[route.params.policyID, route.params.tagName, currentTagName],
[currentTagName, route.params.policyID, route.params.tagName, route.params.orderWeight],
);

return (
Expand Down
4 changes: 2 additions & 2 deletions src/pages/workspace/tags/WorkspaceEditTagsPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,11 @@ function WorkspaceEditTagsPage({route, policyTags}: WorkspaceEditTagsPageProps)
const updateTaglistName = useCallback(
(values: FormOnyxValues<typeof ONYXKEYS.FORMS.POLICY_TAG_NAME_FORM>) => {
if (values[INPUT_IDS.POLICY_TAGS_NAME] !== taglistName) {
Tag.renamePolicyTaglist(route.params.policyID, {oldName: taglistName, newName: values[INPUT_IDS.POLICY_TAGS_NAME]}, policyTags);
Tag.renamePolicyTaglist(route.params.policyID, {oldName: taglistName, newName: values[INPUT_IDS.POLICY_TAGS_NAME]}, policyTags, route.params.orderWeight);
}
Navigation.goBack();
},
[policyTags, route.params.policyID, taglistName],
[policyTags, route.params.orderWeight, route.params.policyID, taglistName],
);

return (
Expand Down
26 changes: 18 additions & 8 deletions tests/actions/PolicyTagTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ describe('actions/Policy', () => {
newName: newTagListName,
},
fakePolicyTags,
Object.values(fakePolicyTags)[0].orderWeight,
);
return waitForBatchedUpdates();
})
Expand Down Expand Up @@ -244,6 +245,7 @@ describe('actions/Policy', () => {
newName: newTagListName,
},
fakePolicyTags,
Object.values(fakePolicyTags)[0].orderWeight,
);
return waitForBatchedUpdates();
})
Expand Down Expand Up @@ -514,10 +516,14 @@ describe('actions/Policy', () => {
Onyx.set(`${ONYXKEYS.COLLECTION.POLICY_TAGS}${fakePolicy.id}`, fakePolicyTags);
})
.then(() => {
Tag.renamePolicyTag(fakePolicy.id, {
oldName: oldTagName,
newName: newTagName,
});
Tag.renamePolicyTag(
fakePolicy.id,
{
oldName: oldTagName,
newName: newTagName,
},
0,
);
return waitForBatchedUpdates();
})
.then(
Expand Down Expand Up @@ -580,10 +586,14 @@ describe('actions/Policy', () => {
.then(() => {
mockFetch?.fail?.();

Tag.renamePolicyTag(fakePolicy.id, {
oldName: oldTagName,
newName: newTagName,
});
Tag.renamePolicyTag(
fakePolicy.id,
{
oldName: oldTagName,
newName: newTagName,
},
0,
);
return waitForBatchedUpdates();
})
.then(mockFetch?.resume)
Expand Down
Loading