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: unexpected behavior of tag selection #47687

Merged
merged 2 commits into from
Aug 22, 2024
Merged

fix: unexpected behavior of tag selection #47687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
086de7d
fix: unexpected behavior of tag selection
daledah Aug 20, 2024
5f7cb17
fix: add unit test for insertTagIntoTransactionTagsString
daledah Aug 22, 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
9 changes: 5 additions & 4 deletions src/libs/IOUUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,11 @@ function insertTagIntoTransactionTagsString(transactionTags: string, tag: string
const tagArray = TransactionUtils.getTagArrayFromName(transactionTags);
tagArray[tagIndex] = tag;

return tagArray
.map((tagItem) => tagItem.trim())
.filter((tagItem) => !!tagItem)
.join(CONST.COLON);
while (tagArray.length > 0 && !tagArray[tagArray.length - 1]) {
tagArray.pop();
}

return tagArray.map((tagItem) => tagItem.trim()).join(CONST.COLON);
Comment on lines +144 to +148
Copy link
Contributor

Choose a reason for hiding this comment

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

@daledah will only the end values be falsey? the original code removes falsy from anywhere in the array. Your tests don't cover a case with falsey in the middle. I don't have full context on tags cc @situchan

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, we cannot remove falsey values in the middle. It causes regression (array index mismatch).
This was original code (before regression PR) which trims only last space after ::
tagArray.join(CONST.COLON).replace(/:*$/, '')

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the context @situchan

}

function isMovingTransactionFromTrackExpense(action?: IOUAction) {
Expand Down
18 changes: 18 additions & 0 deletions tests/unit/IOUUtilsTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,24 @@ describe('IOUUtils', () => {
expect(IOUUtils.calculateAmount(participantsAccountIDs.length, 100, 'BHD')).toBe(33);
});
});

describe('insertTagIntoTransactionTagsString', () => {
test('Inserting a tag into tag string should update the tag', () => {
expect(IOUUtils.insertTagIntoTransactionTagsString(':NY:Texas', 'California', 2)).toBe(':NY:California');
});

test('Inserting a tag into an index with no tags should update the tag', () => {
expect(IOUUtils.insertTagIntoTransactionTagsString('::California', 'NY', 1)).toBe(':NY:California');
});

test('Inserting a tag with colon in name into tag string should keep the colon in tag', () => {
expect(IOUUtils.insertTagIntoTransactionTagsString('East:NY:California', 'City \\: \\:', 1)).toBe('East:City \\: \\::California');
});

test('Remove a tag from tagString', () => {
expect(IOUUtils.insertTagIntoTransactionTagsString('East:City \\: \\::California', '', 1)).toBe('East::California');
});
});
});

describe('isValidMoneyRequestType', () => {
Expand Down
Loading