Lcam 976 call stateless from stateful #395

starswan · 2023-10-11T12:47:12Z

https://dsdmoj.atlassian.net/browse/LCAM-976

Call stateless service (SMAS) from stateful service (MAS). This includes a translation layer between stateful integer ids and their 'more friendly' code-based ids (so EMP_INC rather than 135). This will help support future work where CMA (or something similar) invokes the eligibility platform via an HTTP API call rather than a Java call (as now)

Note to reviewer: I think this PR might remove some side-effects from the API - specifically the calculation of the partnerAnnualTotal and applicantAnnualTotal fields (but there may be others). Is there some way we can determine if these are required? There are no tests that assert this behaviour.

Checklist

Before you ask people to review this PR:

Tests should be passing: ./gradlew test
Github should not be reporting conflicts; you should have recently run git rebase main.
Avoid mixing whitespace changes with code changes in the same commit. These make diffs harder to read and conflicts more likely.
You should have looked at the diff against main and ensured that nothing unexpected is included in your changes.
You should have checked that the commit messages say why the change was made.

sonarqubecloud · 2023-10-12T11:27:54Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

95.7% Coverage
0.0% Duplication

Mhowell494

I think we need to have a wider conversation about this work and whether it's really necessary. It doesn't seem to enable Phase 2 in any way and there are serious risks to our production services.

Mhowell494 · 2023-10-16T12:01:15Z

...t/src/main/java/uk/gov/justice/laa/crime/meansassessment/service/MeansAssessmentService.java

        this.assessmentCompletionService = assessmentCompletionService;
        this.meansAssessmentBuilder = meansAssessmentBuilder;
        this.assessmentCriteriaDetailService = assessmentCriteriaDetailService;
        this.incomeEvidenceService = incomeEvidenceService;
        this.crownCourtEligibilityService = crownCourtEligibilityService;
+
+        statelessAssessmentService = new StatelessAssessmentService(assessmentCriteriaService, meansAssessmentServiceFactory, fullAssessmentAvailabilityService);


This should follow the pattern we've used for the other dependent classes. We shouldn't be initialising this ourselves when Spring can do it for us.

I chose this route as the current tests are based on mocking the dependencies. If this change altered the dependencies, it would alter the tests too which felt like a risk

Mhowell494 · 2023-10-16T12:16:29Z

...t/src/main/java/uk/gov/justice/laa/crime/meansassessment/service/MeansAssessmentService.java

+                        requestDTO.getCaseType(),
+                        requestDTO.getMagCourtOutcome(),
+                        incomesFromSectionSummaries(requestDTO.getSectionSummaries(), assessmentCriteria),
+                        requestDTO.getNewWorkReason());


If formatting a method call like this, please place the ); on a new line, I think it really helps the readability.

Mhowell494 · 2023-10-16T12:57:25Z

...t/src/main/java/uk/gov/justice/laa/crime/meansassessment/service/MeansAssessmentService.java

            AssessmentCriteriaEntity assessmentCriteria = assessmentCriteriaService.getAssessmentCriteria(
                    assessmentDate, requestDTO.getHasPartner(), requestDTO.getPartnerContraryInterest());

-            BigDecimal summariesTotal = calculateSummariesTotal(requestDTO, assessmentCriteria);


The answer is yes, the side effects from this method are important. See my comments in the StatelessMeansAssessment class for ideas on how to mitigate this.

Mhowell494 · 2023-10-16T13:00:41Z

...a/uk/gov/justice/laa/crime/meansassessment/service/stateless/StatelessAssessmentService.java

