Skip to content

Instantly share code, notes, and snippets.

@vojtapol
Created April 17, 2026 16:14
Show Gist options
  • Select an option

  • Save vojtapol/0122f16ad29993fd9b6ff6e4d43dc852 to your computer and use it in GitHub Desktop.

Select an option

Save vojtapol/0122f16ad29993fd9b6ff6e4d43dc852 to your computer and use it in GitHub Desktop.

Review: PR #6934 - ED-18731 record OEM XML and display it in console

Summary

Adds logging of OEM lead distribution request/response payloads to S3 and a new lead_distribution_log table, with a console view for browsing and downloading logs. Well-structured change that captures the payloads at the right layer (client classes) and surfaces them through a clean console UI. A few issues worth addressing before merge.

Key Code Paths

Path: Lead distribution with event logging

When LeadDistributionService.sendLeadActivityToProgram() runs (transactionally), it delegates to the appropriate program service (Shift/Ford/Mitsubishi/VW). Each service's client class (ShiftClient, FordDirectDelivrClient, UniteDigitalClient) now serializes the request to a string first, sends the string, captures the raw response string, deserializes it, and wraps all three in a LeadDistributionClientResult<T>. The service layer builds LeadDistributionEvent objects from these results and returns them in LeadDistributionResponse.events. Back in LeadDistributionService, LeadDistributionLogService.logEvents() uploads the payloads to S3 and inserts a lead_distribution_log row for each event.

Needs verification: the logEvents call sits inside the same try/catch as service.sendLead(). If the DB insert in logEvents throws (constraint violation, connection issue), the exception is caught by the outer catch block, which adds FAIL to the statuses array - even though the lead was successfully distributed to the OEM. See finding below.

When the Shift or VW flow includes a lead-status check and that check fails with ShiftLeadNotFoundException or ShiftException, the getLeadStatus method returns a LeadStatusResult with clientResult = null, which means no log event is created for the status call. This is acceptable since there's nothing meaningful to log when we couldn't reach the API.

Path: Console log viewer

The LeadDistributionLogsView renders a grid backed by LeadDistributionLogRepository, which queries lead_distribution_log left-joined to lead_activity (to resolve the lead ID). Filters are applied server-side via jOOQ conditions. Download buttons create Vaadin DownloadHandler instances that stream S3 objects directly to the browser. The S3 lifecycle policy transitions to STANDARD_IA at 90 days, GLACIER_IR at 180 days, and expires at 365 days - so download from console will stop working for files older than 6 months unless Glacier IR instant retrieval is factored in (it is - Glacier IR supports synchronous access, so this is fine).

Findings

edealer-core/src/main/java/com/edealer/core/feature/leadactivity/distribution/LeadDistributionService.java

[important] Line 84: If leadDistributionLogService.logEvents() throws (e.g., DB insert failure), the exception is caught by the outer catch block, which adds FAIL to statuses. But the lead was already successfully sent to the OEM. Logging failure should not affect the distribution status.

// current
response = service.sendLead(new LeadDistributionRequest(integration, thirdPartyLeadSources, leadActivity, rooftopIntegrationAttributes));
leadDistributionLogService.logEvents(leadActivity.getId(), leadDistributionProgram, response.getEvents());
statuses.add(response.getStatus());

// suggested
response = service.sendLead(new LeadDistributionRequest(integration, thirdPartyLeadSources, leadActivity, rooftopIntegrationAttributes));
statuses.add(response.getStatus());
try {
    leadDistributionLogService.logEvents(leadActivity.getId(), leadDistributionProgram, response.getEvents());
} catch (Exception logException) {
    log.error("Failed to log lead distribution events for lead activity ID `{}`.", leadActivity.getId(), logException);
}

Moving statuses.add before the logging call and wrapping logEvents in its own try/catch ensures a logging failure is isolated from the distribution outcome.

edealer-core/src/main/java/com/edealer/core/feature/leadactivity/distribution/log/LeadDistributionLogRepository.java

