[DRAFT]PP-13333 parity checker checks exemption data matches #5669

hjvoid · 2024-12-13T10:38:32Z

WHAT YOU DID

When the historical events emitter is emitting events for a change in connector, if the charge has an exemption_3ds_requested or exemption_3ds value then when the historical events emitter emits the events for the charge it should include a GATEWAY_3DS_EXEMPTION_RESULT_OBTAINED with specific event details dependant upon the combination of exemption 3ds data as per below.

Connector `exemption3dsRequested`	Connector `exemption3ds`	Ledger `exemption`
OPTIMISED	HONOURED	`"exemption": {"requested": true,"outcome": {"result": "honoured"}}`
OPTIMISED	REJECTED	`"exemption": {"requested": true,"outcome": {"result": "rejected"}}`
OPTIMISED	OUT_OF_SCOPE	`"exemption": {"requested": true,"outcome": {"result": "out of scope"}}`
CORPORATE	HONOURED	`"exemption": {"requested": true,"type": "corporate","outcome": {"result": "honoured"}}`
CORPORATE	REJECTED	`"exemption": {"requested": true,"type": "corporate","outcome": {"result": "rejected"}}`
CORPORATE	OUT_OF_SCOPE	`"exemption": {"requested": true,"type": "corporate","outcome": {"result": "out of scope"}}`
null	HONOURED	`"exemption": {"requested": true,"outcome": {"result": "honoured"}}`
null	REJECTED	`"exemption": {"requested": true,"outcome": {"result": "rejected"}}`
null	EXEMPTION_NOT_REQUESTED	`"exemption": {"requested": false}`
null	EXEMPTION_OUT_OF_SCOPE	`"exemption": {"requested": true,"outcome": {"result": "out of scope"}}`
null	null	No `exemption` object

How to test

TBC

Code review checklist

TBD

Logging

TBC

…d ledger

… logic

alexbishop1

Looks good. Mostly stylistic changes requested. A couple of small logic changes and some more tests also.

alexbishop1 · 2024-12-18T10:15:12Z

src/main/java/uk/gov/pay/connector/tasks/HistoricalEventEmitter.java

+            var chargeEvent = chargeEventEntities
+                    .stream()
+                    .filter(event -> event.getStatus() == AUTHORISATION_SUCCESS || event.getStatus() == AUTHORISATION_REJECTED)
+                    .findFirst();
+
+            chargeEvent.ifPresent(event -> {
+                var threeDsInfoEvent = Gateway3dsExemptionResultObtained.from(charge, event.getUpdated().toInstant());
+                boolean hasBeenEmittedBefore = emittedEventDao.hasBeenEmittedBefore(threeDsInfoEvent);
+                if (forceEmission || !hasBeenEmittedBefore) {
+                    eventService.emitAndRecordEvent(threeDsInfoEvent, getDoNotRetryEmitUntilDate());
+                }
+            });


This can all be done as one:

chargeEventEntities .stream() .filter(event -> event.getStatus() == AUTHORISATION_SUCCESS || event.getStatus() == AUTHORISATION_REJECTED) .findFirst() .map(event -> Gateway3dsExemptionResultObtained.from(charge, event.getUpdated().toInstant())) .filter(threeDsInfoEvent -> forceEmission || !emittedEventDao.hasBeenEmittedBefore(threeDsInfoEvent)) .ifPresent(threeDsInfoEvent -> eventService.emitAndRecordEvent(threeDsInfoEvent, getDoNotRetryEmitUntilDate()));

I think threeDsInfoEvent has been copied and pasted from elsewhere. gateway3dsExemptionResultObtainedEvent would be better.

