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

Fix bug on adding skills to new tasks

Đang mở
#1,161 4 bình luận 0 reaction 0 người được giao Xem trên GitHub
bug Difficulty: Medium needs clarification needs estimate
Ngôn ngữ chính
JavaScript
Star
120
Fork
75
Chỉ số merge pull request
Không có pull request nào được merge trong 30 ngày

Mô tả

# 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.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.