708 move logic for detecting output defence bot filtering

[gsproston-scottlogic]

Description

Refactor which moves the logic for detecting triggered defences on the bot output out of the openai.ts file.

Notes

• the chat controller is now in charge of adding user and bot messages to the chat history.
• new pushMessageToHistory util method which trims the chat history length when a new message is added to it.

Concerns

• I refactored chatGptSendMessage to return a list of emails that were sent as a result of a user's message, rather than returning the list of ALL emails sent that level. I was also tempted to do the same for chat history... so return a list of new messages rather than the list of ALL messages for that level. Turns out this is a bit of work, so I've made Have chatGptSendMessage only return new chat messages #756 for this.

Checklist

Have you done the following?

• Linked the relevant Issue
• Added tests
• Ensured the workflow steps are passing

[pmarsh-scottlogic]

this logic deals with combining the defence reports, which was removed in Heather's branch here

[chriswilty]

I still need to give the entire PR another look... Just a couple of suggestions for tidy-up so far.

[chriswilty]

Turns out this transformMessage function is only called from here (plus tests). The result is then modified using combineTransformedMessage in two different places.

Additionally, combineTransformedMessage is only called from this function, and from transformMessage itself, so you are duplicating the effort.

Better to have the return of transformMessage already combined into a single string (or undefined). That will simplify the logic elsewhere.

[pmarsh-scottlogic]

Not quite sure I understand.

However, it's important to get the transformed message with the TransformedChatMessage type, and save that to history, for that is how we do the styling in the front end (where the message is rendered in bold).

interface TransformedChatMessage {
	preMessage: string;
	message: string;
	postMessage: string;
	transformationName: string;
}

I agree that we only need a plain string for the purpose of getting a reply. But there's only one place in the code where we need that (arg to chatGptSendMessage(...)).

[chriswilty]

Ah yes, I see now that we are returning the full TransformedChatMessage in updatedChatResponse (although not the history, actually). But we are invoking combineTransformedMessage twice. I wonder if the transformMessage function itself might do that work and pass it back in its return value? If you don't want to modify the TransformedChatMessage type, then you could pass it back as a separate property, e.g.

const { transformedMessage, combinedMessage} = transformMessage(message, defences);

Alternatively, you could just declare the result of combineTransformedMessage() as a separate const, so it can be used in both places.

[pmarsh-scottlogic]

I tried making transformMessage(...) return both transformed and combined, but it got a bit gross for instances where we don't have any transformations and want to return null. So I went with your last suggestion which I think is the most simple

[chriswilty]

Wonderful! Big respect for the tests ❤️

[chriswilty]

Ah yes, I see now that we are returning the full TransformedChatMessage in updatedChatResponse (although not the history, actually). But we are invoking combineTransformedMessage twice. I wonder if the transformMessage function itself might do that work and pass it back in its return value? If you don't want to modify the TransformedChatMessage type, then you could pass it back as a separate property, e.g.

const { transformedMessage, combinedMessage} = transformMessage(message, defences);

Alternatively, you could just declare the result of combineTransformedMessage() as a separate const, so it can be used in both places.

[chriswilty]

Lovely job!