feature/cb2-12317: Refactor into services

[me-matt]

Refactor services

Refactor service methods into their own service classes to improve readability, extendability and code reuse.

Refactor services

Note: This is dependent on the repositories refactor PR #186.

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

[matthew2564]

Just checking, what happens here if searchResult is an empty array? Do we need to guard against that? If so, maybe the method would look like

 private async processGetCurrentProvisionalRecords<T extends TechRecordGet['techRecord_vehicleType']>(
    searchResult: ISearchResult[]
  ): Promise<TechRecordType<T> | undefined> {
    if (!searchResult || searchResult.length === 0) {
      throw new Error('Tech record Search returned nothing.');
    }

    const { currentRecords, provisionalRecords } = this.groupRecordsByStatusCode(searchResult);

    const recordToProcess = 
      currentRecords[0] || 
      provisionalRecords[0] || 
      provisionalRecords[1];

    if (!recordToProcess) {
      throw new Error('No suitable record found for processing.');
    }

    return this.techRecordRepository.callGetTechRecords(
      recordToProcess.systemNumber,
      recordToProcess.createdTimestamp
    );
  }
}

Either way, I'm happy. What are your thoughts?

[me-matt]

If searchResult is empty, is should throw the 'Tech record Search returned nothing.' error. Though I'm not a fan of this, it's basically a lift and shift of the existing logic.

I haven't really tried to improve the code on a functional level because of the subsequent changes to testing that would make it harder to know if regressions have been introduced.

In my opinion, errors should be reserved for legitimate unexpected behaviours, whether this meets that criteria (amongst many other places) needs evaluating. Similarly the practice of returning empty collections when an outbound request is successful but returns a 404 should be introduced to make code paths easier to follow.

This is all for a later PR though ;)

[matthew2564]

Might be one for the future, but there is now a groupBy method from ES2023 onwards (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/groupBy).

We could have then had removed this method completely and utilised something like

const { current, provisional } = Object.groupBy(records, (record) => record.techRecord_statusCode)

[me-matt]

Absolutely agree 👍

Also agree that I think functional changes should be in follow up PRs.

[matthew2564]

I'm not sure we need the type coercion here, processGetCurrentProvisionalRecords takes in a generic argument, therefore this can just be

	public getAdrDetails = async (testResult: any) => {
		const searchRes = await this.techRecordRepository.callSearchTechRecords(testResult.systemNumber);
		return await this.processGetCurrentProvisionalRecords<'hgv' | 'trl'>(searchRes);
	};

Where we pass in the vehicle type as T.

We can also remove the generic any argument where this is called and let it be inferred i.e.

const adrDetails = await this.techRecordService.getAdrDetails(testResult);

[me-matt]

Yeah good spot, happy for me to include this in a later PR? (for same reasons as above)

[matthew2564]

Functionally this PR looks good, so happy to defer potential clean ups / improvements for another time. Same goes for all of the comments I've made on this PR. 👍

[cb-cs]

This method is no longer being used as FE now provide the date in the payload, so can be removed. Thanks.

[me-matt]

Good spot, will remove 🙏

[cb-cs]

I think this PR needs a quick rebase, but apart from that agree with the comments made regarding making any functional changes/improvements in subsequent PR's.

[Daniel-Searle]

I'd be inclined to say to merge the defect formatting into one function given that they're both functionally very similar to each other

[me-matt]

Yep agree with this, same as the previous comments. I'm trying to constraint these PRs to structural changes, functional changes will be easier to review in isolation in a follow up PR.

[Daniel-Searle]

I think using a flat map might be more useful here instead of the nested for loops, so would have a flatmap to get the im number, im descirptin, and desc welsh, then another flatmap to go thorgugh the items and destructure the item number etc... then finally return the deficiciencies map and directly return an array, instead of having to make an array and push to it each time

[me-matt]

Same as above comment.