Regression: Various search provider fixes

[tkurz]

This PR provides smaller fixes for the new search provider concept + chatpal (its reference implementation), which are:

• suggestions are now closed when a search is already issues. This was a bug.
• add a link from the chatpal admin page (./admin/chatpal) to the chatpal website. This gives the admins more information about the types of backends, etc.
• fixes sizing issue for the chatpal search icon, this was an issue in firefox, the image was to large (Kudos to @wernerharing) + an unused icon in lightblue is removed

Would be great if you can approve the PR and merge it, @sampaiodiego @rodrigok @graywolf336

[graywolf336]

@tkurz any idea about this exception? We keep seeing it on our Open Community server (open.rocket.chat):

TypeError: Cannot read property '_id' of undefined
    at /app/bundle/programs/server/packages/rocketchat_search.js:102:49
    at /app/bundle/programs/server/packages/rocketchat_lib.js:1188:30
    at Array.reduce (<anonymous>:null:null)
    at Object.RocketChat.callbacks.run (/app/bundle/programs/server/packages/rocketchat_lib.js:1181:8)
    at /app/bundle/programs/server/packages/rocketchat_lib.js:4509:28
    at Meteor.EnvironmentVariable.EVp.withValue (packages/meteor.js:1186:12)
    at packages/meteor.js:502:25
    at runWithEnvironment (packages/meteor.js:1238:24)

[graywolf336]

I've also updated the name since the feature was added in v0.64, so any additional fixes are just regressions.

[tkurz]

Hi @graywolf336, regarding the error. It happens when deleting a message, in this function:

RocketChat.callbacks.add('afterDeleteMessage', function(m) {
	eventService.promoteEvent('message.delete', m._id);
});

This can only happen, if m is set (and a message), which I took as given. If this is not the case, we have a problem here (as the search index does not know which message is deleted. If this can happen (and a quick lookup of the /packages/rocketchat-lib/server/functions/deleteMessage.js function makes me feel that it can, we have to trigger it in another way. As Message class is a subclass of RocketChat.models._Base we could use Emitters (like we do it for Rooms and Users). What do you think?

The code will then look like this:

RocketChat.models.Messages.on('changed', (type, message)=>{
	if (type === 'inserted' || type === 'updated') {
		eventService.promoteEvent('message.save', message._id, message);
	}
	if (type === 'removed') {
		eventService.promoteEvent('message.delete', message._id);
	}
});

[tkurz]

@graywolf336 No, Message class doesn not emit event (I think because cache is not true).. Any other ideas?

[sampaiodiego]

I guess we should fix it by calling the callback this way

RocketChat.callbacks.run('afterDeleteMessage', deletedMsg || { _id: message.id });

I mean, it doesn't make any sense calling afterDeleteMessage with an empty variable just because Message_KeepHistory is set to false and Message_ShowDeletedStatus is set to true

[graywolf336]

@sampaiodiego sounds like a solid plan to me. Should we go ahead and make this commit to his branch?

[sampaiodiego]

@graywolf336 I think so.. It can part of this pull request

[sampaiodiego]

just so nobody merges this accidentally:

please add the fix to the afterDeleteMessage callback

[tkurz]

Just to get it right, you do the change right?

[graywolf336]

If you have time, feel free to

[tkurz]

Hi @graywolf336
I did the fix that was recommended by @sampaiodiego. The error should not happen anymore.

[graywolf336]

@sampaiodiego should this call to the callbacks be ran outside the if (keepHistory)? Because if that setting is true, the message is being deleted to the user's perspective even though we aren't technically deleting it.

[sampaiodiego]

right.. since the message will be suppressed from any endpoint, we should call the callback. I'll change it to reflect that.