-    private StatelessInitialResult initialResult(@NotNull Assessment assessment,
-                                                 @NotNull BigDecimal totalIncome,
-                                                 @NotNull Map<AgeRange, Integer> childGroupings) {
+    public StatelessInitialResult initialResult(@NotNull Map<AgeRange, Integer> childGroupings,


I think we should remove the incomes parameter here and the outgoings one in the fullResult method. Instead, the methods should accept a BigDecimal total. This way, the MeansAssessmentService can call calculateSummariesTotal and then simply pass in the overall total. Then it's just a case of moving the calcIncomeTotals and calcOutgoingTotals to the execute method.

Mhowell494 · 2023-10-16T13:06:29Z

...a/uk/gov/justice/laa/crime/meansassessment/service/stateless/StatelessAssessmentService.java

        final var children = mapChildGroupings(childGroupings, criteriaEntry.getAssessmentCriteriaChildWeightings());

+        final var totalIncome = calcIncomeTotals(criteriaEntry, caseType, incomes);
+        final var service = meansAssessmentServiceFactory.getService(AssessmentType.INIT);
+
        // assessmentStatus has to be set 'COMPLETE' otherwise the return value is null
        final MeansAssessmentRequestDTO requestDTO = MeansAssessmentRequestDTO
                .builder()
                .childWeightings(children)
                .assessmentStatus(CurrentStatus.COMPLETE)


This is a problem. It's perfectly valid for an assessment to be set as 'Incomplete' in MAAT.

Mhowell494 · 2023-10-16T13:22:37Z

...t/src/main/java/uk/gov/justice/laa/crime/meansassessment/service/MeansAssessmentService.java

-                requestDTO.setEligibilityCheckRequired(crownCourtEligibilityService.isEligibilityCheckRequired(requestDTO));
+            MeansAssessmentDTO completedAssessment;
+            boolean fullAssessmentAvailable;
+            final var childGroupings = childGroupingsFromChildWeightings(requestDTO.getChildWeightings());


Why do we need to convert this when we're just going to do the reverse in the first line of initResult & fullResult? I'd update the methods to take the child weightings instead.

Mhowell494 · 2023-10-16T13:29:10Z

...a/uk/gov/justice/laa/crime/meansassessment/service/stateless/StatelessAssessmentService.java

+    public StatelessFullResult fullResult(Map<AgeRange, Integer> childGroupings,
+                                           AssessmentCriteriaEntity criteriaEntry,
+                                           boolean eligibilityCheckRequired,
+                                           CaseType caseType,
+                                           List<Outgoing> outgoings,
+                                           BigDecimal totalIncome) {


For me, this is too many parameters, however, there is a degree of preference involved. That said, is there a way we could reduce this? For example, moving the mapChildGroupings call? A lot are being passed in just to build the MeansAssessmentRequestDTO, can't we just build this in the execute method instead?

Mhowell494 · 2023-10-16T15:59:14Z

.../gov/justice/laa/crime/meansassessment/service/stateless/MaatToStatelessDataAdapterTest.java

+
+class MaatToStatelessDataAdapterTest {
+
+    private static final AssessmentCriteriaEntity assessmentCriteria = TestModelDataBuilder.getAssessmentCriteriaEntityWithDetails();


The SonarCloud report shows we don't have full coverage of this class.

davidread · 2023-10-17T09:38:03Z

I think we need to have a wider conversation about this work and whether it's really necessary. It doesn't seem to enable Phase 2 in any way

@Mhowell494 it was a while ago now, but this was a key next step agreed when we met on 18th August.

The issue is that the MeansAssessmentService 'MAS' used by MAAT production makes several deep calls into what is now considered the 'stateless business logic', bypassing the new StatelessAssessmentService 'SMAS'. As discussed, it seemed a bit risky having two different ways to call methods assessmentCriteriaSevice.getAssessmentCriteria, calculateSummariesTotal, Full/InitMeansAssesmsnetService.execute and FullAssessmentAvailabilityService. Better to get rid of that duplication and get MAS to call SMAS.

Yes it forces the issue of whether the new SMAS is good enough for MAAT production. Better to do that sooner rather than later. And the development of SMAS has been reviewed and approved by your team, so hopefully not far off.

there are serious risks to our production services

The PR description raises concerns about some untested side-effects and we'd appreciate your views on a practical way forward to address.

Very happy to discuss this all on a all if that helps

Mhowell494 · 2023-10-17T16:49:23Z

I think we need to have a wider conversation about this work and whether it's really necessary. It doesn't seem to enable Phase 2 in any way

@Mhowell494 it was a while ago now, but this was a key next step agreed when we met on 18th August.

The issue is that the MeansAssessmentService 'MAS' used by MAAT production makes several deep calls into what is now considered the 'stateless business logic', bypassing the new StatelessAssessmentService 'SMAS'. As discussed, it seemed a bit risky having two different ways to call methods assessmentCriteriaSevice.getAssessmentCriteria, calculateSummariesTotal, Full/InitMeansAssesmsnetService.execute and FullAssessmentAvailabilityService. Better to get rid of that duplication and get MAS to call SMAS.

Yes it forces the issue of whether the new SMAS is good enough for MAAT production. Better to do that sooner rather than later. And the development of SMAS has been reviewed and approved by your team, so hopefully not far off.

there are serious risks to our production services

The PR description raises concerns about some untested side-effects and we'd appreciate your views on a practical way forward to address.

Very happy to discuss this all on a all if that helps

Thanks @davidread, I think Jude has put a meeting in to discuss tomorrow.

I do recall the discussion on the 18th and believe I raised concerns at the time with the outcome being that we agreed to explore the idea, but as far as I'm aware there's been no discussion since.

I don't believe these changes make Phase 2 any easier to deliver and it's all going to have to be refactored in the future anyway when we update the calculation logic to work with the stateless schema objects. The only thing that's really been eliminated is the call to the FullAssessmentAvailabilityService, in return for a load of new mapping logic.

I did suggest a solution to the side effects problem in my comments above, but it would mean changing several method signatures or duplicated calls to calculateSummariesTotal. There would also need to be some conditional logic to handle the fact that MAAT assessments can have a null result if they're marked as 'Incomplete' in MAS or SMAS.

From my point of view, the underlying logic that deals with the calculations is shared between both implementations and we're confident that's it's robust. So I think for me that small amount of duplication is acceptable in the short term when weighed against the risks of introducing bugs, for what I feel is a fairly limited benefit.

LCAM-976 stateless service refactor

d163cc8

starswan force-pushed the lcam-976-call-stateless-from-stateful branch from 1685c3c to c872ebf Compare October 11, 2023 15:00

MAAT Service now calls stateless service

bfaf435

starswan force-pushed the lcam-976-call-stateless-from-stateful branch 2 times, most recently from 14aa043 to 38afcf8 Compare October 12, 2023 07:57

tidy up test setup

c18ba55

starswan force-pushed the lcam-976-call-stateless-from-stateful branch from 38afcf8 to c18ba55 Compare October 12, 2023 08:05

Sonar cloud fixes

1d4b64e

starswan force-pushed the lcam-976-call-stateless-from-stateful branch from e575bd7 to 1d4b64e Compare October 12, 2023 09:32

starswan marked this pull request as ready for review October 12, 2023 09:33

starswan requested a review from a team as a code owner October 12, 2023 09:33

added more tests

149fb30

starswan marked this pull request as draft October 12, 2023 10:09

starswan added 2 commits October 12, 2023 11:25

remove irrelevant files from coverage numbers

d720b47

add exception tests for new methods in StatelessDataAdapter

91bda3e

starswan marked this pull request as ready for review October 12, 2023 11:04

starswan added 2 commits October 12, 2023 12:18

tidy up optional handling

7780708

rename variable to reflect usage

39a6cbb

Mhowell494 reviewed Oct 16, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lcam 976 call stateless from stateful #395

Lcam 976 call stateless from stateful #395

starswan commented Oct 11, 2023 •

edited

Loading

sonarqubecloud bot commented Oct 12, 2023

Mhowell494 left a comment

Mhowell494 Oct 16, 2023

starswan Oct 17, 2023 •

edited

Loading

Mhowell494 Oct 16, 2023

Mhowell494 Oct 16, 2023

Mhowell494 Oct 16, 2023

Mhowell494 Oct 16, 2023

Mhowell494 Oct 16, 2023

Mhowell494 Oct 16, 2023

Mhowell494 Oct 16, 2023

davidread commented Oct 17, 2023

Mhowell494 commented Oct 17, 2023


		class MaatToStatelessDataAdapterTest {

		private static final AssessmentCriteriaEntity assessmentCriteria = TestModelDataBuilder.getAssessmentCriteriaEntityWithDetails();

Lcam 976 call stateless from stateful #395

Are you sure you want to change the base?

Lcam 976 call stateless from stateful #395

Conversation

starswan commented Oct 11, 2023 • edited Loading

Checklist

sonarqubecloud bot commented Oct 12, 2023

Mhowell494 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starswan Oct 17, 2023 • edited Loading

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

davidread commented Oct 17, 2023

Mhowell494 commented Oct 17, 2023

starswan commented Oct 11, 2023 •

edited

Loading

starswan Oct 17, 2023 •

edited

Loading