Can we add some logging (inside the ifPresent(…) to say we’re outputting the event (we have similar for other events)?

alexbishop1 · 2024-12-18T10:24:13Z

src/main/java/uk/gov/pay/connector/tasks/HistoricalEventEmitter.java

+            var chargeEvent = chargeEventEntities
+                    .stream()
+                    .filter(event -> TERMINAL_AUTHENTICATION_STATES.contains(event.getStatus()))
+                    .findFirst();
+
+            chargeEvent.ifPresent(event -> {
+                var requested3dsExemptionEvent = Requested3dsExemption.from(charge, event.getUpdated().toInstant());
+                boolean hasBeenEmittedBefore = emittedEventDao.hasBeenEmittedBefore(requested3dsExemptionEvent);
+                if (forceEmission || !hasBeenEmittedBefore) {
+                    eventService.emitAndRecordEvent(requested3dsExemptionEvent, getDoNotRetryEmitUntilDate());
+                }
+            });


Similarly:

chargeEventEntities .stream() .filter(event -> TERMINAL_AUTHENTICATION_STATES.contains(event.getStatus())) .findFirst() .map(event -> Requested3dsExemption.from(charge, event.getUpdated().toInstant())) .filter(requested3dsExemptionEvent -> forceEmission || !emittedEventDao.hasBeenEmittedBefore(requested3dsExemptionEvent)) .ifPresent(requested3dsExemptionEvent -> eventService.emitAndRecordEvent(requested3dsExemptionEvent, getDoNotRetryEmitUntilDate()));

Also add some logging here too.

alexbishop1 · 2024-12-18T10:30:04Z

src/main/java/uk/gov/pay/connector/tasks/service/ChargeParityChecker.java

+        if (chargeEntity.getExemption3dsRequested() == null) {
+            return CONNECTOR_HAS_EXEMPTION_3DS_REQUESTED_NULL;
+        } else {
+            if (chargeEntity.getExemption3dsRequested() == Exemption3dsType.OPTIMISED) {
+                return CONNECTOR_HAS_EXEMPTION_3DS_REQUESTED_OPTIMISED;
+            } else  {
+                return CONNECTOR_HAS_EXEMPTION_3DS_REQUESTED_CORPORATE;
+            }
+        }


This can all be done in a single elegant switch expression that explicitly enumerates all possible values:

return switch (chargeEntity.getExemption3dsRequested()) { case null -> CONNECTOR_HAS_EXEMPTION_3DS_REQUESTED_NULL; case OPTIMISED -> CONNECTOR_HAS_EXEMPTION_3DS_REQUESTED_OPTIMISED; case CORPORATE -> CONNECTOR_HAS_EXEMPTION_3DS_REQUESTED_CORPORATE; };

alexbishop1 · 2024-12-18T10:31:16Z

src/main/java/uk/gov/pay/connector/tasks/service/ChargeParityChecker.java

+        if (chargeEntity.getExemption3ds() == null) {
+            return CONNECTOR_HAS_EXEMPTION_NULL;
+        } else {
+            return switch (chargeEntity.getExemption3ds()) {
+                case EXEMPTION_HONOURED -> CONNECTOR_HAS_EXEMPTION_HONOURED;
+                case EXEMPTION_REJECTED -> CONNECTOR_HAS_EXEMPTION_REJECTED;
+                case EXEMPTION_NOT_REQUESTED -> CONNECTOR_HAS_EXEMPTION_NOT_REQUESTED;
+                case EXEMPTION_OUT_OF_SCOPE -> CONNECTOR_HAS_EXEMPTION_OUT_OF_SCOPE;
+            };
+        }


Because we’re living in the future, switch expressions can handle null now:

return switch (chargeEntity.getExemption3ds()) { case null -> CONNECTOR_HAS_EXEMPTION_NULL; case EXEMPTION_HONOURED -> CONNECTOR_HAS_EXEMPTION_HONOURED; case EXEMPTION_REJECTED -> CONNECTOR_HAS_EXEMPTION_REJECTED; case EXEMPTION_NOT_REQUESTED -> CONNECTOR_HAS_EXEMPTION_NOT_REQUESTED; case EXEMPTION_OUT_OF_SCOPE -> CONNECTOR_HAS_EXEMPTION_OUT_OF_SCOPE; };

alexbishop1 · 2024-12-18T10:45:57Z

src/main/java/uk/gov/pay/connector/tasks/service/ChargeParityChecker.java

 import static uk.gov.pay.connector.tasks.service.LedgerAuthorisationSummaryState.LEDGER_HAS_NO_AUTHORISATION_SUMMARY;
 import static uk.gov.pay.connector.tasks.service.LedgerAuthorisationSummaryState.LEDGER_HAS_AUTHORISATION_SUMMARY_WITH_THREE_D_S_REQUIRED_FALSE;
 import static uk.gov.pay.connector.tasks.service.LedgerAuthorisationSummaryState.LEDGER_HAS_AUTHORISATION_SUMMARY_WITH_THREE_D_S_REQUIRED_TRUE;
 import static uk.gov.pay.connector.tasks.service.LedgerAuthorisationSummaryState.LEDGER_HAS_SOMETHING_COMPLETELY_DIFFERENT;
+import static uk.gov.pay.connector.tasks.service.LedgerExemptionState.*;


We don’t like wildcard import statements

alexbishop1 · 2024-12-18T11:33:40Z

src/test/java/uk/gov/pay/connector/events/HistoricalEventEmitterServiceTest.java

+        historicalEventEmitterService.emitHistoricEventsById(1L, OptionalLong.empty(), 1L);
+
+        ArgumentCaptor<Gateway3dsExemptionResultObtained> argument = ArgumentCaptor.forClass(Gateway3dsExemptionResultObtained.class);
+        verify(eventService, times(1)).emitAndRecordEvent(argument.capture(), isNotNull());


No need for times(1): it’s the default

alexbishop1 · 2024-12-18T11:41:30Z

src/test/java/uk/gov/pay/connector/events/HistoricalEventEmitterServiceTest.java

+                Arguments.of(EXEMPTION_NOT_REQUESTED, AUTHORISATION_SUCCESS),
+                Arguments.of(EXEMPTION_HONOURED, AUTHORISATION_SUCCESS),
+                Arguments.of(EXEMPTION_REJECTED, AUTHORISATION_SUCCESS),
+                Arguments.of(EXEMPTION_OUT_OF_SCOPE, AUTHORISATION_SUCCESS)


Presumably we want AUTHORISATION_REJECTED too?

alexbishop1 · 2024-12-18T11:42:09Z

src/test/java/uk/gov/pay/connector/events/HistoricalEventEmitterServiceTest.java

+    }
+
+    @Test
+    void shouldCheckEventTypeIsNot3dsWhen3dsDetailsSetToNull(){


Not sure what this is supposed to be testing

Clarified with clearer name.

alexbishop1 · 2024-12-18T11:47:10Z

src/test/java/uk/gov/pay/connector/events/HistoricalEventEmitterServiceTest.java

+                Arguments.of(CORPORATE, "CORPORATE", EXEMPTION_HONOURED, AUTHORISATION_REJECTED),
+                Arguments.of(CORPORATE, "CORPORATE", EXEMPTION_REJECTED, AUTHORISATION_REJECTED),
+                Arguments.of(CORPORATE, "CORPORATE", EXEMPTION_OUT_OF_SCOPE, AUTHORISATION_REJECTED)
+        );


