Fix bug on adding skills to new tasks

Open
#1,161 4 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
3/5
Estimated time
1-2 days
Newbie friendliness
35/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Stale
Tech stack
javascript
Domain
frontend

Research direction

Start by locating the task.new skill-typeahead-result and the projectSkillsList component, then trace how the selected skills collection is checked. Verify the existing project skill is shown as selected in search results and that clicking it removes it without creating a duplicate; review the related x-SkillsList services if the component flow requires it.

Written by the indexing model from the issue text.

Description

bug Difficulty: Medium needs clarification needs estimate

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

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:

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.

Dominant language
JavaScript
Stars
120
Forks
75
PR merge metrics
No merged PRs in 30d

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from code-corps/code-corps-ember

All issues in code-corps/code-corps-ember

Similar issues

More JavaScript issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.