-
Notifications
You must be signed in to change notification settings - Fork 2
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
SD 272: decs integration #110
Conversation
What
Code changes summary
Why
|
…k queue Add behaviour for SQS integration using sqs-producer Add instructions and queue url to config Add instruction for reading what is on the queue
* Made initial functions to fit submitting application complaint to schema * Assign agent phone to agent details * Add formatting for delay complaints * Move complaintDetails to Parent class constructor and add individual complaint details to child constructor * Add format classes for somethingElse and Existing complaint * Finish formatting for StaffBehaviour * Use hof behaviour functions in behaviour
* Set up tests for AddToCasework * Add tests for complaint base class * Add tests for utils * Add tests for formatComplaintData * Add test for json-converters * Add test for validator throwing errror when undefined value passed in
* SD-272: add custom error page for form failing to submit to queue * SD-272: add max length constraint on all text fields * Add sqs queue config to config file * Increase test timeout * Remove integration test folder * Update node version and fix emailCasweworker config setting * update complaint details max length
2a53920
to
dc59eb4
Compare
@@ -19,6 +19,36 @@ Run the services locally with Docker Compose | |||
$ docker-compose up | |||
``` | |||
|
|||
### Set up AWS SQS queue locally | |||
|
|||
Run AWS services locally using localstack. |
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.
Would you be able to explain the technical architecture before hand, e.g. the data gets sent to a AWS Queue in DECs service.
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.
Okay yeah I can do that. Max and I were gonna make some changes to the README anyway so we can add more about that.
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.
thanks 👍
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.
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.
I don't think you'll have time for this, so maybe just add this as part of a tech debt ticket for the decs intergration
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.
Left a few comments - also please note I haven't properly reviewed the test files yet as I expect they will change based on the suggested changes to the other files.
Any questions give me a shout
Cheers,
Rob
}, | ||
"error": { | ||
"header": "Sorry, your form has not been sent", | ||
"text": "Please try again using the online form" |
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.
Has the copy being approved/reviewed?
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.
This is the wording that was used by the designers on the FBIS service so we also used it here @robertdeniszczyc2
…t class and single list of enums
changed preprod namespace to new preprod namespace
* Updated eslint to version 7.22.0 * Updated .eslintrc file * Fixed the failing eslint errors.
…ng-and-fix-errors SD-720: Fixed eslint errors and upgraded eslint
do QAT need to look through your PR? |
README.md
Outdated
sqs create-queue \ | ||
--queue-name local-queue \ | ||
--endpoint-url http://localhost:4566 \ | ||
--region eu-west-2 \ |
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.
--region eu-west-2 \ | |
--region eu-west-2 |
I had to remove the \
to get this to run
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.
you could make it easier and just write aws sqs create-queue --queue-name local-queue --endpoint-url http://localhost:4566 --region eu-west-2
@@ -1,10 +1,15 @@ | |||
{ | |||
"license": "MIT", | |||
"config": { | |||
"reporter": "--reporter mocha-multi-reporters --reporter-options configFile=test/reporter-config" | |||
}, | |||
"scripts": { | |||
"start": "node app.js", | |||
"start:dev": "hof-build watch --env", |
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.
I know you guys are not responsible for this in the package.json file but I require a .env
file but that's not mentioned in the README. Also it doesn't say which env variables I need
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.
@MaxOSullivan Reckon you could add something about this when you do the readme?
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.
add it as tech debt ticket :)
Hi @joehod I hope you don't mind, I created an example of your work using mostly pure functions rather than classes. I just took one particular part. It's less code, and it doesn't have I think this could be condensed even more. You could just call getComplaints once rather than within each function/class and then add to it where relevant within your switch of if statement. I'm not sure without drilling down into it more Anyway, I'd be interested in your thoughts |
* Set up tests for AddToCasework * Add tests for complaint base class * Add tests for utils * Add tests for formatComplaintData * Add test for json-converters * Add test for validator throwing errror when undefined value passed in
* SD-272: add custom error page for form failing to submit to queue * SD-272: add max length constraint on all text fields * Add sqs queue config to config file * Increase test timeout * Remove integration test folder * Update node version and fix emailCasweworker config setting * update complaint details max length
…t class and single list of enums
…k queue Add behaviour for SQS integration using sqs-producer Add instructions and queue url to config Add instruction for reading what is on the queue
* Made initial functions to fit submitting application complaint to schema * Assign agent phone to agent details * Add formatting for delay complaints * Move complaintDetails to Parent class constructor and add individual complaint details to child constructor * Add format classes for somethingElse and Existing complaint * Finish formatting for StaffBehaviour * Use hof behaviour functions in behaviour
* Set up tests for AddToCasework * Add tests for complaint base class * Add tests for utils * Add tests for formatComplaintData * Add test for json-converters * Add test for validator throwing errror when undefined value passed in
* SD-272: add custom error page for form failing to submit to queue * SD-272: add max length constraint on all text fields * Add sqs queue config to config file * Increase test timeout * Remove integration test folder * Update node version and fix emailCasweworker config setting * update complaint details max length
…t class and single list of enums
"reportDir": "test/executions/mochawesome", | ||
"reportFilename": "unit" | ||
} | ||
} |
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.
new line?
|
||
describe('saveValues', () => { | ||
describe('config.writeToCasework', () => { | ||
it('If false sendToQueue should not be called', () => { |
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.
I usually prefer tests written in prose
e.g. it shouldn't send anything to the queue if it in the configuration it has been set to false
|
||
}); | ||
|
||
describe('If valid complaint data', () => { |
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.
As before, I prefer tests written in prose. It reduces the cognitive overload
so I would rewrite to If the complaint data is valid
}); | ||
|
||
describe('If valid complaint data', () => { | ||
it('formatComplaintData should have been called once with sessionModel attributes', () => { |
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.
as above it should call the method to format the data appropriate i.e. the formatComplaintData
expect(formatComplaintDataStub).to.have.been.calledOnceWith(req.sessionModel.attributes); | ||
}); | ||
|
||
it('sendToQueue should be called once with complaint data', () => { |
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.
as above, it should send the data to the queue
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.
Really great work @joehod man. I know this was a nightmare
I think the main code looks good. Well done.
I've not fully reviewed the tests. I think as part of tech debt it would be good to reword some of these tests to more prose but not important for now.
Top job
Relevant articles for setting up localstack:
https://medium.com/@FloSloot/your-own-local-copy-of-aws-w-node-js-6d98a10533a8
Specific commands are in the readme. Localstack now uses port 4566 which is not the case in the article.