Feature/lp 212 name page #49
Conversation
- create code structure - unit test
- create transaction - unit test
- create controller - integration test
- create controller - integration test
- fake next2 for demo
- disable submit if no name ending
- integration test errors
- integration test for next-2 page (demo)
| @@ -14,6 +14,11 @@ | |||
| "start": "node dist/bin/www.js", | |||
| "dev": "nodemon src/bin/www.ts", | |||
| "test": "jest", | |||
| "test:watch": "jest --watch", | |||
There was a problem hiding this comment.
Maybe the 'watch' changes should go in a separate PR (together with README updates, explaining what they do/how to make use of them), then this PR is entirely focussed on the new page and encompassing framework?
There was a problem hiding this comment.
I use watch mode when I am working
I can update the readme
|
|
||
| import TransactionRegistrationType from "../application/registration/TransactionRegistrationType"; | ||
|
|
||
| interface IRegistrationGateway { |
There was a problem hiding this comment.
See what others think, but I think some light documentation in the code on these key interfaces would be helpful (this would be in addition to the Confluence design).
| @@ -0,0 +1,7 @@ | |||
| enum TransactionRegistrationType { | |||
There was a problem hiding this comment.
I think this is simply a list of all the pages in the Registration journey. If yes, could we maybe remove 'Transaction' from the name as it's confusing me and get 'Page' in there? RegistrationPageType perhaps?
I'm also wondering why the value matches the constant name and is in uppercase?
There was a problem hiding this comment.
We said we were going to change the name, maybe use submission instead of transaction.
But certainly not page because these are not pages
| ): TransactionRouting { | ||
| const coordination = this.transactionMap.get(registrationType); | ||
|
|
||
| if (!coordination) { |
There was a problem hiding this comment.
Generally speaking, getting some debug log statements into the f/w will be really useful. E.g. here when the routing is and isn't called, based on whether the 'coordination' (or should it be named coordinator?) is found.
There was a problem hiding this comment.
I am not a big fan of logging everywhere, that makes reading very hard.
But we can add logs if we really need to
| } from "../../config/constants"; | ||
| import TransactionRegistrationType from "./TransactionRegistrationType"; | ||
|
|
||
| export const NAME_TEMPLATE = TransactionRegistrationType.NAME.toLowerCase(); |
There was a problem hiding this comment.
Why bother converting to lowercase - shouldn't the page names just be named correctly to start with? What if we wanted to use/have camelCase?
There was a problem hiding this comment.
I did this to match the template name with the transaction type
But this can be done differently
The only trick is that when a get request is made, the controller extracts the last part of the url and uses it to find out the TransactionType and retrieve the correct routing.
| import { START_URL } from "../../config/constants"; | ||
| import { NAME_URL } from "../../application/registration/Routing"; | ||
|
|
||
| export type TransactionRouting = { |
There was a problem hiding this comment.
Ok, so 'transaction' confusing things here. Would 'PageRequestRouting' be a better name?
There was a problem hiding this comment.
Submission maybe but not page
| transactionId: string, | ||
| data: Record<string, any> | ||
| ): Promise<string> { | ||
| if (!data.partnership_name) { |
There was a problem hiding this comment.
Won't HTML page validation be trapping this? Plan was to not implement custom validation in the web (initially).
There was a problem hiding this comment.
This simulates errors coming from API after sending the request
| const globalErrorHandler = (err: Error, req: Request, res: Response, _next: NextFunction) => { | ||
| logger.errorRequest(req, `An error has occurred. Re-routing to the error screen - ${err.stack}`); | ||
| const globalErrorHandler = ( | ||
| err: Error, |
There was a problem hiding this comment.
I think these are just formatting changes. Could they be reverted (for now)? This PR is already big and then review can be focussed on the new f/w and not reformatting.
There was a problem hiding this comment.
I use eslint and prettier
everyone is supposed to have plugins activated
| import { TransactionRouting } from "../../domain/entities/TransactionRouting"; | ||
|
|
||
| class RegistrationController { | ||
| private registrationService: RegistrationService; |
There was a problem hiding this comment.
I'm wondering why the controller also has a reference to the RegistrationService as this seems to also sit alongside the controller in the IDependencies interface?
There was a problem hiding this comment.
It has no reference
It uses the injected service
| private getTransactionType(path: string) { | ||
| let id = this.templateName(path); | ||
| if (id.includes("-")) { | ||
| id = id.replace("-", "_"); |
There was a problem hiding this comment.
This seems significant... what is it for/doing?
There was a problem hiding this comment.
if the template name has a hyphen but TransactionType underscore
JIRA link
https://companieshouse.atlassian.net/browse/LP-212
Change description
Work checklist
Merge instructions
We are committed to keeping commit history clean, consistent and linear. To achieve this commit should be structured as follows:
and contain the following structural elements:
BREAKING CHANGE:introduces a breaking API change (correlating with MAJOR in semantic versioning). A BREAKING CHANGE can be part of commits of any type,fix:andfeat:are allowed, for examplebuild:,chore:,ci:,docs:,style:,refactor:,perf:,test:, and others,BREAKING CHANGE: <description>may be provided.