- Notifications
You must be signed in to change notification settings - Fork11
skill & skillsProvider removal#126
base:feature/removing_skill_model
Are you sure you want to change the base?
skill & skillsProvider removal#126
Uh oh!
There was an error while loading.Please reload this page.
Conversation
vikasrohit commentedOct 7, 2021
@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. |
vikasrohit commentedOct 7, 2021
@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 |
maxceem left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 runnpm 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 runnpm run migrations up on DEV / PROD it would adjust existent DB according to new changes while keeping the existent data.
vikasrohit commentedOct 7, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Sande3p commentedOct 8, 2021
Sande3p commentedOct 8, 2021
Yes, that's what I mentioned the challenge spec. |
Sande3p commentedOct 8, 2021
Two things are in progress
|
vikasrohit commentedOct 11, 2021
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@Sande3p I don't see any update in the code wheresetUserSearchClausesToEsQuery 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, I don't think we need to do anything insearchUsers, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What I meant issearchUsers 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed
| module.exports = { | ||
| up: async (query) => { | ||
| await query.removeColumn('UsersSkills', 'skillId') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@maxceem do you have idea how can handle this without loosing data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it will drop tableSkills andSkillsProviders.
If you mean preventing other table's data loss, then yes, i tried.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.