There are other terminal authorisation states as well but it would be silly to test them all

alexbishop1 · 2024-12-18T12:04:12Z

src/test/java/uk/gov/pay/connector/tasks/service/ChargeParityCheckerTest.java

+                Arguments.of(OPTIMISED, EXEMPTION_HONOURED, "honoured", true),
+                Arguments.of(OPTIMISED, EXEMPTION_REJECTED, "rejected", true),
+                Arguments.of(OPTIMISED, EXEMPTION_OUT_OF_SCOPE, "out of scope", true)
+        );


I think we want (possibly as part of another test) ones with corporate exemptions too.

And probably at least some where it doesn’t match (focus on the most likely ones, like connector having an exemption result but ledger not).

hjvoid force-pushed the PP-13333_parity_checker_checks_exemption_data branch 2 times, most recently from f1b61f9 to 06efe47 Compare December 16, 2024 09:59

PP-13333 parity checker checks exemption data matches in connector an…

8bd30c9

…d ledger

hjvoid force-pushed the PP-13333_parity_checker_checks_exemption_data branch from 06efe47 to 8bd30c9 Compare December 16, 2024 12:51

hjvoid added 4 commits December 16, 2024 15:59

Add exemption3dsRequested and exemption3ds to ChargeParityChecker

755b489

Add Exemption models, Exemption3dsState enums and chargeParityChecker…

521d13a

… logic

Add tests for scenarios with no exemptionOutcome potentially null

ad030ae

Remove superfluous line from parameterized test

e691a03

alexbishop1 requested changes Dec 18, 2024

View reviewed changes

Stylistic changes and small logic changes

9916f35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT]PP-13333 parity checker checks exemption data matches #5669

[DRAFT]PP-13333 parity checker checks exemption data matches #5669

hjvoid commented Dec 13, 2024 •

edited

Loading

alexbishop1 left a comment

alexbishop1 Dec 18, 2024

alexbishop1 Dec 18, 2024

alexbishop1 Dec 18, 2024

alexbishop1 Dec 18, 2024

alexbishop1 Dec 18, 2024

alexbishop1 Dec 18, 2024

alexbishop1 Dec 18, 2024

alexbishop1 Dec 18, 2024

hjvoid Dec 18, 2024

alexbishop1 Dec 18, 2024

alexbishop1 Dec 18, 2024

alexbishop1 Dec 18, 2024

hjvoid Dec 18, 2024

alexbishop1 Dec 18, 2024

alexbishop1 Dec 18, 2024

[DRAFT]PP-13333 parity checker checks exemption data matches #5669

Are you sure you want to change the base?

[DRAFT]PP-13333 parity checker checks exemption data matches #5669

Conversation

hjvoid commented Dec 13, 2024 • edited Loading

WHAT YOU DID

How to test

Code review checklist

Logging

alexbishop1 left a 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

hjvoid commented Dec 13, 2024 •

edited

Loading