[FIX] Importers failing when usernames exists but cases don't match and improve the importer framework's performance #8966
Conversation
…the structure of the importers imports
| export class CsvImporter extends Base { | ||
| constructor(info) { | ||
| super(info); | ||
| this.logger.debug(`Constructed a new ${ info.name } Importer.`); |
There was a problem hiding this comment.
Since using template string might as well move this up to Base?
| @@ -282,7 +287,7 @@ Importer.HipChatEnterprise = class ImporterHipChatEnterprise extends Importer.Ba | |||
| this.collection.update({ _id: this.users._id }, { $set: { 'users': this.users.users }}); | |||
|
|
|||
| //Import the channels | |||
| super.updateProgress(Importer.ProgressStep.IMPORTING_CHANNELS); | |||
| super.updateProgress(ProgressStep.IMPORTING_CHANNELS); | |||
There was a problem hiding this comment.
Curious why super.updateProgress? Taking a look at the slack one its using just this.updateProgress
There was a problem hiding this comment.
Great question, although that's been like that since when it was developed...I didn't change it in this pull request.
There was a problem hiding this comment.
😁 I mean its valid. But its inconsistent. In the cases where the name is the same as the class its extending.. makes complete sense because it can't call this.methodName.
|
|
||
| // Clean up all the raw import data, since you can't restart an import at the moment | ||
| return Importer.Imports.find({ valid: { $ne: true }}).forEach(item => Importer.RawImports.remove({ 'import': item._id, 'importer': item.type })); | ||
| try { | ||
| RawImports.model.rawCollection().drop(); |
There was a problem hiding this comment.
Much, much faster then RawImports.remove 😍
| Meteor.startup(function() { | ||
| // Make sure all imports are marked as invalid, data clean up since you can't | ||
| // restart an import at the moment. | ||
| Importer.Imports.update({ valid: { $ne: false } }, { $set: { valid: false } }, { multi: true }); | ||
| Imports.update({ valid: { $ne: false } }, { $set: { valid: false } }, { multi: true }); |
There was a problem hiding this comment.
Why update if going to just drop?
There was a problem hiding this comment.
Imports is the information regarding the progress of the import which we need to have around still so we can see where and why an import failed. RawImports is what is being dropped, as it contains all the "raw" data an import has in it such as every single little message.
| case 'Bot': | ||
| isBot = true; | ||
| break; | ||
| case 'Deactivated': |
There was a problem hiding this comment.
Any chance it'll ever be more than one of these?
There was a problem hiding this comment.
No clue, these are the only ones that I have seen in the export that contained 5000+ users..
There was a problem hiding this comment.
What I mean by this is can it ever be bot and deactivated? or admin and deactivated? or admin, bot and deactivated?
There was a problem hiding this comment.
Ah okay. No, it is a single value field.
graywolf336
changed the title
…Rocket.Chat into slack-importer-related-fixes
Big change, but only major thing that changes is the framework and now using es6 style imports instead of global variables. Also, the import progress which is relayed to the client is no longer a pulling using the same method but instead the server is sending the progress out to the client. This should improve the performance on the server and client side, as pulling is expensive.