Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

741 transformed messages not showing correctly #800

Merged
merged 30 commits into from
Feb 5, 2024

Conversation

pmarsh-scottlogic
Copy link
Contributor

@pmarsh-scottlogic pmarsh-scottlogic commented Jan 25, 2024

Description

Fixes a bug where transformed messages would appear differently after page refresh. A user's transformed message stays shown with the original message in bold, and the info message "xml tagging enabled, your message has been transformed" remains.

Screenshots

image

Notes

main ticket changes

  • We have the type transformedMessage which keeps a pre-message, message and post-message string, allowing us to render those parts of the string differently in the chat feed. The backend chat history now stores an optional transformedMessage of this type, which is returned to the frontend for rendering. This fixes the transformed message appearing differently after refresh.
  • To fix the info message disappearing, we now add the info message to the backend's chat history as part of the logic which does the transformation to the user's message in the backend. To get the info message to the frontend for rendering in the chat box feed, we return it as a new property in ChatHttpResponse

testing

  • New unit test added to chatController.test which checks that the history is updated properly with the transformed message and info message.
  • Refactors the unit tests for transforming messages and moves them into their own file.

refactoring

  • combines two if statemets in processChatResponse which were both checking if we had met the win condition.
  • renames incrementNumCompletedLevels to updateNumCompletedLevels (this is called when we win a level. Renamed it because we don't increment if the beaten level has been beaten before.)
  • renames ChatHistoryMessage to ChatMessageDTO (this is the object that is returned when the frontend requests the entire chat history)
  • refactors getChatHistory method in frontend's chatService.ts just to make it read a bit more clearly.
  • transformedMessage, transformedMessageInfo and transformedMessageCombined are now packaged under one object: messageTransformation. It is an object of this type, which is now returned by transformMessage
  • We remove the isTriggered property from Defence because it wasn't being used anymore (replaced by defenceReport)

Concerns

  • Happy to move some refactorings to a separate PR if requested.

Checklist

Have you done the following?

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

@pmarsh-scottlogic pmarsh-scottlogic linked an issue Jan 25, 2024 that may be closed by this pull request
@pmarsh-scottlogic pmarsh-scottlogic marked this pull request as ready for review January 25, 2024 14:37
Copy link
Member

@chriswilty chriswilty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some potential type cleanup, but this is good work 👍

backend/src/controller/chatController.ts Outdated Show resolved Hide resolved
backend/src/controller/chatController.ts Show resolved Hide resolved
Copy link
Member

@chriswilty chriswilty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, just some very minor advice on using undefined.

backend/src/controller/chatController.ts Outdated Show resolved Hide resolved
backend/src/controller/chatController.ts Outdated Show resolved Hide resolved
backend/src/defence.ts Outdated Show resolved Hide resolved
Copy link
Member

@chriswilty chriswilty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful 👌

@pmarsh-scottlogic pmarsh-scottlogic merged commit 6f436bb into dev Feb 5, 2024
2 checks passed
@pmarsh-scottlogic pmarsh-scottlogic deleted the 741-transformed-messages-not-showing-correctly branch February 5, 2024 08:16
chriswilty pushed a commit that referenced this pull request Apr 8, 2024
* combines two separate bits of logic for winning level in processChatResponse

* renames increamentNumCompletedLevels to updateNumCompletedLevels

* renames ChatHistoryMessage to ChatMessageDTO

* refactors getChatHistory

* further refactors getChatHistory to remove immutability

* refactors makeChatMessageFromDTO

* removed some outdated comments

* adds reminder comment and transformedMessage as propery to chatHistoryMessage

* sets transformed message inn chat history

* adds ability to retrieve transformed message from the dto

* add the transformed info message in backend rather than frontend

* adds the transformedMessageInfo to the chat response so it can be shown in the frontend

* combine message transformation objects into one object

* tidies random sequence transformation test

* finalise random sequence transformation test

* tidy up xml tagging transformation test

* tidy up xml tagging transformation test with escaping

* removes unnecessary test and reorders

* moves no transformation into transformation test block and removes unused stuff from file

* moves transform message tests into separate test file

* remove isTriggered from defence object

* complete message transformation test

* use undefined instead of null for transofrmed messages

* updates test

* removes isTriggered from test to make it pass

* implements undefined tricks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transformed messages not showing correctly
2 participants