-
Notifications
You must be signed in to change notification settings - Fork 10
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
828 streamline configure defence info message network call #874
Changes from 4 commits
4509230
03d475a
63f61c5
7c3509b
b124a09
9cf750f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,4 +110,5 @@ export { | |
setGptModel, | ||
addInfoMessageToChatHistory, | ||
getChatMessagesFromDTOResponse, | ||
makeChatMessageFromDTO, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,6 @@ | ||
import { DEFAULT_DEFENCES } from '@src/Defences'; | ||
import { ChatMessage } from '@src/models/chat'; | ||
import { ConfigureDefenceResponse } from '@src/models/combined'; | ||
import { | ||
DEFENCE_ID, | ||
DefenceConfigItem, | ||
|
@@ -9,6 +11,7 @@ import { | |
import { LEVEL_NAMES } from '@src/models/level'; | ||
|
||
import { sendRequest } from './backendService'; | ||
import { makeChatMessageFromDTO } from './chatService'; | ||
|
||
const PATH = 'defence/'; | ||
|
||
|
@@ -54,15 +57,21 @@ async function configureDefence( | |
defenceId: DEFENCE_ID, | ||
config: DefenceConfigItem[], | ||
level: LEVEL_NAMES | ||
): Promise<boolean> { | ||
): Promise<ChatMessage | null> { | ||
const response = await sendRequest(`${PATH}configure`, { | ||
method: 'POST', | ||
headers: { | ||
'Content-Type': 'application/json', | ||
}, | ||
body: JSON.stringify({ defenceId, config, level }), | ||
}); | ||
return response.status === 200; | ||
|
||
const { resultingChatInfoMessage } = | ||
(await response.json()) as ConfigureDefenceResponse; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be problematic because the response type in the case of errors is not application/json, it's text/plain. So I expect Better to check the status first, and if Ok then parse the JSON and make your DTO. And if it's not Ok, what happens to the error message? Looks like we are swallowing that currently. Update Yep we are ignoring defence config errors when we could at least be writing something to the console, or better still, showing an error message somewhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, good call to check the status first. And yes, the message gets swallowed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not-swallowing the errors is a wider UI problem I reckon There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's that "unhappy paths" issue come back to bite us 😁 |
||
|
||
return response.status === 200 | ||
? makeChatMessageFromDTO(resultingChatInfoMessage) | ||
: null; | ||
} | ||
|
||
async function resetDefenceConfigItem( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string
orobject
- is that cos we have some error responses with a string body?What do we do with those error responses? I'm now checking your changes in defenceService...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sometimes we return an error message a simple string. Also yes, we swallow those. I suppose they remain helpful because you can see them in the browser inspector.
A note on these request types: This second part (ResBody) seems entirely redundant. TypeScript doesn't seem to enforce the types at all when you call
res.send(...)
. Are we missing something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep I noticed that as well. I couldn't at the time work out what we were doing wrong, but now I look again, I think we should be accessing the response object from the request, i.e.
req.res
instead of passing a separate, untyped Response object to our handler functions.