skill & skillsProvider removal #126
Conversation
|
@Sande3p I don't see any unit test updated in change files in response to the code updated? Do the unit tests not exist for the various files we modified e.g. userSkillService etc. Also adding @maxceem and @nkumar-topcoder for more thorough reviews. |
|
@SathyaJayabal @drasticdpk @lakshmiathreya this PR might cause some disruption in taas application as taas-apis are using ubahn-apis. So, it would be great if we can identify the possible areas which could be impacted after this merge. My major expectation is the skill search where |
There was a problem hiding this comment.
It looks for me that the way migrations are changed would not work for DEV/PROD 
. When we run migrations during local setup, the DB is created from scratch and then migrations are applied to the empty DB.
But in real DEV/PRDO environment we already have DB with data and these migrations has been already applied. So when we would run npm run migrations up nothing would happen.
Instead of editing existent migration files, a new migration file suppose to be created and that migration file should make all the changes in the DB, so when we run npm run migrations up on DEV / PROD it would adjust existent DB according to new changes while keeping the existent data.
Yes, that's what I mentioned the challenge spec. |
|
Two things are in progress
|
|
@Sande3p are review changes implemented? |
|
|
||
| return results.hits.hits.map(hit => hit._source) | ||
| } | ||
|
|
||
| async function setUserSearchClausesToEsQuery (boolClause, keyword) { |
There was a problem hiding this comment.
@Sande3p I don't see any update in the code where setUserSearchClausesToEsQuery is used. I mean if we have removed searching skills in this method's implementation, we must be doing that where this function is being called, right? Am I missing something here?
There was a problem hiding this comment.
No, I don't think we need to do anything in searchUsers, the keyword be used to match user records. since we remove skill model, we just remove the matching skill name and reserve other matchings.
There was a problem hiding this comment.
What I meant is searchUsers method would not support searching by skills name now, if we don't add some other way to provide filtering for user based on skills names. I want to be sure that we are not removing any feature from the calling code. We need to remove skills model but we should not remove any feature from the calling code.
|
|
||
| module.exports = { | ||
| up: async (query) => { | ||
| await query.removeColumn('UsersSkills', 'skillId') |
There was a problem hiding this comment.
@Sande3p this would cause all the existing data to be lost, I guess. How can we handle that?
There was a problem hiding this comment.
@maxceem do you have idea how can handle this without loosing data?
There was a problem hiding this comment.
Good catch @vikasrohit. I don't think these columns should be removed as we this script adds them back after removing which looks like doesn't make sense. I guess these columns should be left as it is without any changes. The only thing which has to be changed - references from other tables to these columns have to be removed and that's it.
There was a problem hiding this comment.
it will drop table Skills and SkillsProviders.
If you mean preventing other table's data loss, then yes, i tried.
There was a problem hiding this comment.
If you mean preventing other table's data loss, then yes, i tried.
Yes, that is what I was worried about. If the data is not lost in any tables excepts the two we want to get rid of.
No description provided.