code-corps / code-corps/code-corps-ember

Fix bug on adding skills to new tasks

Open
#1,161 4 comments 0 reactions 0 assignees View on GitHub
bug Difficulty: Medium needs clarification needs estimate
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:

![image](https://cloud.githubusercontent.com/assets/1925540/23950429/3f66538e-098b-11e7-8579-a090203bd3a5.png)

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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.