diff --git a/scripts/migrate-event-address-nullable.sql b/scripts/migrate-event-address-nullable.sql new file mode 100644 index 0000000..d64b984 --- /dev/null +++ b/scripts/migrate-event-address-nullable.sql @@ -0,0 +1,16 @@ +-- Migration: allow events.address to be NULL +-- Run on any environment with strict schema management (staging / production). +-- Dev uses ddl-auto=update, but Hibernate's "update" mode never DROPS an existing NOT NULL +-- constraint, so this must be applied by hand even on dev DBs created before this change. +-- +-- WHY: +-- Scraped events sometimes arrive without an address. Import previously failed the whole scrape +-- batch on the events.address NOT NULL constraint — one address-less row rolled back every event +-- in that cycle, so nothing reached the ops portal. address is now optional in the entity: the +-- event is kept for admin review and geocoded later once an admin supplies an address via the +-- ops portal edit endpoint (PUT /admin/event/{id}). +-- +-- ROLLBACK (only safe once no rows have address IS NULL): +-- ALTER TABLE events ALTER COLUMN address SET NOT NULL; + +ALTER TABLE events ALTER COLUMN address DROP NOT NULL; diff --git a/src/main/java/org/smalltech/hashtaglocal_backend/entity/EventEntity.java b/src/main/java/org/smalltech/hashtaglocal_backend/entity/EventEntity.java index 6e310ce..3426813 100644 --- a/src/main/java/org/smalltech/hashtaglocal_backend/entity/EventEntity.java +++ b/src/main/java/org/smalltech/hashtaglocal_backend/entity/EventEntity.java @@ -79,8 +79,12 @@ public class EventEntity { /** * Raw address string copied directly from the event data source (e.g., the Excel sheet). Kept as * a human-readable fallback while `location_id` is still null. + * + *

Nullable: scraped events sometimes arrive without an address. We keep the event anyway (for + * admin review) rather than dropping it — geocoding skips events with a null address until an + * admin supplies one via the ops portal edit endpoint. */ - @Column(length = 1024, nullable = false) + @Column(length = 1024) private String address; /** URL to the original event page on the source platform. */ diff --git a/src/main/java/org/smalltech/hashtaglocal_backend/service/EventImportService.java b/src/main/java/org/smalltech/hashtaglocal_backend/service/EventImportService.java index ae536a6..4b526d9 100644 --- a/src/main/java/org/smalltech/hashtaglocal_backend/service/EventImportService.java +++ b/src/main/java/org/smalltech/hashtaglocal_backend/service/EventImportService.java @@ -1,22 +1,17 @@ package org.smalltech.hashtaglocal_backend.service; -import java.util.ArrayList; import java.util.List; import java.util.Locale; import java.util.regex.Pattern; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.smalltech.hashtaglocal_backend.dto.ScrapeEventDTO; -import org.smalltech.hashtaglocal_backend.entity.EventApprovalEntity; import org.smalltech.hashtaglocal_backend.entity.EventEntity; import org.smalltech.hashtaglocal_backend.entity.MediaEntity; -import org.smalltech.hashtaglocal_backend.model.EventApprovalStatus; import org.smalltech.hashtaglocal_backend.model.EventPortalModel; import org.smalltech.hashtaglocal_backend.model.EventTypeModel; -import org.smalltech.hashtaglocal_backend.repository.EventApprovalRepository; import org.smalltech.hashtaglocal_backend.repository.EventRepository; import org.springframework.stereotype.Service; -import org.springframework.transaction.annotation.Transactional; /** * Handles importing events from the scrape service JSON response. @@ -52,22 +47,25 @@ public class EventImportService { private final EventService eventService; private final EventRepository eventRepository; private final EventImageService eventImageService; - private final EventApprovalRepository eventApprovalRepository; /** * Processes a list of events from the scrape service, deduplicates against the database, and - * bulk-saves all new events in a single transaction. + * saves each new event (with its {@code PENDING} approval row) in its own transaction. + * + *

Events are persisted independently, so a single bad row — a constraint violation, a failed + * image upload, etc. — only skips that event instead of rolling back the whole batch. A missing + * address does not drop the event: it is kept for admin review and geocoded later once an + * address is supplied via the ops portal. * * @param scrapeEvents raw event DTOs from the scrape service response - * @return the number of events actually saved (duplicates excluded) + * @return the number of events actually saved (duplicates and failures excluded) */ - @Transactional public int importFromScrapeResponse(List scrapeEvents) { if (scrapeEvents == null || scrapeEvents.isEmpty()) { return 0; } - List toSave = new ArrayList<>(); + int imported = 0; for (ScrapeEventDTO dto : scrapeEvents) { try { @@ -84,8 +82,7 @@ public int importFromScrapeResponse(List scrapeEvents) { continue; } // fromString() returns null for portals it doesn't recognise. portal is a NOT NULL - // column, and toSave is bulk-inserted in one transaction, so letting a null portal - // through here would roll back — and silently drop — the entire import batch. + // column, so letting a null through would fail the insert for this event. if (EventPortalModel.fromString(dto.getPortal()) == null) { log.warn( "Skipping event '{}' — unrecognized portal '{}'", dto.getName(), dto.getPortal()); @@ -104,33 +101,21 @@ public int importFromScrapeResponse(List scrapeEvents) { continue; } - toSave.add(toEntity(dto, media)); + // Persist this event (plus its PENDING approval) in its own transaction. Isolating each + // save means one problematic event never takes the rest of the batch down with it. + eventService.saveWithPendingApproval(toEntity(dto, media)); + imported++; } catch (Exception e) { log.warn("Skipping event '{}' due to error: {}", dto.getName(), e.getMessage()); } } - List saved = eventService.saveAll(toSave); - - // Create a PENDING approval row for every newly imported event so it lands in the - // admin review queue before appearing on the public site. - List approvals = - saved.stream() - .map( - e -> - EventApprovalEntity.builder() - .eventId(e.getId()) - .status(EventApprovalStatus.PENDING) - .build()) - .toList(); - eventApprovalRepository.saveAll(approvals); - log.info( "Imported {} new events ({} received, {} skipped)", - toSave.size(), + imported, scrapeEvents.size(), - scrapeEvents.size() - toSave.size()); - return toSave.size(); + scrapeEvents.size() - imported); + return imported; } private EventEntity toEntity(ScrapeEventDTO dto, MediaEntity media) { @@ -141,12 +126,25 @@ private EventEntity toEntity(ScrapeEventDTO dto, MediaEntity media) { .type(parseEventType(dto.getType())) .startTime(dto.getStartTime()) .endTime(dto.getEndTime()) - .address(dto.getAddress()) + .address(normalizeAddress(dto.getAddress())) .link(stripUtmParams(dto.getLink())) .media(media) .build(); } + /** + * Normalises the scraped address: trims surrounding whitespace and collapses a blank or missing + * value to {@code null}. Storing null (rather than an empty string) keeps the geocoding query + * ({@code findByLocationIsNullAndAddressIsNotNull}) from repeatedly picking up un-geocodable + * events. + */ + private String normalizeAddress(String address) { + if (address == null || address.isBlank()) { + return null; + } + return address.strip(); + } + private String stripUtmParams(String link) { if (link == null || link.isBlank()) { return link; diff --git a/src/main/java/org/smalltech/hashtaglocal_backend/service/EventService.java b/src/main/java/org/smalltech/hashtaglocal_backend/service/EventService.java index dd90eb1..323577a 100644 --- a/src/main/java/org/smalltech/hashtaglocal_backend/service/EventService.java +++ b/src/main/java/org/smalltech/hashtaglocal_backend/service/EventService.java @@ -72,6 +72,25 @@ public List saveAll(List events) { return eventRepository.saveAll(events); } + /** + * Persists a single event together with a fresh {@code PENDING} approval row in one transaction, + * so the event lands in the admin review queue atomically. + * + *

Used by the scraper import: each event is saved independently, so a single failing row (a + * constraint violation, a bad field, etc.) rolls back only that event instead of the whole + * import batch. + */ + @Transactional + public EventEntity saveWithPendingApproval(EventEntity event) { + EventEntity saved = eventRepository.save(event); + eventApprovalRepository.save( + EventApprovalEntity.builder() + .eventId(saved.getId()) + .status(EventApprovalStatus.PENDING) + .build()); + return saved; + } + /** * Returns all {@code APPROVED} events that have a resolved location, mapped to {@link EventData}. * diff --git a/src/test/java/org/smalltech/hashtaglocal_backend/service/EventImportServiceTest.java b/src/test/java/org/smalltech/hashtaglocal_backend/service/EventImportServiceTest.java index 855a25a..1be5c56 100644 --- a/src/test/java/org/smalltech/hashtaglocal_backend/service/EventImportServiceTest.java +++ b/src/test/java/org/smalltech/hashtaglocal_backend/service/EventImportServiceTest.java @@ -28,7 +28,6 @@ import org.smalltech.hashtaglocal_backend.model.EventPortalModel; import org.smalltech.hashtaglocal_backend.model.EventTypeModel; import org.smalltech.hashtaglocal_backend.model.MediaTypeModel; -import org.smalltech.hashtaglocal_backend.repository.EventApprovalRepository; import org.smalltech.hashtaglocal_backend.repository.EventRepository; /** @@ -44,7 +43,6 @@ class EventImportServiceTest { @Mock private EventService eventService; @Mock private EventRepository eventRepository; @Mock private EventImageService eventImageService; - @Mock private EventApprovalRepository eventApprovalRepository; @InjectMocks private EventImportService eventImportService; @@ -79,11 +77,11 @@ private ScrapeEventDTO dto(String name, String type, String portal, LocalDateTim .build(); } - @SuppressWarnings("unchecked") private List capturedSavedEvents() { - ArgumentCaptor> captor = ArgumentCaptor.forClass(List.class); - verify(eventService).saveAll(captor.capture()); - return captor.getValue(); + ArgumentCaptor captor = ArgumentCaptor.forClass(EventEntity.class); + // atLeast(0): captures every per-event save, and tolerates the zero-save cases (all skipped). + verify(eventService, atLeast(0)).saveWithPendingApproval(captor.capture()); + return captor.getAllValues(); } // --------------------------------------------------------------------------- @@ -153,9 +151,8 @@ void mapsPortalStringToEventPortalModel(String raw, String expected, String desc List.of(dto("Test Event", "TREKANDPLOG", raw, START_TIME))); if (expected == null) { - // Unrecognised/null portals are skipped, not saved: portal is a NOT NULL column and the - // batch is bulk-inserted in one transaction, so letting a null portal through would roll - // back the whole import. See EventImportService#importFromScrapeResponse. + // Unrecognised/null portals are skipped, not saved: portal is a NOT NULL column, so letting a + // null portal through would fail the insert. See EventImportService#importFromScrapeResponse. assertTrue( capturedSavedEvents().isEmpty(), "event with unrecognised portal '" + raw + "' should be skipped, not saved"); @@ -224,7 +221,7 @@ void skipsDuplicateEvent() { List.of(dto("Trek and Plog", "TREKANDPLOG", "Team everest", START_TIME))); assertEquals(0, count); - verify(eventService).saveAll(List.of()); + verify(eventService, never()).saveWithPendingApproval(any()); } @Test @@ -281,4 +278,48 @@ void returnsZeroForNullOrEmptyList() { assertEquals(0, eventImportService.importFromScrapeResponse(List.of())); verifyNoInteractions(eventRepository); } + + @Test + @DisplayName("Event with a blank/missing address is kept, with address normalised to null") + void keepsEventWithMissingAddress() { + when(eventRepository.existsByNameAndStartTime(any(), any())).thenReturn(false); + ScrapeEventDTO noAddress = + ScrapeEventDTO.builder() + .name("Trek and Plog") + .organisation("Org") + .portal("Team everest") + .type("TREKANDPLOG") + .startTime(START_TIME) + .address(" ") // blank — must not cause the event to be dropped + .link("https://example.com") + .image("https://example.com/image.jpg") + .build(); + + int count = eventImportService.importFromScrapeResponse(List.of(noAddress)); + + assertEquals(1, count, "a missing address must not drop the event"); + assertNull( + capturedSavedEvents().get(0).getAddress(), "blank address should be normalised to null"); + } + + @Test + @DisplayName("One event failing to save does not abort the rest of the batch") + void oneFailingEventDoesNotAbortBatch() { + LocalDateTime t1 = LocalDateTime.of(2026, 2, 21, 5, 0); + LocalDateTime t2 = LocalDateTime.of(2026, 3, 7, 0, 0); + when(eventRepository.existsByNameAndStartTime(any(), any())).thenReturn(false); + // First per-event save blows up; the second must still be attempted and succeed. + when(eventService.saveWithPendingApproval(any())) + .thenThrow(new RuntimeException("constraint violation")) + .thenReturn(EventEntity.builder().build()); + + int count = + eventImportService.importFromScrapeResponse( + List.of( + dto("Bad Event", "TREKANDPLOG", "Team everest", t1), + dto("Good Event", "TREKANDPLOG", "Team everest", t2))); + + assertEquals(1, count, "only the good event counts as imported"); + verify(eventService, times(2)).saveWithPendingApproval(any()); + } }