[important] Line 50: The sentTo condition uses le (<=), but sentTo is set to the start of the next day (exclusive boundary). A record timestamped exactly at midnight of the next day would be incorrectly included.

// current
condition = condition.and(LEAD_DISTRIBUTION_LOG.SENT.le(sentTo));

// suggested
condition = condition.and(LEAD_DISTRIBUTION_LOG.SENT.lt(sentTo));

[nit] Lines 63-72: The count query selects all fields from LEAD_DISTRIBUTION_LOG and unconditionally joins LEAD_ACTIVITY, but a count only needs selectOne(). The LEFT JOIN also only serves the leadId filter - when leadId is null, the join is unnecessary overhead.

// current
return jooq.fetchCount(
    jooq.select(LEAD_DISTRIBUTION_LOG.fields())
        .from(LEAD_DISTRIBUTION_LOG)
        .leftJoin(LEAD_ACTIVITY).onKey()
        .where(condition)
);

// suggested - at minimum, drop the unnecessary field selection
return jooq.fetchCount(
    jooq.selectOne()
        .from(LEAD_DISTRIBUTION_LOG)
        .leftJoin(LEAD_ACTIVITY).onKey()
        .where(condition)
);

edealer-core/src/main/java/com/edealer/core/feature/leadactivity/distribution/volkswagen/VolkswagenLeadDistributionService.java

[important] Line 6: Wildcard import. The project convention is to use explicit imports.

// current
import com.edealer.core.feature.leadactivity.distribution.*;

// suggested
import com.edealer.core.feature.leadactivity.distribution.CheckUniteDigitalLeadResponse;
import com.edealer.core.feature.leadactivity.distribution.LeadDistributionClientResult;
import com.edealer.core.feature.leadactivity.distribution.LeadDistributionProgram;
import com.edealer.core.feature.leadactivity.distribution.LeadDistributionProgramService;
import com.edealer.core.feature.leadactivity.distribution.LeadDistributionRequest;
import com.edealer.core.feature.leadactivity.distribution.LeadDistributionResponse;
import com.edealer.core.feature.leadactivity.distribution.LeadDistributionUniteDigital;
import com.edealer.core.feature.leadactivity.distribution.SendUniteDigitalLeadResponse;
import com.edealer.core.feature.leadactivity.distribution.UniteDigitalClient;
import com.edealer.core.feature.leadactivity.distribution.UniteDigitalException;
import com.edealer.core.feature.leadactivity.distribution.UniteDigitalLeadStatusException;

edealer-core/src/main/resources/localization/labels_en.properties

[nit] Line 341: lead-distribution-log is placed before lead-activity, but alphabetically it should come after it (lead-a < lead-d).

# current
enum.entity-type.lead = Lead
enum.entity-type.lead-distribution-log = Lead Distribution Log
enum.entity-type.lead-activity = Lead Activity

# suggested
enum.entity-type.lead = Lead
enum.entity-type.lead-activity = Lead Activity
enum.entity-type.lead-distribution-log = Lead Distribution Log

edealer-core/src/main/resources/db/migration/V199__add_lead_distribution_log.sql

[nit] Line 7: Column alignment is inconsistent - already raised by the bot and confirmed by Victor. Repeating here for tracking: request_s3_key needs padding to match the other columns.

Checklist Verification

  • Title formatted as ED-???? descriptive title
  • SQL migration is backwards compatible (new table, no impact on existing code)
  • GraphQL changes are backwards compatible (no GraphQL changes)
  • Reverse SQL migration not provided in PR description (missing DROP TABLE lead_distribution_log)
  • Terraform changes verified via tofu plan previews (all green)
  • Tests updated (EntityRepositoryIT, ApiTestConfiguration mock)

Verdict

Has issues that should be addressed. The logEvents exception handling masking distribution status is the most important item - the rest are style/minor correctness fixes.

Total: 0 critical, 3 important, 3 nits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment