code-corps / code-corps/code-corps-ember
Fix bug on adding skills to new tasks
- Dominant language
- JavaScript
- Stars
- 120
- Forks
- 75
- PR merge metrics
- No merged PRs in 30d
Description
# Problem
From @begedin:
If I add a skill using the project skill list, then try to search for the same skill in the dropdown, it will appear "uncrossed", as if not yet added, in the results:

Even though it looks incorrect, clicking it will still remove it, not add a duplicate, so at least the background logic works.
Basically,
- `skill-typeahead-result` usually uses `service:x-skills.includes(skill)` to determine if skill has already been added. this method is NOT easily rewritten into something else
- In the case of `task.new`, the `skill-typeahead-result` instead uses just an `array.includes(skill)`, but this no longer works when we add project skills into the mix, because those are `DS.PromiseObject` records, not `DS.Model` records. This happened to work because the method was named the same, but the proper method to use here is `array.isAny('id', skillId)` so it works with both `DS.Model` and `DS.PromiseObject` records
- monkeypatch the `includes` method in this specific case - quick and dirty, definitely against it
- wrap the unsaved skills collection into another service, which makes it work - clean, but makes thing worse in the long run, I think. Another complex service, difficult to follow.
- rethink the `projectSkillsList` component added here. make it an extremely simple `skills-item-list` component which just receives and renders a list of skills. let the controller handle the exclusion of already added skills, etc. - clean and doesn't make things worse; a good compromise.
- rewrite the services, rethink them, really. now that I got back into them, I found them extremely difficult to follow and, especially with "sometimes" accessing the `content` property of proxy objects, ex:
```Javascript
return records.any((found) => {
let targetId = get(target, 'id');
let targetModelName =
get(target, 'constructor.modelName') || get(target, 'content.constructor.modelName');
let foundId = found.belongsTo(targetModelName).id();
return (foundId === targetId);
});
```
I think, with proper controller usage, the whole `x-SkillsList` collection of services could be almost if not completely eliminated.
Contributor guide
Assessment
This issue has not been assessed yet.