feature/CB2-12313: Refactor certificate generation into commands #193

me-matt · 2024-10-07T10:15:25Z

Refactor certificate generation into commands

Isolates all the certificate payload generation commands into their own classes for easier extendability and readability.

CB2-12313

Checklist

Branch is rebased against the latest develop/common
Necessary id required prepended with "test-" have been checked with automation testers and added
Code and UI has been tested manually after the additional changes
PR title includes the JIRA ticket number
Squashed commits contain the JIRA ticket number
Link to the PR added to the repo
Delete branch after merge

Daniel-Searle · 2024-10-07T10:38:56Z

src/certificate/commands/AdrCertificateCommand.ts

+
+	private certificateIsAnAdr = (): boolean => this.type === CERTIFICATE_DATA.ADR_DATA;
+
+	public initialise(type: CERTIFICATE_DATA, isWelsh = false) {


is welsh isn't being used in a lot of these commands, can they be removed?

So isWelsh is used by some of the commands and so it exists on the initialise for them all to maintain the same interface. We want this because....take a look at how we iterate the collection in

cvs-tsk-cert-gen/src/certificate/CertificatePayloadGenerator.ts

Line 74 in 56df664

this.commands.forEach((cmd) => cmd.initialise(type, isWelsh));

.

If we remove isWelsh for some of the commands we would need to individually initialise / generate each certificate command which isn't particularly DRY, especially when I feel it's a reasonable assertion that isWelsh may be used across those commands in the future. yk?

Thinking about this a bit more, it's fairly typical here to use a StageBag for injecting params...we could refactor that way. It wouldn't have any real functional difference, but would probably read more cleanly...which is really the intended outcome here.

So I have introduced the state bag pattern and refactored the commands to use a base abstract class to make each command more DRY.

Daniel-Searle · 2024-10-07T10:49:57Z

src/certificate/commands/AdrCertificateCommand.ts

Am i right in thinking, that these commands are a way of busting out what was in the certificate service?

Yeah, basically. We want to adhere to the SOLID princples. So each command becomes it's own isolated unit of code to maintain / extend.

Each command becomes easier to parse and extend, in addition the certificate service becomes easier to parse.

In the future we might want to apply a decorator to some of these commands, but again I'm just trying to limit the structural changes here.

cb-cs · 2024-10-07T13:35:17Z

src/certificate/commands/DefectsCommand.ts

+	 * @param flattenedDefects - the list of flattened defects after being retrieved from the defect service
+	 * @param isWelsh - determines whether the atf in which the test result was conducted resides in Wales
+	 */
+	private generateDefects(


In the current develop even though this code is near enough identical, it seems to be implicit that it is an async function. But for some reason it no longer seems to be picking that up with this change, so the linting is coming up with Redundant 'await' for a non-promise type on line 63 for me.

I had this when using eslint. Is your linting running biome?

This function is a hot mess tbh, with the mix of by ref / by val params and lack of typings 😬

Strange, yes I am using Biome for this repo with ESLint disabled.

There is also a tslint.json in this repo which is causing issues in my IDE - it might be the right time to strip that out!

cb-cs · 2024-10-07T14:04:49Z

src/certificate/commands/IvaCertificateCommand.ts

+
+	public generate(testResult: ITestResult): Promise<ICertificatePayload> {
+		if (!this.certificateIsAnIva()) {
+			return Promise.resolve({} as ICertificatePayload);


For consistency is it possible to make this generate method async please, removing the need for the Promise.resolve() wrapper in this return and the return on line 51.

Ah I believe I had it this way because of the same linting reason you mentioned above.

Good spot, I'll update now 🙏

cb-cs · 2024-10-07T14:05:05Z

src/certificate/commands/MsvaCertificateCommand.ts

+		this.type = type;
+	}
+
+	public generate(testResult: ITestResult): Promise<ICertificatePayload> {


For consistency is it possible to make this generate method async please, removing the need for the Promise.resolve() wrapper in this return and the return on line 44.

Same as above :)

matthew2564 · 2024-10-09T07:40:52Z

src/certificate/commands/AdrCertificateCommand.ts

+		const adrDetails: TechRecordType<any> = await this.techRecordService.getAdrDetails(testResult);
+		const makeAndModel = await this.techRecordService.getVehicleMakeAndModel(testResult);


As these appear non dependent, we could optimise this by utilising Promise.all to make these requests in parallel e.g.

const [adrDetails, makeAndModel] = await Promise.all([ this.techRecordService.getAdrDetails(testResult), this.techRecordService.getVehicleMakeAndModel(testResult) ]);

Good spot! Will update 👍

matthew2564 · 2024-10-09T07:46:03Z

src/certificate/commands/DefectsCommand.ts

+	private async getPayloadData(testResult: ITestResult, type: CERTIFICATE_DATA): Promise<any> {
+		const { isWelsh } = this.state;
+
+		let defectListFromApi: IDefectParent[] = [];


It looks like defectListFromApi is only hydrated inside the if statement. Therefore, if we convert it to a const within the if statement, we save creating the variable for non welsh tests and remove the let e.g.

let flattenedDefects: IFlatDefect[] = []; if (isWelsh) { const defectListFromApi = await this.defectRepository.getDefectTranslations(); flattenedDefects = this.defectService.flattenDefectsFromApi(defectListFromApi); } // and so on

src/certificate/commands/DefectsCommand.ts

matthew2564 · 2024-10-09T08:37:57Z

src/certificate/commands/MakeAndModelCommand.ts

+		const makeAndModel = await this.techRecordService.getVehicleMakeAndModel(testResult);
+
+		const trnRegistration = this.testResultService.isValidForTrn(vehicleType, makeAndModel as any)
+			? await this.trailerRepository.getTrailerRegistrationObject(testResult.vin, makeAndModel.Make as any)
+			: undefined;


Looks as if the makeAndModel keys should always exist based on the types package, so to coerce this, I think we should be able to do

const makeAndModel = await this.techRecordService.getVehicleMakeAndModel(testResult) as Required<IMakeAndModel>;

This means we can remove the casting to any.

cb-cs · 2024-10-09T09:33:21Z

This is just more of a general comment, I can't speak for how big the code was prior to the updates, but I think once the changes have been implemented a good ticket to look at would be minification, as we did with authoriser and logs.

The code is currently sitting at over 50mbs.

me-matt · 2024-10-09T09:42:57Z

This is just more of a general comment, I can't speak for how big the code was prior to the updates, but I think once the changes have been implemented a good ticket to look at would be minification, as we did with authoriser and logs.

The code is currently sitting at over 50mbs.

Yep, totally agree. There's currently no optimisations in place. I'm expecting tree shaking and minification to be delivered through the repository template improvement.

cb-cs · 2024-10-10T12:29:17Z

src/certificate/CertificatePayloadGenerator.ts

+	 * Initialises the certificate generation process.
+	 * @param type The type of certificate to generate
+	 * @param isWelsh True if a Welsh certificate should also be generated.
+	 */


Not the end of the world but it looks like this was the doc block prior to the state bag implementation.

I'll remove this in another PR so we don't have to re-request reviews, if that's ok? 😄

Yes no problem, thanks.

isolated the certificate commands

56df664

me-matt requested review from matthew2564, naathanbrown, m-coslett, cb-cs, Daniel-Searle and m-mullen October 7, 2024 10:15

Daniel-Searle reviewed Oct 7, 2024

View reviewed changes

cb-cs reviewed Oct 7, 2024

View reviewed changes

me-matt added 2 commits October 8, 2024 09:25

pr feedback updated generate to be async

69379f1

refactored to use base abstract class and state bag

7a40977

matthew2564 reviewed Oct 9, 2024

View reviewed changes

src/certificate/commands/DefectsCommand.ts Show resolved Hide resolved

matthew2564 reviewed Oct 9, 2024

View reviewed changes

me-matt added 3 commits October 9, 2024 17:01

pr feedback - remove any on trailer registration

6989781

pr feedback - move var init

f458143

pr feedback - parallelise fetch

bb6fae0

me-matt requested review from Daniel-Searle, cb-cs and matthew2564 October 9, 2024 16:10

pr feedback - remove old lint rules

ed0ec8e

matthew2564 approved these changes Oct 10, 2024

View reviewed changes

cb-cs reviewed Oct 10, 2024

View reviewed changes

cb-cs approved these changes Oct 10, 2024

View reviewed changes

Daniel-Searle approved these changes Oct 10, 2024

View reviewed changes

me-matt merged commit bbea5eb into develop Oct 11, 2024
7 checks passed

me-matt deleted the feature/CB2-12313 branch October 11, 2024 08:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/CB2-12313: Refactor certificate generation into commands #193

feature/CB2-12313: Refactor certificate generation into commands #193

me-matt commented Oct 7, 2024

Daniel-Searle Oct 7, 2024

me-matt Oct 8, 2024

me-matt Oct 8, 2024

me-matt Oct 8, 2024

Daniel-Searle Oct 7, 2024

me-matt Oct 8, 2024

cb-cs Oct 7, 2024

me-matt Oct 8, 2024

me-matt Oct 8, 2024

cb-cs Oct 9, 2024

matthew2564 Oct 9, 2024

cb-cs Oct 7, 2024

me-matt Oct 8, 2024

cb-cs Oct 7, 2024

me-matt Oct 8, 2024

matthew2564 Oct 9, 2024

me-matt Oct 9, 2024

matthew2564 Oct 9, 2024

matthew2564 Oct 9, 2024

cb-cs commented Oct 9, 2024

me-matt commented Oct 9, 2024

cb-cs Oct 10, 2024

me-matt Oct 10, 2024

cb-cs Oct 11, 2024


		private certificateIsAnAdr = (): boolean => this.type === CERTIFICATE_DATA.ADR_DATA;

		public initialise(type: CERTIFICATE_DATA, isWelsh = false) {

		const adrDetails: TechRecordType<any> = await this.techRecordService.getAdrDetails(testResult);
		const makeAndModel = await this.techRecordService.getVehicleMakeAndModel(testResult);

feature/CB2-12313: Refactor certificate generation into commands #193

feature/CB2-12313: Refactor certificate generation into commands #193

Conversation

me-matt commented Oct 7, 2024

Refactor certificate generation into commands

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cb-cs commented Oct 9, 2024

me-matt commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment