Add user skills #163
Add user skills #163
Conversation
|
@begedin could use your help on this. |
|
@joshsmith Looks very clean to me. I got no remarks, component or any other part of the code. |
joshsmith
force-pushed
the
add-user-skills
branch
from
c86da21
to
fc76f3d
Compare
|
@begedin this is mostly done, it just needs a review and also needs some love on these tests. I can't figure out why they're passing on their own but failing in other instances. Can you take a look? |
|
Weird. The tests pass here but not in my browser when run all at once. |
|
@joshsmith Tests run fine on my machine. Could be it's the same issue I had in #120. I Updated the dependencies, so try pulling the latest, running bower up and testing again. |
| this.get('/skills'); | ||
| this.get('/user_skills'); | ||
|
|
||
| // TODO: Make this work when relationships work |
There was a problem hiding this comment.
Which part of this doesn't work? A join model should be fine, since, unlike projects, there isn't a user -> skill many-to-many relationship here, just a user > userSkills, and skill > userSkills.
There was a problem hiding this comment.
Maybe I'm misunderstanding then. I can remove this TODO in that case?
There was a problem hiding this comment.
You might even be able to outright remove the custom function and just have this.post('/user_skills')
There was a problem hiding this comment.
This is the case for user-skill, user-role, and user-category then, right?
There was a problem hiding this comment.
@begedin I don't think I can. I think Sam mentioned this was still many-to-many so it wouldn't work because Mirage doesn't support it out of the box right now.
There was a problem hiding this comment.
It's not a many to many, though
It's a one-to-many user -> userSkill and a one-to-many skill -> userSkill. user -> skills would be a many to many relationship, but we don't have that on our model here. What we have is what mirage should already be supporting.
There was a problem hiding this comment.
In any case, I would try and remove this custom method and see if the tests run without it. I believe they should.
There was a problem hiding this comment.
Yeah, let me verify.
|
looks good. there's that one question, because I'm not understanding why it doesn't work, but other than that, seems fine. |
Update config to be alpha ordered
joshsmith
force-pushed
the
add-user-skills
branch
from
0ef8285
to
817dfed
Compare
This has no tests and the input's not there yet. Lots of work left to do.
Big thing I can use help on at this point is the
highlight-substringshelper. I'm down a rabbit hole and can't see my way out with that one.