[FIX] #345 - where smarti conversation cache got out of synch #346

ruKurz · 2018-06-05T22:40:50Z

Closes #345

This PR includes:

force re-synch all rooms and messages with smarti when re-synch is triggered via admin UI
updating the livechat-external-message collection when replacing/creating new convIds in smarti
reducing redundant code for getting conversations

…arti

mrsimpson

The change is comparatively huge - for actually two places in which functionality has changed. At least this is how far I understood it. This raised a lot of additional questions which I would like to get clarified prior to merging this. Anyway: Thanks for analyzing it 👍

mrsimpson · 2018-06-06T06:27:03Z

packages/assistify-ai/server/lib/SmartiAdapter.js

-	 * @param smartiConversationId
-	 * @param token
-	 */
-	static analysisCompleted(roomId, smartiConversationId, token) {


Why do you think there should be no interface method anymore for Smarti to inform about a completed analysis?
As written above, I'd favor to keep the adapter as rich as possible. However you are right: The code for updating the LivechatExternalMessage was redundant: _updateMapping should be called

mrsimpson · 2018-06-06T06:29:44Z

packages/assistify-ai/server/SmartiRouter.js

@@ -27,7 +27,8 @@ RocketChat.API.v1.addRoute('smarti.result/:_token', {authRequired: false}, {
 		if (this.urlParams._token && this.urlParams._token === rcWebhookToken) {

 			SystemLogger.debug('Smarti - got conversation result:', JSON.stringify(this.bodyParams, null, 2));
-			SmartiAdapter.analysisCompleted(this.bodyParams.channelId, this.bodyParams.conversationId, this.bodyParams.token);
+			SmartiAdapter.updateMapping(this.bodyParams.channelId, this.bodyParams.conversationId, new Date(), this.bodyParams.token);


This updating information retrieved from Smarti and notifying about it was intended to be handled in the semantically richer analysisCompleted Method. I tried to have no application code in the routes but instead have the routes only handle the tech and then pass on the relevant data to the adapter layer. Reverting from this should be well justified.

mrsimpson · 2018-06-06T06:36:15Z

packages/assistify-ai/server/lib/SmartiAdapter.js

@@ -35,34 +36,47 @@ export class SmartiAdapter {
 	}

 	/**
+	 * Returns a Smarti conversation Id for the given roomId.


This is pretty much what the signature is saying, isn't it? I intentionally avoided redundant information in comments and obviously had nothing more to say about it ;)

mrsimpson · 2018-06-06T06:36:21Z

packages/assistify-ai/server/lib/SmartiAdapter.js

@@ -18,7 +19,7 @@ export class SmartiAdapter {
 		return RocketChat.settings.get('Assistify_AI_Smarti_Domain');
 	}

-	static _updateMapping(roomId, conversationId, timestamp) {
+	static updateMapping(roomId, conversationId, timestamp) {


having the private methods prefixed _ was intentional. Changing the scope of a method is kind-of a huge change which shouldn't be done imho, since this is only a helper method which is not subject to anyone outside the adapter.

mrsimpson · 2018-06-06T06:40:01Z

packages/assistify-ai/server/lib/SmartiAdapter.js

@@ -99,7 +113,7 @@ export class SmartiAdapter {
 		SystemLogger.debug('RocketChatMessage:', message);
 		SystemLogger.debug('Message:', requestBodyMessage);

-		let conversationId = SmartiAdapter._getConversationId(message.rid, message);
+		let conversationId = SmartiAdapter.getConversationId(message.rid);


It seems as if there has been a consumer who was passing on the message. Thus, again: Why remove this from the mapping? Providing the message allos a more precise timestamping.
And the semantic is well included in the signature:
"get the current view Smarti has about a conversation for a given message which has occurred". Without the message, to me it looks like we could also get an outdated state.

mrsimpson · 2018-06-06T06:57:35Z

packages/assistify-ai/server/lib/SmartiAdapter.js

-		if (smartiResponse && smartiResponse.conversationId) {
-			conversationId = smartiResponse.conversationId;
+		SystemLogger.debug(`Retrieving conversation ID for channel: ${ roomId }`);
+		const m = RocketChat.models.LivechatExternalMessage.findOneById(roomId);


Any reason you renamed smartiResponse to m? Do you think it was not explanatory earlier on? The collection holds the Smarti-Analysis and should better be named like that, but I was reluctant to perform this change yet.

mrsimpson · 2018-06-06T07:00:23Z

packages/assistify-ai/server/lib/SmartiAdapter.js

-					// 404 is expected if no mapping exists
-					if (error.response.statusCode === 404) {
-						return null;
+			const conversation = RocketChat.RateLimiter.limitFunction(


Any reason you limited this rate?

mrsimpson · 2018-06-06T07:03:05Z

packages/assistify-ai/server/lib/SmartiAdapter.js

-				conversationId = conversation.id;
-				const timestamp = message ? message.ts : Date.now();
-				SmartiAdapter._updateMapping(roomId, conversationId, timestamp);
+				let timestamp = conversation.messages &&


Essentially, this is the actual change, right? Instead of utilizing the timestamp from the message, you take the timestamp from the conversation stored in Smarti. How can they differ? I'd love to read some comment about the "why we do it like that" in the code

mrsimpson · 2018-06-06T07:06:00Z

packages/assistify-ai/server/methods/SmartiReload.js


 Meteor.methods({
 	triggerFullResync() {
 		SystemLogger.info('Full Smarti resync triggered');

-		const query = {$or: [{outOfSync: true}, {outOfSync: {$exists: false}}]};
+		const query = {$or: [{outOfSync: true}, {outOfSync: false}, {outOfSync: {$exists: false}}]};


now this includes pretty much anything, doesn't it? a boolean value can be absent or have the values true or false. Why did you introduce this? Should we not - given the method name - just remove the selection criterion?

mrsimpson · 2018-06-06T07:10:35Z

packages/assistify-ai/server/methods/SmartiReload.js

-		SmartiProxy.propagateToSmarti(verbs.post, 'conversation', conversation);
+		const response = SmartiProxy.propagateToSmarti(verbs.post, 'conversation', conversation);
+		SystemLogger.debug('Update conversation with id: ', response.id);
+		SmartiAdapter.updateMapping(rid, response.id);


I believe this should be done implicitly inside the Smarti adapter. Actually, I would expect we don't use the SmartiProxy outside the Smarti Adapter. I obviously oversaw this earlier on.

Added an admin option to re-synch completely.

Ordered private dunctions and added some comments.

ruKurz · 2018-06-07T21:07:31Z

I've now finished my work so far.

The current status should ensure that the cache for the Smarti analysis results is updated correctly. The most important concepts (visibility of methods and the distribution of responsibilities) were taken into account. In the admin backend there is now an additional option to perform a complete synchronization with Smarti.

ruKurz · 2018-06-07T21:11:56Z

@mrsimpson Thank you for the valuable comments from the review. It would be great if you could take a look at the current state. The most important changes I've made you'll find within the SmartiAdapter.

mrsimpson

Looks way better now 👍 . The new determination of the sync-timestamp raises some questions as well as the limiting.

mrsimpson · 2018-06-08T03:23:30Z

packages/assistify-ai/server/SmartiRouter.js

@@ -27,7 +27,8 @@ RocketChat.API.v1.addRoute('smarti.result/:_token', {authRequired: false}, {
 		if (this.urlParams._token && this.urlParams._token === rcWebhookToken) {

 			SystemLogger.debug('Smarti - got conversation result:', JSON.stringify(this.bodyParams, null, 2));
-			SmartiAdapter.analysisCompleted(this.bodyParams.channelId, this.bodyParams.conversationId, this.bodyParams.token);
+			SmartiAdapter.analysisCompleted(this.bodyParams.channelId, this.bodyParams.conversationId, Date.now(), this.bodyParams.token);


@ruKurz after y'day's decision to use the timestamp of the smarti conversation for the mapping information, why do we pass Date.now() into this method actually?

mrsimpson · 2018-06-08T03:29:55Z

packages/assistify-ai/server/lib/SmartiAdapter.js

 	 */
-	static analysisCompleted(roomId, smartiConversationId, token) {
+	static analysisCompleted(roomId, conversationId, timestamp, token) {


Is it not like that we've always got the conversation at hand once this method is called? Then, we could (and should) get the timestamp via _getLastConversationAnalyzedTime and remove the conversationId and timestamp from the signature.

mrsimpson · 2018-06-08T03:31:53Z

packages/assistify-ai/server/methods/SmartiReload.js

-					return false;
+
+		conversation = RocketChat.RateLimiter.limitFunction(
+			SmartiProxy.propagateToSmarti, 5, 1000, {


We should limit the rate of this method being called - but not the interactions inside this method with Smarti - it would slow-down the sync.

mrsimpson · 2018-06-08T03:32:08Z

packages/assistify-ai/server/methods/SmartiReload.js

-			SmartiProxy.propagateToSmarti(verbs.delete,
-				`conversation/${ conversation.id }`, null);
+			RocketChat.RateLimiter.limitFunction(
+				SmartiProxy.propagateToSmarti, 5, 1000, {


same here: no limiting of the propagation

mrsimpson · 2018-06-08T03:32:40Z

packages/assistify-ai/server/methods/SmartiReload.js

@@ -137,7 +154,16 @@ Meteor.methods({
 			conversation.messages.push(newMessage);
 		}

-		SmartiProxy.propagateToSmarti(verbs.post, 'conversation', conversation);
+		const analyzedConversation =  RocketChat.RateLimiter.limitFunction(
+			SmartiProxy.propagateToSmarti, 5, 1000, {


and again: limiting

mrsimpson · 2018-06-08T03:36:04Z

P.s.: If you merge develop into this PR now, a docker image of the PR will be built automatically - at least once you fixed the linting ;)

ruKurz · 2018-06-08T13:16:59Z

Hi @mrsimpson, I had to notice that the asynchronous callbacks have been replaced by using the synchronous analysis when updating to Smarti 0.7.0. Thought I'd just put it back in. I had to realize that the response body from Smarti, which is sent to the RC Webhook, only contains the analysis results but not the roomId, so it is not possible to notify the room at all.

If we want to get through the current changes, we have to do the analysis synchronously or we can use the conversationId cache (livechat-external-messages) in order to get the roomId by conversationId on the Assistify.Chat side.

For now I've implemented to get the roomId by the livechat-external-messages when the asynchronous callback gets back to Assistify.Chat. This resolves the dependencies to Smarti for now and we can make use of asynchronous communication between Smarti an Assistify.Chat.

Created an Issue for Smarti: redlink-gmbh/smarti#251

…-not-loaded

…essage cache.

Restricted resynch access for admins Extracted synch to Smartiadapter Using getProperties instead legacy impl when sync

mrsimpson

some intermediate comments

mrsimpson · 2018-06-12T07:04:26Z

packages/assistify-ai/server/lib/SmartiAdapter.js

 			SmartiProxy.propagateToSmarti(verbs.delete, `conversation/${ conversationId }/message/${ message._id }`);
+			SmartiAdapter._getAnalysisResult(message.rid, conversationId);


why's an analysis result retrieved after delete?

mrsimpson · 2018-06-12T07:43:38Z

packages/assistify-ai/server/methods/SmartiReload.js

+		// post the new conversation to Smarti
+		const newSmartiConversation = SmartiProxy.propagateToSmarti(verbs.post, 'conversation', conversation);
+		// get the analysis result
+		const analysisResult = SmartiProxy.propagateToSmarti(verbs.get, `conversation/${ newSmartiConversation.id }/analysis`);


When submitting analysis: true in the body of the post, the analysis is synchronously returned. Thus, the get can be omitted.

Simplified clear method.

…read is created

mrsimpson

looks reasonable to me ;)

mrsimpson · 2018-06-12T11:23:10Z

packages/assistify-help-request/server/methods/createRequestFromExpertise.js

@@ -29,6 +31,9 @@ export class CreateRequestFromExpertise extends CreateRequestBase {
 			this._members = CreateRequestFromExpertise.getExperts(this._expertise);
 		}
 		const roomCreateResult = RocketChat.createRoom('r', this.name, Meteor.user() && Meteor.user().username, this._members, false, {expertise: this._expertise});
+		const knowledgeAdapter = getKnowledgeAdapter();
+		knowledgeAdapter.afterCreateChannel(roomCreateResult.rid);


I asked myself "Why not have this registered as a hook?". Then I found the answer: there's no good implementation of this in the core: There's only

if (type === 'c') { Meteor.defer(() => { RocketChat.callbacks.run('afterCreateChannel', owner, room); }); } else if (type === 'p') { Meteor.defer(() => { RocketChat.callbacks.run('afterCreatePrivateGroup', owner, room); }); }

in createRoom.js

ruKurz · 2018-06-13T06:36:39Z

From my side this PR has an nearly good state.

Include

SmartiAdapter.onMessage should trigger a resync to heal outdated conversations, when at least one message is out of sync (include)
SmartiAdapter.resync should handle livechat rooms as well (include)
previously implemented end-to-end tests do not work because of Rocket.Chat core changes, make them run again (include)

create a separate Issue

Changing the Smarti client configuration (and switching back) should be supported by the resynchronization (not tested yet)
SmartiAdapter.getConversationId should use the legacy endpoint as fallback (depends on: Rocket.Chat legacy endpoint does not filter for conversations marked as deleted redlink-gmbh/smarti#259)

Noticed

Conversation artifacts that have been created by resync should be cleaned up within Smarti-MongoDB, since Smarti does not support real deletion of conversations (DELETE /conversation/{conversationId} does not delete the conversation redlink-gmbh/smarti#257)
SmartiAdapter.afterCreateChannel should be triggered by a hook (not by createExpertise, createRequestFromRoom, createEquestFromRoomId)

ThomasRoehl · 2018-06-13T08:49:49Z

Created issues:
#350
#351

(cherry picked from commit 406eca9)

ruKurz · 2018-06-13T16:24:23Z

@ThomasRoehl did you make the end-to-end tests run again?

ruKurz · 2018-06-14T08:52:24Z

The last commit 5644649 fixes #350

all changed

…Rocket.Chat into fix/smarti-results-not-loaded

…sistify/Rocket.Chat into fix/smarti-results-not-loaded

…Rocket.Chat into fix/smarti-results-not-loaded

ThomasRoehl · 2018-06-14T13:25:30Z

Tests are finally running through on my machine, hopefully on circleCi as well.

ruKurz · 2018-06-14T16:02:23Z

tests in circleCI went well. So is there anything keeping us from merging this PR into develop?
@mrsimpson could you please accept the requested changes / finish your review?

Fixing #345 - where outdated conversation id could not be found in sm…

d72c735

…arti

ghost assigned ruKurz Jun 5, 2018

ghost added the progress:working label Jun 5, 2018

ruKurz requested review from ThomasRoehl and mrsimpson June 5, 2018 23:00

mrsimpson requested changes Jun 6, 2018

View reviewed changes

Made serveral changes regarding the review.

8e88469

Added an admin option to re-synch completely.

ruKurz changed the title ~~[FIX] #345 - where outdated conversation ids could not be found in smarti~~ [FIX] #345 - where the smarti conversation cache got out of synch Jun 7, 2018

ruKurz changed the title ~~[FIX] #345 - where the smarti conversation cache got out of synch~~ [FIX] #345 - where smarti conversation cache got out of synch Jun 7, 2018

ruKurz added 2 commits June 7, 2018 22:15

Remove conversations from mapping cache, if a room is being ereased.

002673e

Ordered private dunctions and added some comments.

Fixed i18n keys.

60d2ba2

Get analysis and notify room after deleting a message.

4a34d5b

mrsimpson previously requested changes Jun 8, 2018

View reviewed changes

Removed rate limiter and explicitly perform an conversation analysis.

e1b10d1

ruKurz added 7 commits June 8, 2018 22:08

Merge remote-tracking branch 'origin/develop' into fix/smarti-results…

c5ce192

…-not-loaded

Fixed several synchronisation issues related to the LivechatExternalM…

63f2379

…essage cache.

Removed interface method getRoomId.

f465a96

removed unused SystemLogger

7fb1aba

Improved error handling and debug/error messages.

8055e10

Added missing _ definition

9182716

Resynch Limiter optimizied

8cf8415

Restricted resynch access for admins Extracted synch to Smartiadapter Using getProperties instead legacy impl when sync

mrsimpson reviewed Jun 12, 2018

View reviewed changes

ruKurz added 3 commits June 12, 2018 10:11

Refactored SmartiRelaod

dfd862d

Moved hooks to Assistify AI.

f5cf86c

Simplified clear method.

Make sure conversations being created when a new request, topic or th…

2fe59db

…read is created

mrsimpson reviewed Jun 12, 2018

View reviewed changes

Don't flush the cache if legacy webservice is not used

079ddb2

auto resync on message, resync livechat rooms

3a43bba

Peyman Aparviz and others added 2 commits June 13, 2018 10:50

redlink-gmbh/smarti#225: Improve error handling

e384818

(cherry picked from commit 406eca9)

fix duplicated room creation on new room

dcfa1e0

ruKurz mentioned this pull request Jun 13, 2018

The ConversationWidget is not refreshed correctly redlink-gmbh/smarti#252

Closed

ThomasRoehl and others added 3 commits June 14, 2018 10:25

fixed test and minor bug in smarti adapter

510ae21

fix linting

43c78d0

Use leagcy rocket.chat endpoint again when getting conversations

5644649

ruKurz mentioned this pull request Jun 14, 2018

SmartiAdapter.getConversationId should use the legacy endpoint as fallback #350

Closed

ThomasRoehl and others added 7 commits June 14, 2018 13:34

fixed assistify tests

e7a8276

Merge branch 'fix/smarti-results-not-loaded' of github.com:assistify/…

695a1fc

…Rocket.Chat into fix/smarti-results-not-loaded

removed redundant error logging from proxy

24c08e3

Improved health check before resynch

bd2ba02

Merge branch 'fix/smarti-results-not-loaded' of https://github.com/as…

f966f87

…sistify/Rocket.Chat into fix/smarti-results-not-loaded

fix tests

3cfe09b

Merge branch 'fix/smarti-results-not-loaded' of github.com:assistify/…

e2fbd1f

…Rocket.Chat into fix/smarti-results-not-loaded

ruKurz added 2 commits June 14, 2018 19:04

Merge branch 'develop' into fix/smarti-results-not-loaded

b554ddc

merged with PR #352

d035626

This was referenced Jun 14, 2018

Smarti Widget 0.7.4 #352

Closed

HTML-Tags for highlighting should not be escaped in Smarti results redlink-gmbh/smarti#263

Closed

ruKurz merged commit 41fb6d4 into develop Jun 14, 2018

ghost removed the progress:working label Jun 14, 2018

mrsimpson deleted the fix/smarti-results-not-loaded branch December 7, 2020 09:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] #345 - where smarti conversation cache got out of synch #346

[FIX] #345 - where smarti conversation cache got out of synch #346

ruKurz commented Jun 5, 2018

mrsimpson left a comment

mrsimpson Jun 6, 2018

mrsimpson Jun 6, 2018

mrsimpson Jun 6, 2018

mrsimpson Jun 6, 2018

mrsimpson Jun 6, 2018

mrsimpson Jun 6, 2018

mrsimpson Jun 6, 2018

mrsimpson Jun 6, 2018

mrsimpson Jun 6, 2018

mrsimpson Jun 6, 2018

ruKurz commented Jun 7, 2018

ruKurz commented Jun 7, 2018

mrsimpson left a comment •

edited

Loading

mrsimpson Jun 8, 2018

mrsimpson Jun 8, 2018

mrsimpson Jun 8, 2018

mrsimpson Jun 8, 2018

mrsimpson Jun 8, 2018

mrsimpson commented Jun 8, 2018

ruKurz commented Jun 8, 2018 •

edited

Loading

mrsimpson left a comment

mrsimpson Jun 12, 2018 •

edited

Loading

mrsimpson Jun 12, 2018

mrsimpson left a comment

mrsimpson Jun 12, 2018

ruKurz commented Jun 13, 2018 •

edited by ThomasRoehl

Loading

ThomasRoehl commented Jun 13, 2018

ruKurz commented Jun 13, 2018

ruKurz commented Jun 14, 2018

ThomasRoehl commented Jun 14, 2018

ruKurz commented Jun 14, 2018

		SmartiProxy.propagateToSmarti(verbs.delete, `conversation/${ conversationId }/message/${ message._id }`);
		SmartiAdapter._getAnalysisResult(message.rid, conversationId);

[FIX] #345 - where smarti conversation cache got out of synch #346

[FIX] #345 - where smarti conversation cache got out of synch #346

Conversation

ruKurz commented Jun 5, 2018

mrsimpson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruKurz commented Jun 7, 2018

ruKurz commented Jun 7, 2018

mrsimpson left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrsimpson commented Jun 8, 2018

ruKurz commented Jun 8, 2018 • edited Loading

mrsimpson left a comment

Choose a reason for hiding this comment

mrsimpson Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrsimpson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruKurz commented Jun 13, 2018 • edited by ThomasRoehl Loading

ThomasRoehl commented Jun 13, 2018

ruKurz commented Jun 13, 2018

ruKurz commented Jun 14, 2018

ThomasRoehl commented Jun 14, 2018

ruKurz commented Jun 14, 2018

mrsimpson left a comment •

edited

Loading

ruKurz commented Jun 8, 2018 •

edited

Loading

mrsimpson Jun 12, 2018 •

edited

Loading

ruKurz commented Jun 13, 2018 •

edited by ThomasRoehl

Loading