Added:Control Certer Backend APIs #45
Conversation
controllers/ControlCenter.js
Outdated
| try { | ||
| const { orderId } = req.body | ||
| const messageBody = CANCEL_BOOKING_MESSAGE; | ||
| const getOrderFulfillmentDetails = await axios.get( |
There was a problem hiding this comment.
We could re-use the call_api function instead of all axios calls.
controllers/ControlCenter.js
Outdated
| } from '../utils/constants.js' | ||
|
|
||
| const action = new Actions() | ||
| const TOURISM_STRAPI_URL = process.env.TOURISM_STRAPI_URL || '' |
There was a problem hiding this comment.
New environment variables need to be added to the .env.sample file
server.js
Outdated
| dotenv.config() | ||
| import express from 'express' | ||
| import bodyParser from 'body-parser' | ||
| import logger from './utils/logger.js' | ||
| import messageController from './controllers/Bot.js' | ||
| import DBService from './services/DBService.js' | ||
|
|
||
| import { notificationController } from './controllers/Notification.js' |
There was a problem hiding this comment.
Can't find this controller.
controllers/ControlCenter.js
Outdated
| } | ||
| ) | ||
|
|
||
| await action.send_message(TWILIO_RECEPIENT_NUMBER, messageBody) |
There was a problem hiding this comment.
Why are we using twilio recipient number, shouldn't this be the user's number?
There was a problem hiding this comment.
From where should the user's number be provided, from the payload sent by the FE?? @mayurvir
controllers/ControlCenter.js
Outdated
| } | ||
|
|
||
|
|
||
| export const notificationController = async (req, res) => { |
There was a problem hiding this comment.
Not sure we we can put one controller inside another. Why can't we use a function for this?
There was a problem hiding this comment.
Need Clarity for this @mayurvir
services/notificationService.js
Outdated
|
|
||
| const client = Twilio(TWILIO_ACCOUNT_SID, TWILIO_AUTH_TOKEN) | ||
|
|
||
| export const sendWhatsappNotification = async ( |
There was a problem hiding this comment.
Can't we use the send_message() from Actions.js service here?
utils/constants.js
Outdated
| @@ -0,0 +1,11 @@ | |||
| export const STRAPI_TOURISM_TOKEN = | |||
There was a problem hiding this comment.
lets keep this under the config folder. Its not a utility. As a practise, things that are not keys etc. should be kept here so Token should be an environment variable whereas TOURISM_STRAPI_URL should be under config .
shreyvishal
force-pushed
the
feat/create_control_center
branch
from
a9090a2 to
93ea8ac
Compare
shreyvishal
force-pushed
the
feat/create_control_center
branch
from
93ea8ac to
f25fdaa
Compare
config/openai.json
Outdated
| @@ -11,6 +11,14 @@ | |||
| { "role": "system", "content": "A typical order flow should be search > select > init > confirm."}, | |||
| { "role": "system", "content": "Use the response of search from assistant to select items from the list of items provided by the assistant."}, | |||
| { "role": "system", "content": "Use the response of search request from assistant for filling transaction_id, bpp_id, bpp_uri in the context of all calls except `search`."}, | |||
| { "role": "system", "content": "For `select`, `init`, `confirm`, you must use the item `id` as part of the payload for selected item instead of name or any other key."} | |||
There was a problem hiding this comment.
Why is the prompt changed for line #14?
config/openai.json
Outdated
| ], | ||
| "PRESETS" : { | ||
| "bap_id": "mit-ps-bap.becknprotocol.io", | ||
| "bap_uri": "https://mit-ps-bap.becknprotocol.io", |
There was a problem hiding this comment.
This should not be here under openai.json.
One that things like bap_id, bap_uri etc are now part of registry.json
Strapi url is not openai related config, it should be under a different file like config.json or constants.js file that you have created.
controllers/ControlCenter.js
Outdated
| const config = JSON.parse(readFileSync('./config/openai.json')) | ||
|
|
||
| const action = new Actions() | ||
| const STRAPI_TOURISM_TOKEN = process.env.STRAPI_TOURISM_TOKEN || '' |
There was a problem hiding this comment.
No need to re-decalre STRAPI_TOURISM_TOKEN or STRAPI_TOURISM_TOKEN as they can be used directly from the environment variables.
controllers/ControlCenter.js
Outdated
| try { | ||
| const { orderId } = req.body | ||
| if(!orderId){ | ||
| return res.status(500).json({message:"Order Id is Required", status:false}) |
There was a problem hiding this comment.
500 is for internal server error, you should use 400 for a bad request , typically used for missing data.
controllers/ControlCenter.js
Outdated
| } | ||
| const validOrderId = await action.call_api(`${config.PRESETS.TOURISM_STRAPI_URL}/orders/${orderId}`,'GET',{},{ Authorization: `Bearer ${STRAPI_TOURISM_TOKEN}`}) | ||
| if(!validOrderId.status){ | ||
| return res.send({ message: `Invalid Order Id`, status:false }) |
There was a problem hiding this comment.
Status should be 400, its sending 200 when the order id is invalid. Isn't it a bad request?
| describe('API tests for /notify endpoint for an end to end Notify Request', () => { | ||
| it('Should test unsuccess response for invalid whatsapp number.', async () => { | ||
| const response = await request(app).post('/notify').send({ | ||
| "userNo":"+919123456789" |
There was a problem hiding this comment.
This looks like a valid number, it should be something like "INVALID_NUMBER", a string is invalid number and it will defiitely faill. Test cases should be written in such a way that they always do what they are supposed to do. They should not be flaky (which works sometimes and dont work sometimes)
| const response = await request(app).post('/notify').send({ | ||
|
|
||
| }) | ||
| expect(response._body.status.status).to.not.equal('failed') |
There was a problem hiding this comment.
If its succes case, why are we testing for failure?
| const response = await request(app).post('/notify').send({ | ||
| "userNo":process.env.TEST_RECEPIENT_NUMBER | ||
| }) | ||
| expect(response._body.status.status).to.not.equal('failed') |
There was a problem hiding this comment.
If its a sucess, response, why is the status being test against failed?
| const response = await request(app).post('/cancel-booking').send({ | ||
| "orderId":"1" | ||
| }) | ||
| expect(response._body.message).equal('Notification delivered') |
There was a problem hiding this comment.
Should check for status also.
config/openai.json
Outdated
| ], | ||
| "PRESETS" : { | ||
| "bap_id": "mit-ps-bap.becknprotocol.io", | ||
| "bap_uri": "https://mit-ps-bap.becknprotocol.io", |
config/openai.json
Outdated
| @@ -11,6 +11,14 @@ | |||
| { "role": "system", "content": "A typical order flow should be search > select > init > confirm."}, | |||
| { "role": "system", "content": "Use the response of search from assistant to select items from the list of items provided by the assistant."}, | |||
| { "role": "system", "content": "Use the response of search request from assistant for filling transaction_id, bpp_id, bpp_uri in the context of all calls except `search`."}, | |||
| { "role": "system", "content": "For `select`, `init`, `confirm`, you must use the item `id` as part of the payload for selected item instead of name or any other key."} | |||
tests/unit/services/actions.test.js
Outdated
| @@ -46,21 +46,22 @@ describe.skip('Test cases for process_instruction function', ()=> { | |||
| }) | |||
| }) | |||
|
|
|||
| describe('should test send_message()', () => { | |||
| describe.only('should test send_message()', () => { | |||
Description
Added:Control Certer Backend APIs
Features covered
Create a control center
Changes made
Added APIs for Control Center UI
Screenshots/recordings
NA
Testing
NA
Self checks
Reviewer Suggestions
NA