[TTAHUB-3541] Add code coverage for src/services/recipient.js #2591
Conversation
kryswisnaskas
changed the title
kryswisnaskas
requested review from
thewatermethod,
nvms,
AdamAdHocTeam,
GarrettEHill and
hardwarehuman
src/services/recipient.js
Outdated
| export function ensureArray(input) { | ||
| return Array.isArray(input) ? input : []; | ||
| } |
There was a problem hiding this comment.
just a thought, but maybe we could move this function to constants or somewhere else
src/services/recipient.js
Outdated
| const newTopics = uniq((topics || null) | ||
| .filter((topic) => topic !== null && topic !== undefined).map((topic) => { | ||
| if (typeof topic === 'string') { | ||
| return topic; | ||
| } | ||
|
|
||
| if (topic.name) { | ||
| return topic.name; | ||
| } | ||
| if (typeof topic === 'object' && topic.name) { | ||
| return topic.name.trim(); | ||
| } | ||
|
|
||
| return topic; | ||
| })); | ||
| return topic; | ||
| })); | ||
|
|
||
| newTopics.sort(); | ||
| newTopics.sort((a, b) => { | ||
| if (typeof a === 'string' && typeof b === 'string') { | ||
| return a.localeCompare(b); | ||
| } | ||
| if (typeof a === 'string') return -1; | ||
| if (typeof b === 'string') return 1; | ||
| return 0; | ||
| }); |
There was a problem hiding this comment.
This wont reduce it as expected, the unique or similar functionality needs to be last or twice. topcis = ['apples', { name: "apples"}] => ["apples", "apples"]
There was a problem hiding this comment.
We are good here as is for the case topics = ['apples', { name: "apples"}] => ["apples", "apples"]. The second 'apples' topic is already handled by this logic:
if (typeof topic === 'object' && topic.name) {
return topic.name.trim();
}
and deduplicated via uniq
Additionally, the existing test case on line 1024, it('should handle mixed strings and objects with "name"', () => { already tests and confirms it.
There was a problem hiding this comment.
Its interesting that you are using multiple methods to unique the values, lodash.unique and [...new Set()].
I don't see how the code would do that?
const newTopics = uniq((topics || null) /// the only unique is on the initial input, which will not handle the string /object case.
.filter((topic) => topic !== null && topic !== undefined) // this filters out the empty cases, but not the empty string case.
.map((topic) => {
if (typeof topic === 'string') {
return topic; // return if its a string, even empty or space padded.
}
if (typeof topic === 'object' && topic.name) {
return topic.name.trim(); // return the trimed value only when it was an object
}
return topic; // return what ever, random objects
}));
the result in newTopics can have duplicates, and space padded values if they came in a strings.
There was a problem hiding this comment.
I see the issue with how I was reading it now. the map is within the uniq.
There was a problem hiding this comment.
The values from the string path do still need to be trimmed
There was a problem hiding this comment.
Its interesting that you are using multiple methods to unique the values, lodash.unique and [...new Set()].
Good catch! I prefer using native JavaScript functions like Set to avoid relying on external libraries, but since the code works fine as-is, I don’t see a big reason to refactor it right now. If we end up revisiting this file later, I’d be up for cleaning it up to use native methods.
There was a problem hiding this comment.
The values from the string path do still need to be trimmed
trim() was added.
GarrettEHill
dismissed
their stale review
After further reading the main issue was a mistake in how I was reading the code, and not the code it self
Description of change
Adds test coverage for src/services/recipient.js
How to test
Verify the test coverage for src/services/recipient.js is above 90%
Issue(s)
Checklists
Every PR
Before merge to main
Production Deploy
After merge/deploy