From 4c6a87f73a241509b8f3ca66e4e0d1e824e9c7e0 Mon Sep 17 00:00:00 2001 From: Preston Cabe Date: Mon, 5 Oct 2026 20:35:41 -0400 Subject: [PATCH 01/14] Add screener import and export --- .../controller/ScreenerTransferResource.java | 48 +++ .../model/dto/screener/ScreenerTransfer.java | 13 + .../EligibilityCheckRepository.java | 4 + .../persistence/GoogleStorageService.java | 5 + .../builder/persistence/StorageService.java | 2 + .../impl/EligibilityCheckRepositoryImpl.java | 11 +- .../impl/ScreenerRepositoryImpl.java | 4 + .../builder/service/LibraryApiService.java | 24 ++ .../service/ScreenerTransferService.java | 301 ++++++++++++++++++ .../ScreenerTransferResourceTest.java | 63 ++++ .../service/LibraryApiServiceTest.java | 28 ++ .../service/ScreenerTransferServiceTest.java | 297 +++++++++++++++++ builder-frontend/README.md | 8 + .../src/api/screenerTransfer.test.ts | 81 +++++ builder-frontend/src/api/screenerTransfer.ts | 60 ++++ .../homeScreen/EditScreenerForm.tsx | 41 ++- .../homeScreen/ImportScreenerForm.test.tsx | 93 ++++++ .../homeScreen/ImportScreenerForm.tsx | 76 +++++ .../components/homeScreen/ScreenersList.tsx | 16 + docs/src/content/docs/user/user-guide.md | 9 + e2e/tests/exampleScreenerTransfer.spec.ts | 108 +++++++ e2e/tests/screenerTransfer.spec.ts | 171 ++++++++++ 22 files changed, 1461 insertions(+), 2 deletions(-) create mode 100644 builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResource.java create mode 100644 builder-api/src/main/java/org/codeforphilly/bdt/builder/model/dto/screener/ScreenerTransfer.java create mode 100644 builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java create mode 100644 builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResourceTest.java create mode 100644 builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java create mode 100644 builder-frontend/src/api/screenerTransfer.test.ts create mode 100644 builder-frontend/src/api/screenerTransfer.ts create mode 100644 builder-frontend/src/components/homeScreen/ImportScreenerForm.test.tsx create mode 100644 builder-frontend/src/components/homeScreen/ImportScreenerForm.tsx create mode 100644 e2e/tests/exampleScreenerTransfer.spec.ts create mode 100644 e2e/tests/screenerTransfer.spec.ts diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResource.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResource.java new file mode 100644 index 00000000..aac765f7 --- /dev/null +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResource.java @@ -0,0 +1,48 @@ +package org.codeforphilly.bdt.builder.controller; + +import io.quarkus.logging.Log; +import io.quarkus.security.identity.SecurityIdentity; +import jakarta.inject.Inject; +import jakarta.ws.rs.*; +import jakarta.ws.rs.core.*; +import org.codeforphilly.bdt.builder.auth.AuthUtils; +import org.codeforphilly.bdt.builder.model.dto.screener.ScreenerTransfer; +import org.codeforphilly.bdt.builder.service.ScreenerTransferService; +import java.util.Map; + +@Path("/api") +@Produces(MediaType.APPLICATION_JSON) +public class ScreenerTransferResource { + @Inject ScreenerTransferService transferService; + + @GET + @Path("/screener/{screenerId}/export") + public Response exportScreener(@Context SecurityIdentity identity, @PathParam("screenerId") String id) { + String owner = AuthUtils.getUserId(identity); + if (owner == null) return Response.status(Response.Status.UNAUTHORIZED).build(); + try { + return Response.ok(transferService.exportScreener(owner, id)).build(); + } catch (WebApplicationException rejected) { + return rejected.getResponse(); + } catch (Exception failure) { + Log.error("Could not export screener", failure); + return Response.serverError().entity(Map.of("error", "Could not export the complete screener.")).build(); + } + } + + @POST + @Path("/screener/import") + @Consumes(MediaType.APPLICATION_JSON) + public Response importScreener(@Context SecurityIdentity identity, ScreenerTransfer transfer) { + String owner = AuthUtils.getUserId(identity); + if (owner == null) return Response.status(Response.Status.UNAUTHORIZED).build(); + try { + return Response.status(Response.Status.CREATED).entity(transferService.importScreener(owner, transfer)).build(); + } catch (BadRequestException rejected) { + return Response.status(Response.Status.BAD_REQUEST).entity(Map.of("error", rejected.getMessage())).build(); + } catch (Exception failure) { + Log.error("Could not import screener", failure); + return Response.serverError().entity(Map.of("error", "Could not import the screener. Please try again.")).build(); + } + } +} diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/model/dto/screener/ScreenerTransfer.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/model/dto/screener/ScreenerTransfer.java new file mode 100644 index 00000000..f1ba8083 --- /dev/null +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/model/dto/screener/ScreenerTransfer.java @@ -0,0 +1,13 @@ +package org.codeforphilly.bdt.builder.model.dto.screener; + +import com.fasterxml.jackson.databind.JsonNode; +import org.codeforphilly.bdt.builder.model.domain.Benefit; +import org.codeforphilly.bdt.builder.model.domain.EligibilityCheck; +import java.util.List; + +/** One portable draft screener, with DMN models embedded in its custom checks. */ +public record ScreenerTransfer(String format, int formatVersion, String screenerName, + JsonNode formSchema, List benefits, List customChecks) { + public static final String FORMAT = "bdt-screener"; + public static final int VERSION = 1; +} diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/EligibilityCheckRepository.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/EligibilityCheckRepository.java index cd9c5445..c9c11664 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/EligibilityCheckRepository.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/EligibilityCheckRepository.java @@ -27,6 +27,8 @@ public interface EligibilityCheckRepository { Optional getPublishedCustomCheck(String userId, String checkId); + Optional getPublishedCustomCheck(String userId, String checkId, boolean includeArchived); + String getWorkingId(EligibilityCheck check); String newWorkingId(); @@ -46,6 +48,8 @@ public interface EligibilityCheckRepository { void deleteWorkingCustomCheck(String checkId) throws Exception; + void deletePublishedCustomCheck(String checkId) throws Exception; + void updatePublishedCustomCheck(EligibilityCheck check) throws Exception; /* The published document id a check would get at the given version. */ diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/GoogleStorageService.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/GoogleStorageService.java index dd047524..7b564fce 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/GoogleStorageService.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/GoogleStorageService.java @@ -18,6 +18,11 @@ @ApplicationScoped public class GoogleStorageService implements StorageService { + @Override + public void deleteFile(String filePath) { + storage.delete(BlobId.of(bucketName, filePath)); + } + @Inject Storage storage; diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/StorageService.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/StorageService.java index bb865adc..b48a24bd 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/StorageService.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/StorageService.java @@ -7,6 +7,8 @@ import java.util.Optional; public interface StorageService { + void deleteFile(String filePath) throws Exception; + void writeStringToStorage(String filePath, String content, String contentType) throws Exception; void writeBytesToStorage(String filePath, byte[] content, String contentType); diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/EligibilityCheckRepositoryImpl.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/EligibilityCheckRepositoryImpl.java index 5bdacdbb..fdb01f9e 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/EligibilityCheckRepositoryImpl.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/EligibilityCheckRepositoryImpl.java @@ -103,6 +103,10 @@ public Optional getWorkingCustomCheckMetadata(String userId, S } public Optional getPublishedCustomCheck(String userId, String checkId){ + return getPublishedCustomCheck(userId, checkId, false); + } + + public Optional getPublishedCustomCheck(String userId, String checkId, boolean includeArchived){ Optional publishedCheckOpt = getCustomCheck(userId, checkId, true); if (publishedCheckOpt.isEmpty()) { return Optional.empty(); @@ -114,7 +118,7 @@ public Optional getPublishedCustomCheck(String userId, String Optional workingCheckOpt = getWorkingCustomCheck(userId, workingCheckId, true); // If working check exists and is archived, return empty - if (workingCheckOpt.isPresent() && workingCheckOpt.get().getIsArchived()) { + if (!includeArchived && workingCheckOpt.isPresent() && workingCheckOpt.get().getIsArchived()) { return Optional.empty(); } @@ -168,6 +172,11 @@ public void deleteWorkingCustomCheck(String checkId) throws Exception { FirestoreUtils.deleteDocument(CollectionNames.WORKING_CUSTOM_CHECK_COLLECTION, checkId); } + @Override + public void deletePublishedCustomCheck(String checkId) throws Exception { + FirestoreUtils.deleteDocument(CollectionNames.PUBLISHED_CUSTOM_CHECK_COLLECTION, checkId); + } + public String saveNewPublishedCustomCheck(EligibilityCheck check) throws Exception { ObjectMapper mapper = new ObjectMapper().setSerializationInclusion(JsonInclude.Include.NON_NULL); Map data = mapper.convertValue(check, Map.class); diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/ScreenerRepositoryImpl.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/ScreenerRepositoryImpl.java index 73ac601e..e08ed07e 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/ScreenerRepositoryImpl.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/ScreenerRepositoryImpl.java @@ -85,6 +85,10 @@ public Optional getWorkingScreenerMetaDataOnly(String screenerId){ public String saveNewWorkingScreener(Screener screener) throws Exception{ ObjectMapper mapper = new ObjectMapper().setSerializationInclusion(JsonInclude.Include.NON_NULL); Map data = mapper.convertValue(screener, Map.class); + data.remove("formSchema"); + if (screener.getId() != null) { + return FirestoreUtils.persistDocumentWithId(CollectionNames.WORKING_SCREENER_COLLECTION, screener.getId(), data); + } return FirestoreUtils.persistDocument(CollectionNames.WORKING_SCREENER_COLLECTION, data); } diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/LibraryApiService.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/LibraryApiService.java index 2fc0a723..81051ac1 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/LibraryApiService.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/LibraryApiService.java @@ -197,6 +197,30 @@ public List getBenefits() { return benefits; } + /** + * A screener can reference composition-only checks carried by a library benefit. + * Resolve those against trusted benefit metadata without adding them to the public picker. + */ + public Optional getScreenerCheckById(String id) { + return getById(id).or(() -> benefits.stream() + .filter(benefit -> benefit.getChecks() != null) + .flatMap(benefit -> benefit.getChecks().stream()) + .filter(config -> id.equals(config.getSourceCheckId() != null + ? config.getSourceCheckId() : config.getCheckId())) + .findFirst() + .map(config -> { + EligibilityCheck check = new EligibilityCheck(); + check.setId(id); + check.setName(config.getCheckName()); + check.setModule(config.getCheckModule()); + check.setVersion(config.getCheckVersion()); + check.setEvaluationUrl(config.getEvaluationUrl()); + check.setInputDefinition(config.getInputDefinition()); + check.setParameterDefinitions(config.getParameterDefinitions()); + return check; + })); + } + public Optional getBenefitById(String id) { return benefits.stream() .filter(benefit -> id.equals(benefit.getId())) diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java new file mode 100644 index 00000000..a2b86bdb --- /dev/null +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java @@ -0,0 +1,301 @@ +package org.codeforphilly.bdt.builder.service; + +import com.fasterxml.jackson.databind.ObjectMapper; +import io.quarkus.logging.Log; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; +import jakarta.ws.rs.BadRequestException; +import jakarta.ws.rs.ForbiddenException; +import jakarta.ws.rs.NotFoundException; +import org.codeforphilly.bdt.builder.model.domain.*; +import org.codeforphilly.bdt.builder.model.dto.screener.ScreenerTransfer; +import org.codeforphilly.bdt.builder.persistence.*; + +import java.util.*; + +@ApplicationScoped +public class ScreenerTransferService { + private final ScreenerRepository screeners; + private final EligibilityCheckRepository checks; + private final StorageService storage; + private final LibraryApiService library; + private final ObjectMapper mapper = new ObjectMapper(); + + @Inject + public ScreenerTransferService(ScreenerRepository screeners, EligibilityCheckRepository checks, + StorageService storage, LibraryApiService library) { + this.screeners = screeners; + this.checks = checks; + this.storage = storage; + this.library = library; + } + + public ScreenerTransfer exportScreener(String owner, String id) throws Exception { + // Authorize before fetching the form, benefits, or check models. + Screener metadata = screeners.getWorkingScreenerMetaDataOnly(id).orElseThrow(NotFoundException::new); + if (!owner.equals(metadata.getOwnerId())) throw new ForbiddenException(); + Screener screener = screeners.getWorkingScreener(id).orElseThrow(NotFoundException::new); + List benefits = new ArrayList<>(); + Map byId = new HashMap<>(); + for (Benefit benefit : screeners.getBenefitsInScreener(screener)) byId.put(benefit.getId(), benefit); + // The screener's benefit list determines the order shown in the editor. + if (screener.getBenefits() != null) { + for (BenefitDetail detail : screener.getBenefits()) { + Benefit benefit = byId.remove(detail.getId()); + if (benefit == null) throw new IllegalStateException("Missing screener benefit"); + benefits.add(copy(benefit, Benefit.class)); + } + } + if (!byId.isEmpty()) throw new IllegalStateException("Screener benefit list is inconsistent"); + Map customChecks = new LinkedHashMap<>(); + for (Benefit benefit : benefits) { + benefit.setOwnerId(null); + for (CheckConfig config : configs(benefit)) { + String source = sourceId(config); + if (source.startsWith("L")) continue; + if (customChecks.containsKey(source)) continue; + EligibilityCheck check = loadCheck(owner, source); + customChecks.put(source, portableCheck(check)); + String workingId = checks.getWorkingId(check); + if (!customChecks.containsKey(workingId)) { + Optional working = checks.getWorkingCustomCheck(owner, workingId, true); + if (working.isPresent()) { + requireOwnedCheck(owner, working.get()); + customChecks.put(workingId, portableCheck(working.get())); + } + } + } + } + return new ScreenerTransfer(ScreenerTransfer.FORMAT, ScreenerTransfer.VERSION, + screener.getScreenerName(), mapper.valueToTree(screener.getFormSchema()), + benefits, new ArrayList<>(customChecks.values())); + } + + private EligibilityCheck loadCheck(String owner, String id) { + EligibilityCheck check = (id.startsWith("W-") + ? checks.getWorkingCustomCheck(owner, id, true) + : checks.getPublishedCustomCheck(owner, id, true)) + .orElseThrow(() -> new IllegalStateException("Missing custom check " + id)); + requireOwnedCheck(owner, check); + return check; + } + + private void requireOwnedCheck(String owner, EligibilityCheck check) { + // Repository lookups do not themselves enforce ownership. + if (!owner.equals(check.getOwnerId())) throw new ForbiddenException(); + } + + private EligibilityCheck portableCheck(EligibilityCheck original) { + EligibilityCheck check = copy(original, EligibilityCheck.class); + if (blank(check.getDmnModel())) throw new IllegalStateException("Missing custom check DMN model"); + check.setOwnerId(null); + check.setExampleSourceId(null); + check.setDatePublished(null); + check.setEvaluationUrl(null); + return check; + } + + public Screener importScreener(String owner, ScreenerTransfer transfer) throws Exception { + // Validate the complete file and resolve library references before any writes. + Map originals = validate(transfer); + Map libraryChecks = new HashMap<>(); + for (Benefit benefit : transfer.benefits()) { + for (CheckConfig config : configs(benefit)) { + String source = sourceId(config); + if (source.startsWith("L") && !libraryChecks.containsKey(source)) { + libraryChecks.put(source, library.getScreenerCheckById(source).orElseThrow(() -> + new BadRequestException("Library check " + source + " is unavailable on this server."))); + } + } + } + Map newWorkingIds = new LinkedHashMap<>(); + Map workingCopies = new LinkedHashMap<>(); + for (EligibilityCheck original : originals.values()) { + String family = checks.getWorkingId(original); + newWorkingIds.computeIfAbsent(family, ignored -> checks.newWorkingId()); + EligibilityCheck candidate = workingCopies.get(family); + if (candidate == null || original.getId().startsWith("W-") + || (!candidate.getId().startsWith("W-") + && CheckVersion.compare(original.getVersion(), candidate.getVersion()) > 0)) { + workingCopies.put(family, copy(original, EligibilityCheck.class)); + } + } + Deque cleanup = new ArrayDeque<>(); + try { + // A collision gets a separate module, keeping decision names and DMN XML intact. + List existing = checks.getAllWorkingCustomChecks(owner); + for (var entry : workingCopies.entrySet()) { + EligibilityCheck working = entry.getValue(); + working.setId(newWorkingIds.get(entry.getKey())); + resetCheckOwnership(working, owner); + reserveImportedName(working, existing); + cleanup.push(() -> checks.releaseCheckName(owner, working.getModule(), working.getName(), working.getId())); + existing = new ArrayList<>(existing); + existing.add(working); + } + Map remappedIds = new HashMap<>(); + for (var entry : workingCopies.entrySet()) { + EligibilityCheck working = entry.getValue(); + remappedIds.put(entry.getKey(), working.getId()); + cleanup.push(() -> checks.deleteWorkingCustomCheck(working.getId())); + saveCheck(working, false, cleanup); + } + for (EligibilityCheck original : originals.values()) { + if (!original.getId().startsWith("P-")) continue; + String family = checks.getWorkingId(original); + EligibilityCheck published = copy(original, EligibilityCheck.class); + resetCheckOwnership(published, owner); + published.setModule(workingCopies.get(family).getModule()); + published.setId(newWorkingIds.get(family)); + published.setId(checks.getPublishedId(published, published.getVersion())); + remappedIds.put(original.getId(), published.getId()); + cleanup.push(() -> checks.deletePublishedCustomCheck(published.getId())); + saveCheck(published, true, cleanup); + } + List benefits = new ArrayList<>(); + List details = new ArrayList<>(); + for (Benefit original : transfer.benefits()) { + Benefit benefit = copy(original, Benefit.class); + benefit.setId(UUID.randomUUID().toString()); + benefit.setOwnerId(owner); + for (CheckConfig config : configs(benefit)) { + String source = sourceId(config); + config.setCheckId(UUID.randomUUID().toString()); + if (source.startsWith("L")) { + // Use this server's endpoint, never a URL supplied by the file. + config.setSourceCheckId(source); + config.setEvaluationUrl(libraryChecks.get(source).getEvaluationUrl()); + } else { + config.setSourceCheckId(remappedIds.get(source)); + config.setEvaluationUrl(null); + config.setCheckModule(workingCopies.get(checks.getWorkingId(originals.get(source))).getModule()); + } + } + benefits.add(benefit); + details.add(new BenefitDetail(benefit.getId(), benefit.getName(), benefit.getDescription())); + } + Screener screener = Screener.create(owner, transfer.screenerName(), null); + // Make the draft visible only once all its artifacts have been saved. + screener.setId(UUID.randomUUID().toString()); + String id = screener.getId(); + for (Benefit benefit : benefits) { + cleanup.push(() -> screeners.deleteCustomBenefit(id, benefit.getId())); + screeners.saveNewCustomBenefit(id, benefit); + } + if (transfer.formSchema() != null && !transfer.formSchema().isNull()) { + String path = storage.getScreenerWorkingFormSchemaPath(id); + cleanup.push(() -> storage.deleteFile(path)); + storage.writeStringToStorage(path, mapper.writeValueAsString(transfer.formSchema()), "application/json"); + } + screener.setBenefits(details); + cleanup.push(() -> screeners.deleteWorkingScreener(id)); + screeners.saveNewWorkingScreener(screener); + return screener; + } catch (Exception failure) { + while (!cleanup.isEmpty()) { + try { cleanup.pop().run(); } + catch (Exception rollbackFailure) { Log.error("Failed to clean up screener import", rollbackFailure); } + } + throw failure; + } + } + + private void saveCheck(EligibilityCheck check, boolean published, Deque cleanup) throws Exception { + String dmn = check.getDmnModel(); + check.setDmnModel(null); + String path = storage.getCheckDmnModelPath(check.getId()); + cleanup.push(() -> storage.deleteFile(path)); + storage.writeStringToStorage(path, dmn, "application/xml"); + if (published) checks.saveNewPublishedCustomCheck(check); + else checks.saveNewWorkingCustomCheck(check); + } + + private void resetCheckOwnership(EligibilityCheck check, String owner) { + check.setOwnerId(owner); + check.setIsArchived(false); + check.setExampleSourceId(null); + check.setDatePublished(null); + check.setEvaluationUrl(null); + } + + private void reserveImportedName(EligibilityCheck check, List existing) throws Exception { + String module = check.getModule(); + for (int attempt = 1; attempt <= 100; attempt++) { + String candidate = attempt == 1 ? module : module + " (imported " + attempt + ")"; + if (existing.stream().anyMatch(other -> Objects.equals(candidate, other.getModule()) + && Objects.equals(check.getName(), other.getName()))) continue; + try { + checks.reserveCheckName(check.getOwnerId(), candidate, check.getName(), check.getId()); + check.setModule(candidate); + return; + } catch (DocumentAlreadyExistsException collision) { + // Another import or check creation may have claimed this name since the read. + } + } + throw new BadRequestException("Could not find an available module for imported check " + check.getName()); + } + + private Map validate(ScreenerTransfer transfer) { + if (transfer == null || !ScreenerTransfer.FORMAT.equals(transfer.format()) + || transfer.formatVersion() != ScreenerTransfer.VERSION) + throw new BadRequestException("This file is not a supported BDT screener export."); + if (blank(transfer.screenerName()) || transfer.benefits() == null || transfer.customChecks() == null) + throw new BadRequestException("The screener file is missing required fields."); + if (transfer.formSchema() != null && !transfer.formSchema().isNull() && !transfer.formSchema().isObject()) + throw new BadRequestException("The form schema must be an object."); + Map byId = new LinkedHashMap<>(); + for (EligibilityCheck check : transfer.customChecks()) { + if (check == null || blank(check.getId()) || blank(check.getName()) || blank(check.getModule()) + || blank(check.getVersion()) || CheckVersion.parse(check.getVersion()).isEmpty() + || blank(check.getDmnModel()) + || !(check.getId().startsWith("W-") || check.getId().startsWith("P-"))) + throw new BadRequestException("A custom check is incomplete."); + try { + checks.getWorkingId(check); + new CustomCheckDmnRenameValidator().validate(check.getDmnModel(), check.getDmnModel(), + check.getName(), check.getName()); + } catch (Exception invalid) { throw new BadRequestException("Invalid DMN model or ID for check " + check.getName()); } + if (byId.putIfAbsent(check.getId(), check) != null) + throw new BadRequestException("Duplicate custom check ID."); + } + Set benefitIds = new HashSet<>(); + Set referencedFamilies = new HashSet<>(); + Set referencedIds = new HashSet<>(); + for (Benefit benefit : transfer.benefits()) { + if (benefit == null || blank(benefit.getId()) || blank(benefit.getName()) || !benefitIds.add(benefit.getId())) + throw new BadRequestException("Invalid or duplicate benefit."); + for (CheckConfig config : configs(benefit)) { + if (config == null || blank(config.getCheckName())) throw new BadRequestException("Invalid configured check."); + String source = sourceId(config); + if (source.startsWith("L")) continue; + EligibilityCheck check = byId.get(source); + if (check == null) throw new BadRequestException("Missing custom check " + source); + if (!Objects.equals(config.getCheckName(), check.getName()) + || !Objects.equals(config.getCheckVersion(), check.getVersion())) + throw new BadRequestException("Custom check configuration does not match its model."); + referencedIds.add(source); + referencedFamilies.add(checks.getWorkingId(check)); + } + } + for (EligibilityCheck check : byId.values()) { + if (!referencedFamilies.contains(checks.getWorkingId(check)) + || (check.getId().startsWith("P-") && !referencedIds.contains(check.getId()))) + throw new BadRequestException("The file contains an unrelated custom check."); + } + return byId; + } + + private List configs(Benefit benefit) { + return benefit.getChecks() == null ? List.of() : benefit.getChecks(); + } + + private String sourceId(CheckConfig config) { + String id = blank(config.getSourceCheckId()) ? config.getCheckId() : config.getSourceCheckId(); + if (blank(id)) throw new BadRequestException("A configured check is missing its source ID."); + return id; + } + + private boolean blank(String value) { return value == null || value.isBlank(); } + private T copy(T value, Class type) { return mapper.convertValue(value, type); } + @FunctionalInterface private interface Cleanup { void run() throws Exception; } +} diff --git a/builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResourceTest.java b/builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResourceTest.java new file mode 100644 index 00000000..edcd3843 --- /dev/null +++ b/builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResourceTest.java @@ -0,0 +1,63 @@ +package org.codeforphilly.bdt.builder.controller; + +import io.quarkus.security.identity.SecurityIdentity; +import jakarta.ws.rs.BadRequestException; +import jakarta.ws.rs.ForbiddenException; +import org.codeforphilly.bdt.builder.model.domain.Screener; +import org.codeforphilly.bdt.builder.service.ScreenerTransferService; +import org.eclipse.microprofile.jwt.JsonWebToken; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import java.util.Map; +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.*; + +class ScreenerTransferResourceTest { + private final ScreenerTransferResource resource = new ScreenerTransferResource(); + private final ScreenerTransferService service = mock(ScreenerTransferService.class); + private final SecurityIdentity identity = mock(SecurityIdentity.class); + private final JsonWebToken token = mock(JsonWebToken.class); + + @BeforeEach + void setup() { + resource.transferService = service; + when(identity.getPrincipal()).thenReturn(token); + when(token.getClaim("user_id")).thenReturn("recipient"); + } + + @Test + void successfulImportReturnsCreatedWithTheNewDraft() throws Exception { + Screener draft = Screener.create("recipient", "Name", null); + draft.setId("new-screener"); + when(service.importScreener("recipient", null)).thenReturn(draft); + try (var response = resource.importScreener(identity, null)) { + assertEquals(201, response.getStatus()); + assertSame(draft, response.getEntity()); + } + } + + @Test + void invalidFileReturnsAReadableValidationError() throws Exception { + when(service.importScreener("recipient", null)).thenThrow(new BadRequestException("Missing DMN model")); + try (var response = resource.importScreener(identity, null)) { + assertEquals(400, response.getStatus()); + assertEquals(Map.of("error", "Missing DMN model"), response.getEntity()); + } + } + + @Test + void forbiddenExportReturnsForbidden() throws Exception { + when(service.exportScreener("recipient", "other-screener")).thenThrow(new ForbiddenException()); + try (var response = resource.exportScreener(identity, "other-screener")) { + assertEquals(403, response.getStatus()); + } + } + + @Test + void missingIdentityDoesNotCallTransferService() { + when(token.getClaim("user_id")).thenReturn(null); + assertEquals(401, resource.exportScreener(identity, "id").getStatus()); + assertEquals(401, resource.importScreener(identity, null).getStatus()); + verifyNoInteractions(service); + } +} diff --git a/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/LibraryApiServiceTest.java b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/LibraryApiServiceTest.java index b682e465..a00aa7da 100644 --- a/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/LibraryApiServiceTest.java +++ b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/LibraryApiServiceTest.java @@ -64,6 +64,34 @@ void loadBenefitsMetadata_skipsBenefitsWithUnsupportedBindings() throws Exceptio ); } + @Test + void screenerCheckLookupIncludesCompositionChecksFromBenefitMetadata() throws Exception { + LibraryApiService service = new LibraryApiService(); + service.loadMetadata(""" + {"checks":[{"id":"L-public-0.9.0","name":"public"}],"benefits":[{ + "id":"sctf","name":"Senior Citizen Tax Freeze","checks":[{ + "checkId":"L-internal-sctf-age-requirement-0.9.0", + "sourceCheckId":"L-internal-sctf-age-requirement-0.9.0", + "checkName":"sctf-age-requirement","checkModule":"internal","checkVersion":"0.9.0", + "evaluationUrl":"/api/v1/checks/internal/sctf-age-requirement", + "parameters":{},"parameterDefinitions":[] + }] + }]} + """); + + String id = "L-internal-sctf-age-requirement-0.9.0"; + var check = service.getScreenerCheckById(id).orElseThrow(); + assertEquals(id, check.getId()); + assertEquals("internal", check.getModule()); + assertEquals("sctf-age-requirement", check.getName()); + assertEquals("0.9.0", check.getVersion()); + assertEquals("/api/v1/checks/internal/sctf-age-requirement", check.getEvaluationUrl()); + assertTrue(service.getById(id).isEmpty()); + assertEquals(1, service.getAll().size()); + assertTrue(service.getScreenerCheckById("L-public-0.9.0").isPresent()); + assertTrue(service.getScreenerCheckById("L-internal-sctf-age-requirement-99.0.0").isEmpty()); + } + @Test void loadMetadata_acceptsLegacyCheckArray() throws Exception { LibraryApiService service = new LibraryApiService(); diff --git a/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java new file mode 100644 index 00000000..c1c189e3 --- /dev/null +++ b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java @@ -0,0 +1,297 @@ +package org.codeforphilly.bdt.builder.service; + +import com.fasterxml.jackson.databind.ObjectMapper; +import jakarta.ws.rs.BadRequestException; +import jakarta.ws.rs.ForbiddenException; +import org.codeforphilly.bdt.builder.model.domain.*; +import org.codeforphilly.bdt.builder.model.dto.screener.ScreenerTransfer; +import org.codeforphilly.bdt.builder.persistence.*; +import org.codeforphilly.bdt.builder.persistence.impl.EligibilityCheckRepositoryImpl; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; + +import java.util.*; +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +class ScreenerTransferServiceTest { + private final ScreenerRepository screeners = mock(ScreenerRepository.class); + // Use the real ID helpers so round trips test the repository's versioned ID format. + private final EligibilityCheckRepository checks = mock(EligibilityCheckRepositoryImpl.class, CALLS_REAL_METHODS); + private final StorageService storage = mock(StorageService.class); + private final LibraryApiService library = mock(LibraryApiService.class); + private final ScreenerTransferService service = new ScreenerTransferService(screeners, checks, storage, library); + private final ObjectMapper mapper = new ObjectMapper(); + private Screener source; + private List benefits; + private EligibilityCheck draft; + + @BeforeEach + void setup() throws Exception { + doReturn(List.of()).when(checks).getAllWorkingCustomChecks(anyString()); + doNothing().when(checks).reserveCheckName(anyString(), anyString(), anyString(), anyString()); + doNothing().when(checks).releaseCheckName(anyString(), anyString(), anyString(), anyString()); + doAnswer(invocation -> invocation.getArgument(0).getId()).when(checks).saveNewWorkingCustomCheck(any()); + doAnswer(invocation -> invocation.getArgument(0).getId()).when(checks).saveNewPublishedCustomCheck(any()); + doNothing().when(checks).deleteWorkingCustomCheck(anyString()); + doNothing().when(checks).deletePublishedCustomCheck(anyString()); + when(storage.getCheckDmnModelPath(anyString())).thenAnswer(i -> "check/" + i.getArgument(0) + ".dmn"); + when(storage.getScreenerWorkingFormSchemaPath(anyString())).thenAnswer(i -> "form/working/" + i.getArgument(0) + ".json"); + when(screeners.saveNewWorkingScreener(any())).thenAnswer(i -> i.getArgument(0).getId()); + source = Screener.create("sender", "Housing screener", null); + source.setId("source-screener"); + source.setPublishedScreenerId("public-link"); + source.setLastPublishDate("yesterday"); + source.setFormSchema(Map.of("type", "default", "components", List.of(Map.of("key", "income", "type", "number")))); + draft = check("W-family", "3.0.0"); + EligibilityCheck v1 = check("P-family-1.0.0", "1.0.0"); + EligibilityCheck v2 = check("P-family-2.0.0", "2.0.0"); + doReturn(Optional.of(draft)).when(checks).getWorkingCustomCheck("sender", "W-family", true); + doReturn(Optional.of(v1)).when(checks).getPublishedCustomCheck("sender", v1.getId(), true); + doReturn(Optional.of(v2)).when(checks).getPublishedCustomCheck("sender", v2.getId(), true); + CheckConfig c1 = config(v1.getId(), "1.0.0"); + c1.setAliasName("Income under $50,000"); + c1.setAliasGenerated(true); + c1.setParameters(Map.of("limit", 50000)); + CheckConfig c2 = config(v2.getId(), "2.0.0"); + CheckConfig lc = config("L-library", "1.0.0"); + lc.setEvaluationUrl("http://sender/library"); + EligibilityCheck libraryCheck = new EligibilityCheck(); + libraryCheck.setEvaluationUrl("http://recipient/library"); + when(library.getScreenerCheckById("L-library")).thenReturn(Optional.of(libraryCheck)); + Benefit first = new Benefit("benefit-a", "First", "A description", "sender", List.of(c1, lc)); + Benefit second = new Benefit("benefit-b", "Second", "Another description", "sender", List.of(c2)); + benefits = List.of(second, first); // Repository collection order differs from screener order. + source.setBenefits(List.of(new BenefitDetail("benefit-a", "First", "A description"), + new BenefitDetail("benefit-b", "Second", "Another description"))); + when(screeners.getWorkingScreenerMetaDataOnly(source.getId())).thenReturn(Optional.of(source)); + when(screeners.getWorkingScreener(source.getId())).thenReturn(Optional.of(source)); + when(screeners.getBenefitsInScreener(source)).thenReturn(benefits); + } + + @Test + void roundTripPreservesRulesVersionsFormAndSettingsWithNewOwnershipAndIds() throws Exception { + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + String json = mapper.writeValueAsString(export); + assertFalse(json.contains("ownerId\":\"sender")); + assertFalse(json.contains("public-link")); + assertEquals(3, export.customChecks().size()); + // Exercise the exact single-file representation, including embedded XML. + Screener imported = service.importScreener("recipient", mapper.readValue(json, ScreenerTransfer.class)); + assertNotEquals(source.getId(), imported.getId()); + assertEquals("recipient", imported.getOwnerId()); + assertEquals(source.getScreenerName(), imported.getScreenerName()); + assertNull(imported.getPublishedScreenerId()); + assertNull(imported.getLastPublishDate()); + assertEquals(List.of("First", "Second"), imported.getBenefits().stream().map(BenefitDetail::getName).toList()); + ArgumentCaptor savedBenefits = ArgumentCaptor.forClass(Benefit.class); + verify(screeners, times(2)).saveNewCustomBenefit(eq(imported.getId()), savedBenefits.capture()); + Benefit first = savedBenefits.getAllValues().getFirst(); + assertNotEquals("benefit-a", first.getId()); + assertEquals("recipient", first.getOwnerId()); + CheckConfig configured = first.getChecks().getFirst(); + assertEquals(Map.of("limit", 50000), configured.getParameters()); + assertEquals("Income under $50,000", configured.getAliasName()); + assertTrue(configured.isAliasGenerated()); + assertEquals("1.0.0", configured.getCheckVersion()); + assertNotEquals("P-family-1.0.0", configured.getSourceCheckId()); + assertNull(configured.getEvaluationUrl()); + assertEquals("L-library", first.getChecks().get(1).getSourceCheckId()); + assertEquals("http://recipient/library", first.getChecks().get(1).getEvaluationUrl()); + ArgumentCaptor savedDraft = ArgumentCaptor.forClass(EligibilityCheck.class); + verify(checks).saveNewWorkingCustomCheck(savedDraft.capture()); + EligibilityCheck importedDraft = savedDraft.getValue(); + assertEquals("3.0.0", importedDraft.getVersion()); + assertEquals("recipient", importedDraft.getOwnerId()); + assertFalse(importedDraft.getIsArchived()); + assertNull(importedDraft.getExampleSourceId()); + ArgumentCaptor savedPublished = ArgumentCaptor.forClass(EligibilityCheck.class); + verify(checks, times(2)).saveNewPublishedCustomCheck(savedPublished.capture()); + for (EligibilityCheck published : savedPublished.getAllValues()) { + assertEquals(importedDraft.getId(), checks.getWorkingId(published)); + verify(storage).writeStringToStorage("check/" + published.getId() + ".dmn", check(published.getId(), published.getVersion()).getDmnModel(), "application/xml"); + } + verify(storage).writeStringToStorage("form/working/" + imported.getId() + ".json", + mapper.writeValueAsString(source.getFormSchema()), "application/json"); + assertEquals("sender", draft.getOwnerId()); + assertEquals("benefit-a", source.getBenefits().getFirst().getId()); + } + + @Test + void importingTwiceDoesNotReuseOrOverwriteCustomChecks() throws Exception { + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + doReturn(List.of(draft)).when(checks).getAllWorkingCustomChecks("recipient"); + Screener one = service.importScreener("recipient", export); + Screener two = service.importScreener("recipient", export); + assertNotEquals(one.getId(), two.getId()); + ArgumentCaptor saved = ArgumentCaptor.forClass(EligibilityCheck.class); + verify(checks, times(2)).saveNewWorkingCustomCheck(saved.capture()); + assertNotEquals(saved.getAllValues().get(0).getId(), saved.getAllValues().get(1).getId()); + assertEquals("income (imported 2)", saved.getValue().getModule()); + assertEquals("Income", saved.getValue().getName()); + assertEquals("income", draft.getModule()); + } + + @Test + void missingModelIsRejectedBeforeAnyWrites() throws Exception { + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + export.customChecks().getFirst().setDmnModel(null); + assertThrows(BadRequestException.class, () -> service.importScreener("recipient", export)); + verify(checks, never()).reserveCheckName(anyString(), anyString(), anyString(), anyString()); + verify(screeners, never()).saveNewWorkingScreener(any()); + verify(storage, never()).writeStringToStorage(anyString(), anyString(), anyString()); + } + + @Test + void rejectsUnsupportedFormatAndUnresolvedReferences() throws Exception { + assertThrows(BadRequestException.class, () -> service.importScreener("recipient", + new ScreenerTransfer(ScreenerTransfer.FORMAT, 99, "Name", null, List.of(), List.of()))); + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + export.benefits().getFirst().getChecks().getFirst().setSourceCheckId("P-missing-1.0.0"); + assertThrows(BadRequestException.class, () -> service.importScreener("recipient", export)); + verify(screeners, never()).saveNewWorkingScreener(any()); + } + + @Test + void refusesExportsForOtherUsersBeforeReadingArtifacts() throws Exception { + assertThrows(ForbiddenException.class, () -> service.exportScreener("recipient", source.getId())); + verify(screeners, never()).getWorkingScreener(anyString()); + verify(screeners, never()).getBenefitsInScreener(any()); + } + + @Test + void refusesCustomCheckBelongingToSomeoneElse() throws Exception { + EligibilityCheck foreign = check("P-family-1.0.0", "1.0.0"); + foreign.setOwnerId("someone-else"); + doReturn(Optional.of(foreign)).when(checks).getPublishedCustomCheck("sender", foreign.getId(), true); + assertThrows(ForbiddenException.class, () -> service.exportScreener("sender", source.getId())); + } + + @Test + void failedFormWriteRollsBackBenefitsChecksModelsAndNameReservations() throws Exception { + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + doThrow(new Exception("Storage unavailable")).when(storage) + .writeStringToStorage(startsWith("form/"), anyString(), eq("application/json")); + assertThrows(Exception.class, () -> service.importScreener("recipient", export)); + verify(screeners, times(2)).deleteCustomBenefit(anyString(), anyString()); + verify(checks).deleteWorkingCustomCheck(anyString()); + verify(checks, times(2)).deletePublishedCustomCheck(anyString()); + verify(checks).releaseCheckName(eq("recipient"), eq("income"), eq("Income"), anyString()); + verify(storage, times(4)).deleteFile(anyString()); + verify(screeners, never()).saveNewWorkingScreener(any()); + } + + @Test + void emptyScreenerCanBeSharedWithoutAFormOrCustomChecks() throws Exception { + Screener imported = service.importScreener("recipient", new ScreenerTransfer(ScreenerTransfer.FORMAT, + ScreenerTransfer.VERSION, "Empty", null, List.of(), List.of())); + assertEquals(List.of(), imported.getBenefits()); + verify(screeners).saveNewWorkingScreener(imported); + verify(storage, never()).writeStringToStorage(anyString(), anyString(), anyString()); + } + + @Test + void archivedChecksAlreadyUsedByAScreenerCanBeShared() throws Exception { + draft.setIsArchived(true); + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + assertTrue(export.customChecks().stream().anyMatch(EligibilityCheck::getIsArchived)); + service.importScreener("recipient", export); + ArgumentCaptor saved = ArgumentCaptor.forClass(EligibilityCheck.class); + verify(checks).saveNewWorkingCustomCheck(saved.capture()); + assertFalse(saved.getValue().getIsArchived()); + } + + @Test + void publishedModelCanSupplyAnEditableDraftWhenTheOriginalDraftIsMissing() throws Exception { + doReturn(Optional.empty()).when(checks).getWorkingCustomCheck("sender", "W-family", true); + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + assertEquals(2, export.customChecks().size()); + service.importScreener("recipient", export); + ArgumentCaptor saved = ArgumentCaptor.forClass(EligibilityCheck.class); + verify(checks).saveNewWorkingCustomCheck(saved.capture()); + assertEquals("2.0.0", saved.getValue().getVersion()); + } + + @Test + void unavailableLibraryCheckRejectsImportBeforeWrites() throws Exception { + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + when(library.getScreenerCheckById("L-library")).thenReturn(Optional.empty()); + assertThrows(BadRequestException.class, () -> service.importScreener("recipient", export)); + verify(checks, never()).reserveCheckName(anyString(), anyString(), anyString(), anyString()); + verify(storage, never()).writeStringToStorage(anyString(), anyString(), anyString()); + } + + @Test + void bundledExampleCanBeExportedAndImportedWithItsInternalLibraryCheck() throws Exception { + var manifest = readResource("seed-data/example-screener/manifest.json", com.fasterxml.jackson.databind.JsonNode.class); + var screenerFiles = manifest.path("screeners").get(0); + Screener example = readResource(screenerFiles.path("screenerPath").asText(), Screener.class); + example.setOwnerId("sender"); + example.setFormSchema(mapper.convertValue(readResource(screenerFiles.path("formSchema").asText(), + com.fasterxml.jackson.databind.JsonNode.class), Map.class)); + List exampleBenefits = new ArrayList<>(); + for (var path : screenerFiles.path("benefits")) { + exampleBenefits.add(readResource(path.asText(), Benefit.class)); + } + when(screeners.getWorkingScreenerMetaDataOnly(example.getId())).thenReturn(Optional.of(example)); + when(screeners.getWorkingScreener(example.getId())).thenReturn(Optional.of(example)); + when(screeners.getBenefitsInScreener(example)).thenReturn(exampleBenefits); + for (String group : List.of("workingCustomChecks", "publishedCustomChecks")) { + for (var path : manifest.path(group)) { + EligibilityCheck model = readResource(path.asText(), EligibilityCheck.class); + model.setOwnerId("sender"); + try (var xml = getClass().getClassLoader().getResourceAsStream( + "seed-data/example-screener/storage/check/" + model.getId() + ".dmn")) { + model.setDmnModel(new String(xml.readAllBytes(), java.nio.charset.StandardCharsets.UTF_8)); + } + if (model.getId().startsWith("W-")) { + doReturn(Optional.of(model)).when(checks).getWorkingCustomCheck("sender", model.getId(), true); + } else { + doReturn(Optional.of(model)).when(checks).getPublishedCustomCheck("sender", model.getId(), true); + } + } + } + LibraryApiService actualLibrary = new LibraryApiService(); + actualLibrary.loadBenefitsMetadata(mapper.writeValueAsString(exampleBenefits)); + String internalId = "L-internal-sctf-age-requirement-0.9.0"; + assertTrue(actualLibrary.getById(internalId).isEmpty()); + ScreenerTransferService transferService = new ScreenerTransferService(screeners, checks, storage, actualLibrary); + var exported = transferService.exportScreener("sender", example.getId()); + var imported = transferService.importScreener("recipient", + mapper.readValue(mapper.writeValueAsString(exported), ScreenerTransfer.class)); + assertEquals(4, imported.getBenefits().size()); + var saved = ArgumentCaptor.forClass(Benefit.class); + verify(screeners, times(4)).saveNewCustomBenefit(eq(imported.getId()), saved.capture()); + var internal = saved.getAllValues().stream().flatMap(benefit -> benefit.getChecks().stream()) + .filter(config -> internalId.equals(config.getSourceCheckId())).findFirst().orElseThrow(); + assertEquals("/api/v1/checks/internal/sctf-age-requirement", internal.getEvaluationUrl()); + } + + private T readResource(String path, Class type) throws Exception { + try (var stream = getClass().getClassLoader().getResourceAsStream(path)) { + return mapper.readValue(stream, type); + } + } + + private EligibilityCheck check(String id, String version) { + EligibilityCheck check = new EligibilityCheck("Income", "income", "Description", List.of(), "sender"); + check.setId(id); + check.setVersion(version); + check.setExampleSourceId("example"); + check.setDmnModel("" + + "true"); + return check; + } + + private CheckConfig config(String source, String version) { + CheckConfig config = new CheckConfig(); + config.setCheckId(UUID.randomUUID().toString()); + config.setSourceCheckId(source); + config.setCheckName("Income"); + config.setCheckModule("income"); + config.setCheckVersion(version); + return config; + } +} diff --git a/builder-frontend/README.md b/builder-frontend/README.md index 4e4ff227..0c23bdaf 100644 --- a/builder-frontend/README.md +++ b/builder-frontend/README.md @@ -49,3 +49,11 @@ The authenticated editor uses three consistent navigation levels: The account menu contains User Guide, Logout, and the development-only example export. Published `/screener/:publishedScreenerId` pages remain outside the editor navigation. + +## Screener sharing + +The Screeners page offers **Import screener**. Each screener card's menu offers +**Export screener**, which downloads the saved draft as a single `.bdt.json` file. +Import creates a new unpublished screener and independent custom checks for the +signed-in user. Files contain a `bdt-screener` format marker and `formatVersion: 1`. +Library check references are resolved against the receiving server's library. diff --git a/builder-frontend/src/api/screenerTransfer.test.ts b/builder-frontend/src/api/screenerTransfer.test.ts new file mode 100644 index 00000000..50c65f1f --- /dev/null +++ b/builder-frontend/src/api/screenerTransfer.test.ts @@ -0,0 +1,81 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("@/api/auth", () => ({ authGet: vi.fn(), authPost: vi.fn() })); +import { authGet, authPost } from "@/api/auth"; +import { + exportScreener, + importScreener, + MAX_SCREENER_FILE_BYTES, +} from "./screenerTransfer"; + +const file = (content: string) => + ({ size: content.length, text: async () => content }) as File; + +describe("screener transfer API", () => { + beforeEach(() => vi.clearAllMocks()); + + it("downloads the complete export through the authenticated endpoint", async () => { + const json = JSON.stringify({ format: "bdt-screener", formatVersion: 1 }); + vi.mocked(authGet).mockResolvedValue(new Response(json)); + expect(await (await exportScreener("screener-1")).text()).toBe(json); + expect(authGet).toHaveBeenCalledWith( + expect.stringContaining("/screener/screener-1/export"), + ); + }); + + it("uploads the parsed file and returns the new screener ID", async () => { + const data = { + format: "bdt-screener", + formatVersion: 1, + benefits: [], + customChecks: [], + }; + vi.mocked(authPost).mockResolvedValue( + new Response(JSON.stringify({ id: "new-screener" }), { status: 201 }), + ); + await expect(importScreener(file(JSON.stringify(data)))).resolves.toEqual({ + id: "new-screener", + }); + expect(authPost).toHaveBeenCalledWith( + expect.stringContaining("/screener/import"), + data, + ); + }); + + it("rejects oversized files before reading or uploading them", async () => { + const oversized = { + size: MAX_SCREENER_FILE_BYTES + 1, + text: vi.fn(), + } as unknown as File; + await expect(importScreener(oversized)).rejects.toThrow("10 MB"); + expect(oversized.text).not.toHaveBeenCalled(); + expect(authPost).not.toHaveBeenCalled(); + }); + + it("reports invalid JSON without uploading it", async () => { + await expect(importScreener(file("not JSON"))).rejects.toThrow( + "not valid JSON", + ); + expect(authPost).not.toHaveBeenCalled(); + }); + + it("shows the server's validation error", async () => { + vi.mocked(authPost).mockResolvedValue( + new Response(JSON.stringify({ error: "Missing custom check" }), { + status: 400, + }), + ); + await expect(importScreener(file("{}"))).rejects.toThrow( + "Missing custom check", + ); + }); + + it("reports non-JSON export failures", async () => { + vi.mocked(authGet).mockResolvedValue( + new Response("Forbidden", { status: 403 }), + ); + await expect(exportScreener("screener-1")).rejects.toThrow( + "Could not export", + ); + }); +}); diff --git a/builder-frontend/src/api/screenerTransfer.ts b/builder-frontend/src/api/screenerTransfer.ts new file mode 100644 index 00000000..819afa70 --- /dev/null +++ b/builder-frontend/src/api/screenerTransfer.ts @@ -0,0 +1,60 @@ +import { authGet, authPost } from "@/api/auth"; +import { env } from "@/config/environment"; + +export const MAX_SCREENER_FILE_BYTES = 10 * 1024 * 1024; + +async function errorMessage(response: Response, fallback: string) { + try { + const body = await response.json(); + if (typeof body.error === "string") return body.error; + } catch { + // Keep a readable fallback for non-JSON responses. + } + return fallback; +} + +export async function exportScreener(screenerId: string): Promise { + const response = await authGet( + `${env.apiUrl}/screener/${encodeURIComponent(screenerId)}/export`, + ); + if (!response.ok) { + throw new Error( + await errorMessage(response, "Could not export the screener."), + ); + } + return response.blob(); +} + +export async function importScreener(file: File): Promise<{ id: string }> { + if (file.size > MAX_SCREENER_FILE_BYTES) { + throw new Error("Choose a screener file smaller than 10 MB."); + } + let data: unknown; + try { + data = JSON.parse(await file.text()); + } catch { + throw new Error( + "This file is not valid JSON. Choose a BDT screener export.", + ); + } + const response = await authPost(`${env.apiUrl}/screener/import`, data); + if (!response.ok) { + throw new Error( + await errorMessage(response, "Could not import the screener."), + ); + } + return response.json(); +} + +export async function downloadScreener(screenerId: string, name: string) { + const blob = await exportScreener(screenerId); + const url = URL.createObjectURL(blob); + const link = document.createElement("a"); + link.href = url; + link.download = `${name.replace(/[^a-zA-Z0-9_-]+/g, "-") || "screener"}.bdt.json`; + document.body.appendChild(link); + link.click(); + link.remove(); + // Allow the browser to start the download before releasing the object URL. + setTimeout(() => URL.revokeObjectURL(url), 1000); +} diff --git a/builder-frontend/src/components/homeScreen/EditScreenerForm.tsx b/builder-frontend/src/components/homeScreen/EditScreenerForm.tsx index c904b49a..1d1197de 100644 --- a/builder-frontend/src/components/homeScreen/EditScreenerForm.tsx +++ b/builder-frontend/src/components/homeScreen/EditScreenerForm.tsx @@ -1,4 +1,5 @@ -import { createSignal, JSX } from "solid-js"; +import { createSignal, JSX, Show } from "solid-js"; +import { downloadScreener } from "@/api/screenerTransfer"; import TrashIcon from "../icon/TrashIcon"; import DeleteConfirmation from "./DeleteConfirmation"; import { Modal } from "@/components/shared/Modal"; @@ -23,6 +24,8 @@ export default function EditScreenerForm(props: Props) { const [isLoading, setIsLoading] = createSignal(false); const [isConfirmationVisible, setIsConfirmationVisible] = createSignal(false); const [error, setError] = createSignal(""); + const [exporting, setExporting] = createSignal(false); + const [exportError, setExportError] = createSignal(""); const handleSubmit: JSX.EventHandler = async ( e, @@ -76,6 +79,42 @@ export default function EditScreenerForm(props: Props) {
+

+ Download this screener’s saved benefits, checks, and form to share an + editable copy. +

+ + + + + +
+ + + {file()?.name ?? "No file selected"} + + + + + + + + + ); +} diff --git a/builder-frontend/src/components/homeScreen/ScreenersList.tsx b/builder-frontend/src/components/homeScreen/ScreenersList.tsx index 839c1a50..020410be 100644 --- a/builder-frontend/src/components/homeScreen/ScreenersList.tsx +++ b/builder-frontend/src/components/homeScreen/ScreenersList.tsx @@ -10,6 +10,8 @@ import { useNavigate } from "@solidjs/router"; import EditScreenerForm, { EditModalData } from "./EditScreenerForm"; import NewScreenerForm from "./NewScreenerForm"; +import ImportScreenerForm from "./ImportScreenerForm"; +import { Button } from "@/components/shared/Button"; import MenuIcon from "../icon/MenuIcon"; import { @@ -28,6 +30,7 @@ export default function ScreenersList() { const [isNewScreenerModalVisible, setIsNewScreenerModalVisible] = createSignal(false); const [isEditModalVisible, setIsEditModalVisible] = createSignal(false); + const [isImportModalVisible, setIsImportModalVisible] = createSignal(false); const [editModelData, setEditModalData] = createSignal( null, ); @@ -117,6 +120,19 @@ export default function ScreenersList() { > Create new screener + + setIsImportModalVisible(false)} + > + + setIsNewScreenerModalVisible(false)} diff --git a/docs/src/content/docs/user/user-guide.md b/docs/src/content/docs/user/user-guide.md index b8b6b240..7fb2bd11 100644 --- a/docs/src/content/docs/user/user-guide.md +++ b/docs/src/content/docs/user/user-guide.md @@ -20,6 +20,7 @@ From here, you can: - View all of your existing screeners - Open and edit a screener - Create a new screener +- Import a screener shared by another analyst **Creating a new screener**: @@ -27,6 +28,14 @@ Select **Create new screener** and provide a name for your screener. The name sh After the screener is created, you are automatically taken to the **Screener Dashboard**. +### Sharing a screener + +Save your changes, then return to **Screeners**, open the menu on the screener's card, and select **Export screener**. BDT downloads one `.bdt.json` file containing the saved benefits, configured check versions, parameters, aliases, custom-check rules and drafts, and form. Share this file with the recipient. + +To open a shared screener, select **Import screener** on the **Screeners** page, choose the JSON file (up to 10 MB), and select **Import screener** in the dialog. BDT opens a new editable draft owned by your account. You can preview, edit, and publish it with its own public URL. + +Each import creates independent copies of the custom checks. If a custom check's name and module already exist in your account, the copy goes into a module with an `imported` suffix. The screener keeps the check versions used by the sender. Library checks use your BDT server's library and must be available there; a file with missing checks is rejected before anything is imported. + --- ## 2. The Screener Dashboard diff --git a/e2e/tests/exampleScreenerTransfer.spec.ts b/e2e/tests/exampleScreenerTransfer.spec.ts new file mode 100644 index 00000000..5fb1c4eb --- /dev/null +++ b/e2e/tests/exampleScreenerTransfer.spec.ts @@ -0,0 +1,108 @@ +import { expect, test } from "@playwright/test"; +import { randomUUID } from "node:crypto"; + +// Provision our own local demo account instead of clearing an existing developer's data. +// This also exercises the exact bundled example received by a new analyst. +test("the bundled example can be chosen, exported, and imported with its internal library checks", async ({ + page, + request, +}) => { + const email = `import-regression-${randomUUID()}@example.com`; + const password = "local-import-regression-account"; + const signUp = await request.post( + "http://localhost:9099/identitytoolkit.googleapis.com/v1/accounts:signUp?key=local-demo", + { + data: { email, password, returnSecureToken: true }, + }, + ); + expect(signUp.ok()).toBe(true); + const account = await signUp.json(); + const provision = await request.post( + "http://localhost:8081/api/account/hooks", + { + headers: { Authorization: `Bearer ${account.idToken}` }, + data: { hooks: ["add example screener"] }, + }, + ); + expect(provision.ok()).toBe(true); + expect((await provision.json()).success).toBe(true); + + await page.goto("/login"); + await page.locator("#email").fill(email); + await page.locator("#password").fill(password); + await page.getByRole("button", { name: "Sign In", exact: true }).click(); + await expect(page).toHaveURL("/screeners"); + const card = page + .locator("article") + .filter({ hasText: "Philadelphia Benefits Example" }); + await expect(card).toHaveCount(1); + await card.getByRole("button").click(); + const downloadPromise = page.waitForEvent("download"); + await page + .getByRole("button", { name: "Export screener", exact: true }) + .click(); + const download = await downloadPromise; + const stream = await download.createReadStream(); + const chunks: Buffer[] = []; + for await (const chunk of stream!) chunks.push(Buffer.from(chunk)); + const buffer = Buffer.concat(chunks); + const exported = JSON.parse(buffer.toString()); + expect(exported.benefits).toHaveLength(4); + expect(exported.benefits.flatMap((benefit: any) => benefit.checks)).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + sourceCheckId: "L-internal-sctf-age-requirement-0.9.0", + }), + ]), + ); + + await page.goto("/screeners"); + await page + .getByRole("button", { name: "Import screener", exact: true }) + .click(); + const chooseFile = page.getByRole("button", { + name: "Choose file", + exact: true, + }); + await expect(chooseFile).toBeVisible(); + expect( + await chooseFile.evaluate((element) => + Number.parseFloat(getComputedStyle(element).borderTopWidth), + ), + ).toBeGreaterThan(0); + const chooserPromise = page.waitForEvent("filechooser"); + await chooseFile.click(); + const chooser = await chooserPromise; + await chooser.setFiles({ + name: "example.bdt.json", + mimeType: "application/json", + buffer, + }); + await expect( + page.getByText("example.bdt.json", { exact: true }), + ).toBeVisible(); + const importPromise = page.waitForResponse( + (response) => + response.url().endsWith("/api/screener/import") && + response.request().method() === "POST", + ); + await page + .locator("form") + .getByRole("button", { name: "Import screener", exact: true }) + .click(); + const response = await importPromise; + expect(response.status(), await response.text()).toBe(201); + const imported = await response.json(); + expect(imported.benefits).toHaveLength(4); + await expect(page).toHaveURL(`/screeners/${imported.id}`); + await expect(page.locator("#manage-benefits-title")).toBeVisible(); + await expect( + page.getByText("Senior Citizen Tax Freeze (SCTF)", { exact: true }), + ).toBeVisible(); + expect(imported.benefits.map((benefit: any) => benefit.name)).toEqual( + exported.benefits.map((benefit: any) => benefit.name), + ); + for (const benefit of imported.benefits) { + await expect(page.getByText(benefit.name, { exact: true })).toBeVisible(); + } +}); diff --git a/e2e/tests/screenerTransfer.spec.ts b/e2e/tests/screenerTransfer.spec.ts new file mode 100644 index 00000000..b1628384 --- /dev/null +++ b/e2e/tests/screenerTransfer.spec.ts @@ -0,0 +1,171 @@ +import { expect, Page, test } from "@playwright/test"; +import { readFile } from "node:fs/promises"; +import { authLogin } from "./authLogin"; +import { + resetEmulator, + seedCustomCheckWithParameter, + seedScreenerWithForm, + TEST_BENEFIT_ID, + TEST_SCREENER_ID, +} from "./firestore"; +import { getCollection, patchDocument } from "./firestore/firestoreClient"; +import { uploadObject } from "./firestore/storageClient"; + +async function downloadScreener(page: Page): Promise { + await page.goto("/screeners"); + const card = page + .locator("article") + .filter({ hasText: "Shared screener" }) + .first(); + await card.getByRole("button").click(); + const downloadPromise = page.waitForEvent("download"); + await page + .getByRole("button", { name: "Export screener", exact: true }) + .click(); + const download = await downloadPromise; + expect(download.suggestedFilename()).toBe("Shared-screener.bdt.json"); + const stream = await download.createReadStream(); + const chunks: Buffer[] = []; + for await (const chunk of stream!) chunks.push(Buffer.from(chunk)); + return Buffer.concat(chunks); +} + +test.describe("Screener sharing", () => { + test.beforeEach(resetEmulator); + test.beforeEach(authLogin); + test.afterEach(resetEmulator); + + test("exports and imports a complete screener with independent custom checks", async ({ + page, + }) => { + await seedScreenerWithForm("Shared screener"); + const { workingCheckId, publishedCheckId } = + await seedCustomCheckWithParameter(); + const model = ( + await readFile( + "../builder-api/src/main/resources/seed-data/example-screener/storage/check/P-5bdf1ac7-b0ca-403b-89f9-7f1504f6bf26-1.0.0.dmn", + "utf8", + ) + ).replaceAll("Household income limit", "income_threshold"); + await uploadObject(`check/${workingCheckId}.dmn`, model); + await uploadObject(`check/${publishedCheckId}.dmn`, model); + + // Keep the library check and add a parameterized custom check to the same benefit. + const benefitCollection = (await getCollection( + `workingScreener/${TEST_SCREENER_ID}/customBenefit`, + )) as any; + const benefit = benefitCollection.documents[0]; + const checkCollection = (await getCollection( + "publishedCustomCheck", + )) as any; + const fields = checkCollection.documents[0].fields; + benefit.fields.checks.arrayValue.values.push({ + mapValue: { + fields: { + checkId: { stringValue: "configured-income" }, + sourceCheckId: { stringValue: publishedCheckId }, + checkName: fields.name, + checkModule: fields.module, + checkVersion: fields.version, + inputDefinition: fields.inputDefinition, + parameterDefinitions: fields.parameterDefinitions, + parameters: { + mapValue: { fields: { incomeLimit: { integerValue: "50000" } } }, + }, + aliasName: { stringValue: "Income under $50,000" }, + aliasGenerated: { booleanValue: false }, + }, + }, + }); + await patchDocument( + `workingScreener/${TEST_SCREENER_ID}/customBenefit`, + TEST_BENEFIT_ID, + benefit, + ); + await uploadObject( + `form/working/${TEST_SCREENER_ID}.json`, + JSON.stringify({ + schemaVersion: 18, + type: "default", + id: "BDT_Form", + components: [ + { + id: "owner", + key: "simpleChecks.ownerOccupant", + label: "Is owner occupant?", + type: "checkbox", + }, + { + id: "income", + key: "custom.householdIncome", + label: "Household income", + type: "number", + }, + ], + }), + ); + + const buffer = await downloadScreener(page); + const exported = JSON.parse(buffer.toString()); + expect(exported.customChecks).toHaveLength(2); + expect(exported.benefits[0].checks[1].parameters).toEqual({ + incomeLimit: 50000, + }); + expect(exported.benefits[0].checks[1].aliasName).toBe( + "Income under $50,000", + ); + expect(exported.customChecks[0].dmnModel).toBe(model); + + await page.goto("/screeners"); + await page + .getByRole("button", { name: "Import screener", exact: true }) + .click(); + await page + .locator('input[type="file"]') + .setInputFiles({ + name: "shared.bdt.json", + mimeType: "application/json", + buffer, + }); + const responsePromise = page.waitForResponse( + (response) => + response.url().endsWith("/api/screener/import") && + response.request().method() === "POST", + ); + await page + .locator("form") + .getByRole("button", { name: "Import screener", exact: true }) + .click(); + const response = await responsePromise; + expect(response.status()).toBe(201); + const imported = await response.json(); + expect(imported.id).not.toBe(TEST_SCREENER_ID); + expect(imported.publishedScreenerId).toBeNull(); + await expect(page).toHaveURL(`/screeners/${imported.id}`); + await expect(page.locator("#manage-benefits-title")).toBeVisible(); + await expect(page.getByText("Test Benefit", { exact: true })).toBeVisible(); + + // Evaluate both checks using the imported form and DMN artifact. + await page.getByTestId("editor-section-preview").click(); + await page.getByRole("button", { name: "Show all questions" }).click(); + await page.locator('[type="checkbox"].fjs-input').check(); + await page.getByLabel("Household income").fill("40000"); + await expect(page.locator("#benefit-result-title_0")).toContainText( + "Test Benefit: Eligible", + { timeout: 15000 }, + ); + await page.getByLabel("Household income").fill("60000"); + await expect(page.locator("#benefit-result-title_0")).toContainText( + "Test Benefit: Ineligible", + { timeout: 15000 }, + ); + + // The two working copies must remain distinct, even when importing into the sender's account. + const workingChecks = (await getCollection("workingCustomCheck")) as any; + expect(workingChecks.documents).toHaveLength(2); + const copy = workingChecks.documents.find( + (document: any) => document.fields.id.stringValue !== workingCheckId, + ); + expect(copy.fields.module.stringValue).toBe("e2e (imported 2)"); + }); +}); From e55e2476d4750fc2e136b8ba2e462ce5ba54ee35 Mon Sep 17 00:00:00 2001 From: Preston Cabe Date: Mon, 5 Oct 2026 20:52:32 -0400 Subject: [PATCH 02/14] Require unique screener names per user --- .../builder/controller/ScreenerResource.java | 9 +++ .../controller/ScreenerTransferResource.java | 3 + .../DuplicateScreenerNameException.java | 7 ++ .../builder/persistence/FirestoreUtils.java | 40 ++++++++++ .../impl/ScreenerRepositoryImpl.java | 22 ++++-- .../service/ScreenerTransferService.java | 7 +- .../ScreenerTransferResourceTest.java | 10 +++ .../service/ScreenerTransferServiceTest.java | 13 ++++ builder-frontend/README.md | 2 + builder-frontend/src/api/screener.ts | 11 ++- .../src/api/screenerTransfer.test.ts | 23 +++++- builder-frontend/src/api/screenerTransfer.ts | 23 +++++- .../homeScreen/EditScreenerForm.tsx | 5 +- .../homeScreen/ImportScreenerForm.test.tsx | 56 +++++++++++-- .../homeScreen/ImportScreenerForm.tsx | 78 +++++++++++++++++-- .../components/homeScreen/NewScreenerForm.tsx | 9 ++- .../components/homeScreen/ScreenersList.tsx | 16 ++-- docs/src/content/docs/user/user-guide.md | 2 +- e2e/tests/exampleScreenerTransfer.spec.ts | 48 ++++++++++++ e2e/tests/screenerTransfer.spec.ts | 13 ++-- 20 files changed, 349 insertions(+), 48 deletions(-) create mode 100644 builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/DuplicateScreenerNameException.java diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerResource.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerResource.java index a243e120..10c39179 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerResource.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerResource.java @@ -28,6 +28,7 @@ import java.util.Comparator; import java.util.List; import java.util.Map; +import org.codeforphilly.bdt.builder.persistence.DuplicateScreenerNameException; import java.util.Optional; import org.codeforphilly.bdt.builder.model.dto.screener.CreateScreenerRequest; import org.codeforphilly.bdt.builder.model.dto.screener.EditScreenerRequest; @@ -109,6 +110,10 @@ public Response postScreener( String screenerId = screenerRepository.saveNewWorkingScreener(newScreener); newScreener.setId(screenerId); return Response.ok(newScreener, MediaType.APPLICATION_JSON).build(); + } catch (DuplicateScreenerNameException duplicate) { + return Response.status(409).entity(Map.of("error", duplicate.getMessage())).build(); + } catch (jakarta.ws.rs.BadRequestException invalid) { + return Response.status(400).entity(Map.of("error", invalid.getMessage())).build(); } catch (Exception e) { return Response.status(Response.Status.INTERNAL_SERVER_ERROR) .entity(Map.of("error", "Could not save Screener")) @@ -146,6 +151,10 @@ public Response updateScreener( try { screenerRepository.updateWorkingScreener(screener); return Response.ok(screener, MediaType.APPLICATION_JSON).build(); + } catch (DuplicateScreenerNameException duplicate) { + return Response.status(409).entity(Map.of("error", duplicate.getMessage())).build(); + } catch (jakarta.ws.rs.BadRequestException invalid) { + return Response.status(400).entity(Map.of("error", invalid.getMessage())).build(); } catch (Exception e) { return Response.status(Response.Status.INTERNAL_SERVER_ERROR) .entity(Map.of("error", "Could not update Screener")) diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResource.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResource.java index aac765f7..40124e3f 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResource.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResource.java @@ -9,6 +9,7 @@ import org.codeforphilly.bdt.builder.model.dto.screener.ScreenerTransfer; import org.codeforphilly.bdt.builder.service.ScreenerTransferService; import java.util.Map; +import org.codeforphilly.bdt.builder.persistence.DuplicateScreenerNameException; @Path("/api") @Produces(MediaType.APPLICATION_JSON) @@ -38,6 +39,8 @@ public Response importScreener(@Context SecurityIdentity identity, ScreenerTrans if (owner == null) return Response.status(Response.Status.UNAUTHORIZED).build(); try { return Response.status(Response.Status.CREATED).entity(transferService.importScreener(owner, transfer)).build(); + } catch (DuplicateScreenerNameException duplicate) { + return Response.status(409).entity(Map.of("error", duplicate.getMessage())).build(); } catch (BadRequestException rejected) { return Response.status(Response.Status.BAD_REQUEST).entity(Map.of("error", rejected.getMessage())).build(); } catch (Exception failure) { diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/DuplicateScreenerNameException.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/DuplicateScreenerNameException.java new file mode 100644 index 00000000..30cd5a44 --- /dev/null +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/DuplicateScreenerNameException.java @@ -0,0 +1,7 @@ +package org.codeforphilly.bdt.builder.persistence; + +public class DuplicateScreenerNameException extends RuntimeException { + public DuplicateScreenerNameException() { + super("You already have a screener with this name. Choose a different name."); + } +} diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/FirestoreUtils.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/FirestoreUtils.java index 0de861fd..3d3a491a 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/FirestoreUtils.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/FirestoreUtils.java @@ -238,6 +238,46 @@ public static void createDocumentUnlessHeld(String collection, String documentId } } + /** Serializes name changes per owner and includes existing screeners without a name index. */ + public static void saveScreenerWithUniqueName(String collection, String id, Map data, + boolean create) throws Exception { + String owner = (String) data.get("ownerId"); + String name = (String) data.get("screenerName"); + DocumentReference ownerLock = db.collection("screenerNameLocks").document(owner); + DocumentReference document = db.collection(collection).document(id); + try { + db.runTransaction(transaction -> { + transaction.get(ownerLock).get(); + QuerySnapshot screeners = transaction.get(db.collection(collection) + .whereEqualTo("ownerId", owner)).get(); + for (QueryDocumentSnapshot existing : screeners.getDocuments()) { + if (!id.equals(existing.getId()) && normalizeScreenerName(name) + .equals(normalizeScreenerName(existing.getString("screenerName")))) { + throw new DuplicateScreenerNameException(); + } + } + if (create) transaction.create(document, data); + else transaction.set(document, data, SetOptions.merge()); + transaction.set(ownerLock, Map.of("lastChange", UUID.randomUUID().toString())); + return null; + }).get(); + } catch (InterruptedException failure) { + Thread.currentThread().interrupt(); + throw failure; + } catch (ExecutionException failure) { + Throwable cause = failure; + while (cause != null) { + if (cause instanceof DuplicateScreenerNameException duplicate) throw duplicate; + cause = cause.getCause(); + } + throw failure; + } + } + + public static String normalizeScreenerName(String name) { + return name == null ? "" : name.strip().toLowerCase(Locale.ROOT); + } + public static void updateDocument(String collectionName, Map data, String docId) throws Exception { try { WriteResult result = db.collection(collectionName) diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/ScreenerRepositoryImpl.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/ScreenerRepositoryImpl.java index e08ed07e..82a6a09e 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/ScreenerRepositoryImpl.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/ScreenerRepositoryImpl.java @@ -86,10 +86,13 @@ public String saveNewWorkingScreener(Screener screener) throws Exception{ ObjectMapper mapper = new ObjectMapper().setSerializationInclusion(JsonInclude.Include.NON_NULL); Map data = mapper.convertValue(screener, Map.class); data.remove("formSchema"); - if (screener.getId() != null) { - return FirestoreUtils.persistDocumentWithId(CollectionNames.WORKING_SCREENER_COLLECTION, screener.getId(), data); - } - return FirestoreUtils.persistDocument(CollectionNames.WORKING_SCREENER_COLLECTION, data); + if (screener.getId() == null) screener.setId(java.util.UUID.randomUUID().toString()); + validateName(screener); + data.put("screenerName", screener.getScreenerName()); + data.put("id", screener.getId()); + FirestoreUtils.saveScreenerWithUniqueName(CollectionNames.WORKING_SCREENER_COLLECTION, + screener.getId(), data, true); + return screener.getId(); } @Override @@ -102,7 +105,16 @@ public void updateWorkingScreener(Screener screener) throws Exception { // It is saved separately on in cloud storage. data.remove("formSchema"); - FirestoreUtils.updateDocument(CollectionNames.WORKING_SCREENER_COLLECTION, data, screener.getId()); + validateName(screener); + data.put("screenerName", screener.getScreenerName()); + FirestoreUtils.saveScreenerWithUniqueName(CollectionNames.WORKING_SCREENER_COLLECTION, + screener.getId(), data, false); + } + + private void validateName(Screener screener) { + if (screener.getScreenerName() == null || screener.getScreenerName().isBlank()) + throw new jakarta.ws.rs.BadRequestException("Please enter a screener name."); + screener.setScreenerName(screener.getScreenerName().strip()); } public void addBenefitDetailToWorkingScreener(String screenerId, BenefitDetail benefitDetail) throws Exception { diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java index a2b86bdb..7e5208ce 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java @@ -98,6 +98,11 @@ private EligibilityCheck portableCheck(EligibilityCheck original) { public Screener importScreener(String owner, ScreenerTransfer transfer) throws Exception { // Validate the complete file and resolve library references before any writes. Map originals = validate(transfer); + String name = transfer.screenerName().strip(); + if (screeners.getWorkingScreeners(owner).stream().anyMatch(existing -> + name.equalsIgnoreCase(existing.getScreenerName() == null ? "" : existing.getScreenerName().strip()))) { + throw new DuplicateScreenerNameException(); + } Map libraryChecks = new HashMap<>(); for (Benefit benefit : transfer.benefits()) { for (CheckConfig config : configs(benefit)) { @@ -174,7 +179,7 @@ public Screener importScreener(String owner, ScreenerTransfer transfer) throws E benefits.add(benefit); details.add(new BenefitDetail(benefit.getId(), benefit.getName(), benefit.getDescription())); } - Screener screener = Screener.create(owner, transfer.screenerName(), null); + Screener screener = Screener.create(owner, name, null); // Make the draft visible only once all its artifacts have been saved. screener.setId(UUID.randomUUID().toString()); String id = screener.getId(); diff --git a/builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResourceTest.java b/builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResourceTest.java index edcd3843..49220673 100644 --- a/builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResourceTest.java +++ b/builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResourceTest.java @@ -45,6 +45,16 @@ void invalidFileReturnsAReadableValidationError() throws Exception { } } + @Test + void duplicateNameReturnsConflictWithAnActionableMessage() throws Exception { + when(service.importScreener("recipient", null)) + .thenThrow(new org.codeforphilly.bdt.builder.persistence.DuplicateScreenerNameException()); + try (var response = resource.importScreener(identity, null)) { + assertEquals(409, response.getStatus()); + assertTrue(response.getEntity().toString().contains("Choose a different name")); + } + } + @Test void forbiddenExportReturnsForbidden() throws Exception { when(service.exportScreener("recipient", "other-screener")).thenThrow(new ForbiddenException()); diff --git a/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java index c1c189e3..2303e63a 100644 --- a/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java +++ b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java @@ -134,6 +134,19 @@ void importingTwiceDoesNotReuseOrOverwriteCustomChecks() throws Exception { assertEquals("income", draft.getModule()); } + @Test + void duplicateNameRejectsImportBeforeWritingArtifacts() throws Exception { + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + when(screeners.getWorkingScreeners("recipient")) + .thenReturn(List.of(Screener.create("recipient", " housing SCREENER ", null))); + assertThrows(DuplicateScreenerNameException.class, () -> service.importScreener("recipient", export)); + verify(checks, never()).reserveCheckName(anyString(), anyString(), anyString(), anyString()); + verify(storage, never()).writeStringToStorage(anyString(), anyString(), anyString()); + var renamed = new ScreenerTransfer(export.format(), export.formatVersion(), " Housing screener - Copy ", + export.formSchema(), export.benefits(), export.customChecks()); + assertEquals("Housing screener - Copy", service.importScreener("recipient", renamed).getScreenerName()); + } + @Test void missingModelIsRejectedBeforeAnyWrites() throws Exception { ScreenerTransfer export = service.exportScreener("sender", source.getId()); diff --git a/builder-frontend/README.md b/builder-frontend/README.md index 0c23bdaf..ca6cad39 100644 --- a/builder-frontend/README.md +++ b/builder-frontend/README.md @@ -54,6 +54,8 @@ navigation. The Screeners page offers **Import screener**. Each screener card's menu offers **Export screener**, which downloads the saved draft as a single `.bdt.json` file. +The import dialog prefills the exported screener name and requires a different name if it already exists in the account. Creation, rename, and import enforce per-user name uniqueness, ignoring case and surrounding spaces. + Import creates a new unpublished screener and independent custom checks for the signed-in user. Files contain a `bdt-screener` format marker and `formatVersion: 1`. Library check references are resolved against the receiving server's library. diff --git a/builder-frontend/src/api/screener.ts b/builder-frontend/src/api/screener.ts index 4c6b5e56..c8fd459a 100644 --- a/builder-frontend/src/api/screener.ts +++ b/builder-frontend/src/api/screener.ts @@ -8,6 +8,8 @@ import type { ScreenerResult, } from "@/types"; +import { errorMessage } from "./screenerTransfer"; + const apiUrl = env.apiUrl; export const fetchScreeners = async () => { @@ -51,7 +53,9 @@ export const createNewScreener = async (request: { const response = await authPost(url.toString(), request); if (!response.ok) { - throw new Error(`Post failed with status: ${response.status}`); + throw new Error( + await errorMessage(response, "Could not create the screener."), + ); } const data = await response.json(); return data; @@ -71,8 +75,9 @@ export const updateScreener = async ( const response = await authPatch(url.toString(), request); if (!response.ok) { - const err = await response.json(); - throw new Error(err); + throw new Error( + await errorMessage(response, "Could not update the screener."), + ); } } catch (error) { console.error("Error updating screener:", error); diff --git a/builder-frontend/src/api/screenerTransfer.test.ts b/builder-frontend/src/api/screenerTransfer.test.ts index 50c65f1f..738060bb 100644 --- a/builder-frontend/src/api/screenerTransfer.test.ts +++ b/builder-frontend/src/api/screenerTransfer.test.ts @@ -27,6 +27,7 @@ describe("screener transfer API", () => { const data = { format: "bdt-screener", formatVersion: 1, + screenerName: "Example", benefits: [], customChecks: [], }; @@ -65,9 +66,9 @@ describe("screener transfer API", () => { status: 400, }), ); - await expect(importScreener(file("{}"))).rejects.toThrow( - "Missing custom check", - ); + await expect( + importScreener(file('{"screenerName":"Example"}')), + ).rejects.toThrow("Missing custom check"); }); it("reports non-JSON export failures", async () => { @@ -79,3 +80,19 @@ describe("screener transfer API", () => { ); }); }); + +it("uses the chosen name without changing the file", async () => { + vi.mocked(authPost).mockResolvedValue( + new Response('{"id":"copy"}', { status: 201 }), + ); + const original = file( + '{"screenerName":"Example","benefits":[],"customChecks":[]}', + ); + await importScreener(original, " Example - Copy "); + expect(authPost).toHaveBeenLastCalledWith(expect.any(String), { + screenerName: "Example - Copy", + benefits: [], + customChecks: [], + }); + expect(JSON.parse(await original.text()).screenerName).toBe("Example"); +}); diff --git a/builder-frontend/src/api/screenerTransfer.ts b/builder-frontend/src/api/screenerTransfer.ts index 819afa70..e188f033 100644 --- a/builder-frontend/src/api/screenerTransfer.ts +++ b/builder-frontend/src/api/screenerTransfer.ts @@ -3,7 +3,7 @@ import { env } from "@/config/environment"; export const MAX_SCREENER_FILE_BYTES = 10 * 1024 * 1024; -async function errorMessage(response: Response, fallback: string) { +export async function errorMessage(response: Response, fallback: string) { try { const body = await response.json(); if (typeof body.error === "string") return body.error; @@ -25,7 +25,9 @@ export async function exportScreener(screenerId: string): Promise { return response.blob(); } -export async function importScreener(file: File): Promise<{ id: string }> { +export async function readScreenerFile( + file: File, +): Promise> { if (file.size > MAX_SCREENER_FILE_BYTES) { throw new Error("Choose a screener file smaller than 10 MB."); } @@ -37,6 +39,23 @@ export async function importScreener(file: File): Promise<{ id: string }> { "This file is not valid JSON. Choose a BDT screener export.", ); } + if ( + !data || + typeof data !== "object" || + Array.isArray(data) || + typeof (data as Record).screenerName !== "string" + ) { + throw new Error("Choose a BDT screener export with a screener name."); + } + return data as Record; +} + +export async function importScreener( + file: File, + name?: string, +): Promise<{ id: string }> { + const data = await readScreenerFile(file); + if (name !== undefined) data.screenerName = name.trim(); const response = await authPost(`${env.apiUrl}/screener/import`, data); if (!response.ok) { throw new Error( diff --git a/builder-frontend/src/components/homeScreen/EditScreenerForm.tsx b/builder-frontend/src/components/homeScreen/EditScreenerForm.tsx index 1d1197de..09569855 100644 --- a/builder-frontend/src/components/homeScreen/EditScreenerForm.tsx +++ b/builder-frontend/src/components/homeScreen/EditScreenerForm.tsx @@ -34,7 +34,7 @@ export default function EditScreenerForm(props: Props) { setError(""); const form = new FormData(e.currentTarget); const screenerName = form.get("screenerName"); - if (!screenerName) { + if (!screenerName?.toString().trim()) { setError("Please enter a screener name."); } else { try { @@ -43,6 +43,9 @@ export default function EditScreenerForm(props: Props) { await props.handleEditScreener(props.modalData.screenerId, data); setIsLoading(false); } catch (e) { + setError( + e instanceof Error ? e.message : "Could not update the screener.", + ); if (isLoading()) { setIsLoading(false); } diff --git a/builder-frontend/src/components/homeScreen/ImportScreenerForm.test.tsx b/builder-frontend/src/components/homeScreen/ImportScreenerForm.test.tsx index 97ac2649..dc78cac5 100644 --- a/builder-frontend/src/components/homeScreen/ImportScreenerForm.test.tsx +++ b/builder-frontend/src/components/homeScreen/ImportScreenerForm.test.tsx @@ -2,18 +2,20 @@ import { render } from "solid-js/web"; import { afterEach, beforeEach, expect, it, vi } from "vitest"; -const { navigate, importScreener } = vi.hoisted(() => ({ +const { navigate, importScreener, readScreenerFile } = vi.hoisted(() => ({ navigate: vi.fn(), importScreener: vi.fn(), + readScreenerFile: vi.fn(), })); vi.mock("@solidjs/router", () => ({ useNavigate: () => navigate })); -vi.mock("@/api/screenerTransfer", () => ({ importScreener })); +vi.mock("@/api/screenerTransfer", () => ({ importScreener, readScreenerFile })); import ImportScreenerForm from "./ImportScreenerForm"; let dispose: () => void; let container: HTMLDivElement; beforeEach(() => { vi.clearAllMocks(); + readScreenerFile.mockResolvedValue({ screenerName: "Example" }); container = document.createElement("div"); document.body.appendChild(container); dispose = render(() => , container); @@ -24,7 +26,7 @@ afterEach(() => { vi.restoreAllMocks(); }); -function chooseFile() { +async function chooseFile() { const input = container.querySelector('input[type="file"]')!; const file = new File(["{}"], "example.bdt.json", { @@ -32,6 +34,12 @@ function chooseFile() { }); Object.defineProperty(input, "files", { value: [file] }); input.dispatchEvent(new Event("change", { bubbles: true })); + await vi.waitFor(() => + expect( + container.querySelector('input[name="screenerName"]') + ?.value, + ).toBe("Example"), + ); return file; } function submit() { @@ -45,17 +53,17 @@ it("requires a file and opens the imported screener", async () => { container.querySelector('button[type="submit"]')! .disabled, ).toBe(true); - const file = chooseFile(); + const file = await chooseFile(); importScreener.mockResolvedValue({ id: "new-screener" }); submit(); await vi.waitFor(() => expect(navigate).toHaveBeenCalledWith("/screeners/new-screener"), ); - expect(importScreener).toHaveBeenCalledWith(file); + expect(importScreener).toHaveBeenCalledWith(file, "Example"); }); it("shows a failure and allows retrying the same file", async () => { - chooseFile(); + await chooseFile(); importScreener.mockRejectedValue(new Error("Missing DMN model")); submit(); await vi.waitFor(() => @@ -75,7 +83,7 @@ it("shows a failure and allows retrying the same file", async () => { ); }); -it("opens the file picker from a visible button and displays the chosen filename", () => { +it("opens the file picker from a visible button and displays the chosen filename", async () => { const input = container.querySelector('input[type="file"]')!; const click = vi.spyOn(input, "click"); @@ -86,8 +94,40 @@ it("opens the file picker from a visible button and displays the chosen filename expect(button.disabled).toBe(false); button.click(); expect(click).toHaveBeenCalledOnce(); - const file = chooseFile(); + const file = await chooseFile(); expect(container.querySelector('[aria-live="polite"]')?.textContent).toBe( file.name, ); }); + +it("requires a different name on collision, treating case and spaces alike", async () => { + dispose(); + dispose = render( + () => , + container, + ); + await chooseFile(); + expect(container.querySelector('[role="alert"]')?.textContent).toContain( + "Choose a different name", + ); + expect( + container.querySelector('button[type="submit"]')! + .disabled, + ).toBe(true); + submit(); + expect(importScreener).not.toHaveBeenCalled(); + const input = container.querySelector( + 'input[name="screenerName"]', + )!; + input.value = "Example - Copy"; + input.dispatchEvent(new Event("input", { bubbles: true })); + importScreener.mockResolvedValue({ id: "copy" }); + submit(); + await vi.waitFor(() => + expect(navigate).toHaveBeenCalledWith("/screeners/copy"), + ); + expect(importScreener).toHaveBeenCalledWith( + expect.any(File), + "Example - Copy", + ); +}); diff --git a/builder-frontend/src/components/homeScreen/ImportScreenerForm.tsx b/builder-frontend/src/components/homeScreen/ImportScreenerForm.tsx index 7e328cb4..ed9415e1 100644 --- a/builder-frontend/src/components/homeScreen/ImportScreenerForm.tsx +++ b/builder-frontend/src/components/homeScreen/ImportScreenerForm.tsx @@ -1,24 +1,34 @@ import { createSignal, Show } from "solid-js"; import { useNavigate } from "@solidjs/router"; import { Button } from "@/components/shared/Button"; -import { importScreener } from "@/api/screenerTransfer"; +import { importScreener, readScreenerFile } from "@/api/screenerTransfer"; -export default function ImportScreenerForm() { +export default function ImportScreenerForm(props: { + existingNames?: string[]; +}) { const navigate = useNavigate(); const [file, setFile] = createSignal(); const [loading, setLoading] = createSignal(false); const [error, setError] = createSignal(""); + const [name, setName] = createSignal(""); + const [reading, setReading] = createSignal(false); + const duplicate = () => + props.existingNames?.some( + (existing) => + existing.trim().toLowerCase() === name().trim().toLowerCase(), + ); let fileInput!: HTMLInputElement; return (
{ event.preventDefault(); - if (!file() || loading()) return; + if (!file() || !name().trim() || reading() || duplicate() || loading()) + return; setError(""); setLoading(true); try { - const screener = await importScreener(file()!); + const screener = await importScreener(file()!, name()); navigate(`/screeners/${screener.id}`); } catch (failure) { setError( @@ -45,9 +55,29 @@ export default function ImportScreenerForm() { type="file" accept=".json,application/json" disabled={loading()} - onChange={(event) => { - setFile(event.currentTarget.files?.[0]); + onChange={async (event) => { + const selected = event.currentTarget.files?.[0]; + setFile(selected); + setName(""); setError(""); + if (!selected) { + setReading(false); + return; + } + setReading(true); + try { + const data = await readScreenerFile(selected); + if (file() === selected) setName(data.screenerName as string); + } catch (failure) { + if (file() === selected) + setError( + failure instanceof Error + ? failure.message + : "Could not read this file.", + ); + } finally { + if (file() === selected) setReading(false); + } }} />
@@ -63,12 +93,46 @@ export default function ImportScreenerForm() {
+ + + + + + -
diff --git a/builder-frontend/src/components/homeScreen/NewScreenerForm.tsx b/builder-frontend/src/components/homeScreen/NewScreenerForm.tsx index 13fd67f8..8125d188 100644 --- a/builder-frontend/src/components/homeScreen/NewScreenerForm.tsx +++ b/builder-frontend/src/components/homeScreen/NewScreenerForm.tsx @@ -2,7 +2,7 @@ import { createNewScreener } from "@/api/screener"; import { Button } from "@/components/shared/Button"; import Form from "@/components/shared/Form"; import { useNavigate } from "@solidjs/router"; -import { createSignal, JSX, onCleanup } from "solid-js"; +import { createSignal, JSX } from "solid-js"; export default function NewScreenerForm({}) { const navigate = useNavigate(); @@ -17,17 +17,20 @@ export default function NewScreenerForm({}) { setError(""); const form = new FormData(e.currentTarget); const screenerName = form.get("screenerName"); - if (screenerName) { + if (screenerName?.toString().trim()) { const data = { screenerName: screenerName.toString() }; try { const newScreener = await createNewScreener(data); navigate(`/screeners/${newScreener.id}`); } catch (e) { - console.log("Error creating screener", e); + setError( + e instanceof Error ? e.message : "Could not create the screener.", + ); } } else { setError("Please enter a screener name."); } + setIsLoading(false); }; return ( diff --git a/builder-frontend/src/components/homeScreen/ScreenersList.tsx b/builder-frontend/src/components/homeScreen/ScreenersList.tsx index 020410be..1bc83382 100644 --- a/builder-frontend/src/components/homeScreen/ScreenersList.tsx +++ b/builder-frontend/src/components/homeScreen/ScreenersList.tsx @@ -73,13 +73,9 @@ export default function ScreenersList() { screenerId: string, screenerData: { screenerName: string }, ) => { - try { - await updateScreener(screenerId, screenerData); - refetchScreenerList(); - setIsEditModalVisible(false); - } catch (e) { - console.log("Error editing screener", e); - } + await updateScreener(screenerId, screenerData); + refetchScreenerList(); + setIsEditModalVisible(false); }; const handleDeleteScreener = async (screenerData) => { @@ -131,7 +127,11 @@ export default function ScreenersList() { show={isImportModalVisible()} onClose={() => setIsImportModalVisible(false)} > - + screener.screenerName) ?? [] + } + /> response.url().endsWith("/api/screener/import") && @@ -94,6 +105,43 @@ test("the bundled example can be chosen, exported, and imported with its interna expect(response.status(), await response.text()).toBe(201); const imported = await response.json(); expect(imported.benefits).toHaveLength(4); + expect(imported.screenerName).toBe(`${exported.screenerName} - Copy`); + const headers = { Authorization: `Bearer ${account.idToken}` }; + const duplicateCreate = await request.post( + "http://localhost:8081/api/screener", + { + headers, + data: { screenerName: ` ${exported.screenerName.toUpperCase()} ` }, + }, + ); + expect(duplicateCreate.status(), await duplicateCreate.text()).toBe(409); + const duplicateRename = await request.patch( + `http://localhost:8081/api/screener/${imported.id}`, + { + headers, + data: { screenerName: exported.screenerName }, + }, + ); + expect(duplicateRename.status(), await duplicateRename.text()).toBe(409); + const unchangedRename = await request.patch( + `http://localhost:8081/api/screener/${imported.id}`, + { + headers, + data: { screenerName: imported.screenerName }, + }, + ); + expect(unchangedRename.status(), await unchangedRename.text()).toBe(200); + const competingCreates = await Promise.all( + ["Race name", " race NAME "].map((screenerName) => + request.post("http://localhost:8081/api/screener", { + headers, + data: { screenerName }, + }), + ), + ); + expect(competingCreates.map((result) => result.status()).sort()).toEqual([ + 200, 409, + ]); await expect(page).toHaveURL(`/screeners/${imported.id}`); await expect(page.locator("#manage-benefits-title")).toBeVisible(); await expect( diff --git a/e2e/tests/screenerTransfer.spec.ts b/e2e/tests/screenerTransfer.spec.ts index b1628384..222909a5 100644 --- a/e2e/tests/screenerTransfer.spec.ts +++ b/e2e/tests/screenerTransfer.spec.ts @@ -120,13 +120,14 @@ test.describe("Screener sharing", () => { await page .getByRole("button", { name: "Import screener", exact: true }) .click(); + await page.locator('input[type="file"]').setInputFiles({ + name: "shared.bdt.json", + mimeType: "application/json", + buffer, + }); await page - .locator('input[type="file"]') - .setInputFiles({ - name: "shared.bdt.json", - mimeType: "application/json", - buffer, - }); + .getByLabel("Screener name", { exact: true }) + .fill(`${exported.screenerName} - Copy`); const responsePromise = page.waitForResponse( (response) => response.url().endsWith("/api/screener/import") && From ce964b8cf070179ce6716de902e9819bf2c6009e Mon Sep 17 00:00:00 2001 From: Preston Cabe Date: Mon, 5 Oct 2026 21:01:40 -0400 Subject: [PATCH 03/14] Reuse imported custom checks by original identity and version --- .../builder/constants/CollectionNames.java | 1 + .../controller/ScreenerTransferResource.java | 3 + .../model/domain/EligibilityCheck.java | 6 + .../EligibilityCheckRepository.java | 8 + .../impl/EligibilityCheckRepositoryImpl.java | 39 +++ .../CustomCheckDmnRenameValidator.java | 22 ++ .../CustomCheckImportConflictException.java | 5 + .../service/ScreenerTransferService.java | 211 +++++++++++--- .../ScreenerTransferResourceTest.java | 10 + .../service/ScreenerTransferServiceTest.java | 268 +++++++++++++++++- builder-frontend/README.md | 8 +- docs/src/content/docs/user/user-guide.md | 2 +- e2e/tests/exampleScreenerTransfer.spec.ts | 162 ++++++++++- e2e/tests/screenerTransfer.spec.ts | 11 +- 14 files changed, 697 insertions(+), 59 deletions(-) create mode 100644 builder-api/src/main/java/org/codeforphilly/bdt/builder/service/CustomCheckImportConflictException.java diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/constants/CollectionNames.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/constants/CollectionNames.java index 079611ef..c9a4574d 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/constants/CollectionNames.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/constants/CollectionNames.java @@ -5,5 +5,6 @@ public class CollectionNames { public static final String PUBLISHED_SCREENER_COLLECTION = "publishedScreener"; public static final String WORKING_CUSTOM_CHECK_COLLECTION = "workingCustomCheck"; public static final String PUBLISHED_CUSTOM_CHECK_COLLECTION = "publishedCustomCheck"; + public static final String CUSTOM_CHECK_ORIGIN_COLLECTION = "customCheckOrigin"; public static final String CUSTOM_CHECK_NAME_COLLECTION = "customCheckName"; } diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResource.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResource.java index 40124e3f..1a43a827 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResource.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResource.java @@ -8,6 +8,7 @@ import org.codeforphilly.bdt.builder.auth.AuthUtils; import org.codeforphilly.bdt.builder.model.dto.screener.ScreenerTransfer; import org.codeforphilly.bdt.builder.service.ScreenerTransferService; +import org.codeforphilly.bdt.builder.service.CustomCheckImportConflictException; import java.util.Map; import org.codeforphilly.bdt.builder.persistence.DuplicateScreenerNameException; @@ -41,6 +42,8 @@ public Response importScreener(@Context SecurityIdentity identity, ScreenerTrans return Response.status(Response.Status.CREATED).entity(transferService.importScreener(owner, transfer)).build(); } catch (DuplicateScreenerNameException duplicate) { return Response.status(409).entity(Map.of("error", duplicate.getMessage())).build(); + } catch (CustomCheckImportConflictException conflict) { + return Response.status(409).entity(Map.of("error", conflict.getMessage())).build(); } catch (BadRequestException rejected) { return Response.status(Response.Status.BAD_REQUEST).entity(Map.of("error", rejected.getMessage())).build(); } catch (Exception failure) { diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/model/domain/EligibilityCheck.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/model/domain/EligibilityCheck.java index 35187377..b40f702f 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/model/domain/EligibilityCheck.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/model/domain/EligibilityCheck.java @@ -21,6 +21,8 @@ public class EligibilityCheck { private Boolean isArchived; // working id of the example check this was imported from, so re-imports survive renames private String exampleSourceId; + // Original family identity, preserved through screener exports and imports across accounts. + private String originCheckId; // milliseconds since the epoch at which a published version was saved; null for working checks private Long datePublished; @@ -138,6 +140,10 @@ public void setExampleSourceId(String exampleSourceId) { this.exampleSourceId = exampleSourceId; } + public String getOriginCheckId() { return originCheckId; } + + public void setOriginCheckId(String originCheckId) { this.originCheckId = originCheckId; } + public Long getDatePublished() { return datePublished; } diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/EligibilityCheckRepository.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/EligibilityCheckRepository.java index c9c11664..33c3bd6a 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/EligibilityCheckRepository.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/EligibilityCheckRepository.java @@ -8,6 +8,14 @@ public interface EligibilityCheckRepository { + /* Strict metadata read, including archived drafts and all published versions. */ + List getCustomChecksForImport(String userId) throws Exception; + + /* Prevents concurrent imports from creating separate families for the same origin. */ + void reserveImportIdentity(String ownerId, String originId, String workingId) throws Exception; + + void releaseImportIdentity(String ownerId, String originId, String workingId) throws Exception; + List getWorkingCustomChecks(String userId); List getAllWorkingCustomChecks(String userId); diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/EligibilityCheckRepositoryImpl.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/EligibilityCheckRepositoryImpl.java index fdb01f9e..24a963f3 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/EligibilityCheckRepositoryImpl.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/EligibilityCheckRepositoryImpl.java @@ -36,6 +36,45 @@ public class EligibilityCheckRepositoryImpl implements EligibilityCheckRepositor @Inject private StorageService storageService; + @Override + public List getCustomChecksForImport(String userId) throws Exception { + ObjectMapper mapper = new ObjectMapper(); + List all = new ArrayList<>(); + for (String collection : List.of(CollectionNames.WORKING_CUSTOM_CHECK_COLLECTION, + CollectionNames.PUBLISHED_CUSTOM_CHECK_COLLECTION)) { + for (Map data : FirestoreUtils.getFirestoreDocsByFields(collection, + Map.of(FieldNames.OWNER_ID, userId))) { + all.add(mapper.convertValue(data, EligibilityCheck.class)); + } + } + return all; + } + + @Override + public void reserveImportIdentity(String ownerId, String originId, String workingId) throws Exception { + long now = System.currentTimeMillis(); + FirestoreUtils.createDocumentUnlessHeld(CollectionNames.CUSTOM_CHECK_ORIGIN_COLLECTION, + importIdentityId(ownerId, originId), + Map.of("ownerId", ownerId, "originCheckId", originId, "checkId", workingId, "reservedAt", now), + (existing, reader) -> existing.get("reservedAt") instanceof Number reservedAt + && now - reservedAt.longValue() >= ABANDONED_RESERVATION_MILLIS + && existing.get("checkId") instanceof String holder + && reader.get(CollectionNames.WORKING_CUSTOM_CHECK_COLLECTION, holder).isEmpty()); + } + + @Override + public void releaseImportIdentity(String ownerId, String originId, String workingId) throws Exception { + String id = importIdentityId(ownerId, originId); + var reservation = FirestoreUtils.getFirestoreDocById(CollectionNames.CUSTOM_CHECK_ORIGIN_COLLECTION, id); + if (reservation.isPresent() && workingId.equals(reservation.get().get("checkId"))) + FirestoreUtils.deleteDocument(CollectionNames.CUSTOM_CHECK_ORIGIN_COLLECTION, id); + } + + private String importIdentityId(String ownerId, String originId) { + return URLEncoder.encode(ownerId, StandardCharsets.UTF_8) + ":" + + URLEncoder.encode(originId, StandardCharsets.UTF_8); + } + public List getWorkingCustomChecks(String userId){ return getAllWorkingCustomChecks(userId).stream() .filter(check -> !check.getIsArchived()) diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/CustomCheckDmnRenameValidator.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/CustomCheckDmnRenameValidator.java index 36f650e4..8dbacbc9 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/CustomCheckDmnRenameValidator.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/CustomCheckDmnRenameValidator.java @@ -27,6 +27,28 @@ public void validate(String originalXml, String renamedXml, String oldName, Stri } } + public boolean equivalentModels(String left, String right) throws Exception { + Element original = parse(left).getDocumentElement(); + Element imported = parse(right).getDocumentElement(); + removeIndentation(original); + removeIndentation(imported); + return original.isEqualNode(imported); + } + + private void removeIndentation(org.w3c.dom.Node node) { + boolean hasElements = false; + for (var child = node.getFirstChild(); child != null; child = child.getNextSibling()) { + if (child.getNodeType() == org.w3c.dom.Node.ELEMENT_NODE) hasElements = true; + } + for (var child = node.getFirstChild(); child != null;) { + var next = child.getNextSibling(); + if (hasElements && child.getNodeType() == org.w3c.dom.Node.TEXT_NODE && child.getNodeValue().isBlank()) + node.removeChild(child); + else removeIndentation(child); + child = next; + } + } + private Document parse(String xml) throws Exception { if (xml.startsWith("\"")) { xml = new com.fasterxml.jackson.databind.ObjectMapper().readValue(xml, String.class); diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/CustomCheckImportConflictException.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/CustomCheckImportConflictException.java new file mode 100644 index 00000000..e44ff6e0 --- /dev/null +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/CustomCheckImportConflictException.java @@ -0,0 +1,5 @@ +package org.codeforphilly.bdt.builder.service; + +public class CustomCheckImportConflictException extends RuntimeException { + public CustomCheckImportConflictException(String message) { super(message); } +} diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java index 7e5208ce..d403ca17 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java @@ -88,6 +88,7 @@ private void requireOwnedCheck(String owner, EligibilityCheck check) { private EligibilityCheck portableCheck(EligibilityCheck original) { EligibilityCheck check = copy(original, EligibilityCheck.class); if (blank(check.getDmnModel())) throw new IllegalStateException("Missing custom check DMN model"); + check.setOriginCheckId(originId(original)); check.setOwnerId(null); check.setExampleSourceId(null); check.setDatePublished(null); @@ -113,48 +114,39 @@ public Screener importScreener(String owner, ScreenerTransfer transfer) throws E } } } - Map newWorkingIds = new LinkedHashMap<>(); - Map workingCopies = new LinkedHashMap<>(); - for (EligibilityCheck original : originals.values()) { - String family = checks.getWorkingId(original); - newWorkingIds.computeIfAbsent(family, ignored -> checks.newWorkingId()); - EligibilityCheck candidate = workingCopies.get(family); - if (candidate == null || original.getId().startsWith("W-") - || (!candidate.getId().startsWith("W-") - && CheckVersion.compare(original.getVersion(), candidate.getVersion()) > 0)) { - workingCopies.put(family, copy(original, EligibilityCheck.class)); - } - } + // Decide every reuse/conflict before reserving names or writing any artifact. + List existing = checks.getCustomChecksForImport(owner); + CheckImportPlan plan = planChecks(owner, originals, existing, transfer); Deque cleanup = new ArrayDeque<>(); try { - // A collision gets a separate module, keeping decision names and DMN XML intact. - List existing = checks.getAllWorkingCustomChecks(owner); - for (var entry : workingCopies.entrySet()) { - EligibilityCheck working = entry.getValue(); - working.setId(newWorkingIds.get(entry.getKey())); - resetCheckOwnership(working, owner); - reserveImportedName(working, existing); + List occupiedNames = new ArrayList<>(existing.stream() + .filter(check -> owner.equals(check.getOwnerId()) && check.getId().startsWith("W-")).toList()); + for (FamilyImport family : plan.families().values()) { + if (!family.createWorking()) continue; + EligibilityCheck working = family.working(); + if (family.newFamily()) { + try { + checks.reserveImportIdentity(owner, working.getOriginCheckId(), working.getId()); + } catch (DocumentAlreadyExistsException concurrentImport) { + throw new CustomCheckImportConflictException("Another import is creating check " + + working.getName() + ". Please retry the import."); + } + cleanup.push(() -> checks.releaseImportIdentity(owner, working.getOriginCheckId(), working.getId())); + } + reserveImportedName(working, occupiedNames); cleanup.push(() -> checks.releaseCheckName(owner, working.getModule(), working.getName(), working.getId())); - existing = new ArrayList<>(existing); - existing.add(working); - } - Map remappedIds = new HashMap<>(); - for (var entry : workingCopies.entrySet()) { - EligibilityCheck working = entry.getValue(); - remappedIds.put(entry.getKey(), working.getId()); - cleanup.push(() -> checks.deleteWorkingCustomCheck(working.getId())); + occupiedNames.add(working); saveCheck(working, false, cleanup); } - for (EligibilityCheck original : originals.values()) { - if (!original.getId().startsWith("P-")) continue; - String family = checks.getWorkingId(original); - EligibilityCheck published = copy(original, EligibilityCheck.class); - resetCheckOwnership(published, owner); - published.setModule(workingCopies.get(family).getModule()); - published.setId(newWorkingIds.get(family)); - published.setId(checks.getPublishedId(published, published.getVersion())); - remappedIds.put(original.getId(), published.getId()); - cleanup.push(() -> checks.deletePublishedCustomCheck(published.getId())); + for (EligibilityCheck published : plan.publishedToCreate()) { + // A new family's module may have changed while reserving its name. + String workingId = checks.getWorkingId(published); + for (FamilyImport family : plan.families().values()) { + if (family.working().getId().equals(workingId)) { + published.setModule(family.working().getModule()); + break; + } + } saveCheck(published, true, cleanup); } List benefits = new ArrayList<>(); @@ -171,9 +163,10 @@ public Screener importScreener(String owner, ScreenerTransfer transfer) throws E config.setSourceCheckId(source); config.setEvaluationUrl(libraryChecks.get(source).getEvaluationUrl()); } else { - config.setSourceCheckId(remappedIds.get(source)); + EligibilityCheck target = plan.destinations().get(source); + config.setSourceCheckId(target.getId()); config.setEvaluationUrl(null); - config.setCheckModule(workingCopies.get(checks.getWorkingId(originals.get(source))).getModule()); + config.setCheckModule(target.getModule()); } } benefits.add(benefit); @@ -205,16 +198,139 @@ public Screener importScreener(String owner, ScreenerTransfer transfer) throws E } } + private String originId(EligibilityCheck check) { + if (!blank(check.getOriginCheckId())) return check.getOriginCheckId(); + if (!blank(check.getExampleSourceId())) return check.getExampleSourceId(); + return checks.getWorkingId(check); + } + + private CheckImportPlan planChecks(String owner, Map originals, + List existing, ScreenerTransfer transfer) throws Exception { + Map draftSources = new LinkedHashMap<>(); + Set referencedWorking = new HashSet<>(); + for (Benefit benefit : transfer.benefits()) { + for (CheckConfig config : configs(benefit)) { + if (sourceId(config).startsWith("W-")) referencedWorking.add(sourceId(config)); + } + } + for (EligibilityCheck original : originals.values()) { + String family = checks.getWorkingId(original); + EligibilityCheck candidate = draftSources.get(family); + if (candidate == null || original.getId().startsWith("W-") + || (!candidate.getId().startsWith("W-") + && CheckVersion.compare(original.getVersion(), candidate.getVersion()) > 0)) { + draftSources.put(family, original); + } + } + Map families = new LinkedHashMap<>(); + Map destinations = new HashMap<>(); + List publishedToCreate = new ArrayList<>(); + for (var entry : draftSources.entrySet()) { + String sourceFamily = entry.getKey(); + String origin = originId(entry.getValue()); + List matches = existing.stream() + .filter(check -> owner.equals(check.getOwnerId())) + .filter(check -> origin.equals(originId(check)) || origin.equals(checks.getWorkingId(check)) + || sourceFamily.equals(checks.getWorkingId(check))) + .toList(); + Set matchingFamilies = new HashSet<>(); + for (EligibilityCheck match : matches) matchingFamilies.add(checks.getWorkingId(match)); + if (matchingFamilies.size() > 1) + throw new CustomCheckImportConflictException("Several checks share the original identity of " + + entry.getValue().getName() + ". Resolve those duplicates before importing."); + boolean newFamily = matchingFamilies.isEmpty(); + String workingId = newFamily ? checks.newWorkingId() : matchingFamilies.iterator().next(); + EligibilityCheck working = matches.stream().filter(check -> workingId.equals(check.getId())) + .findFirst().orElse(null); + boolean createWorking = working == null; + if (createWorking) { + working = copy(entry.getValue(), EligibilityCheck.class); + working.setId(workingId); + working.setOriginCheckId(origin); + resetCheckOwnership(working, owner); + } else if (referencedWorking.contains(sourceFamily)) { + working = loadCheck(owner, workingId); + requireSameContent(entry.getValue(), working, true); + } + families.put(sourceFamily, new FamilyImport(working, createWorking, newFamily)); + destinations.put(sourceFamily, working); + } + for (EligibilityCheck original : originals.values()) { + if (!original.getId().startsWith("P-")) continue; + FamilyImport family = families.get(checks.getWorkingId(original)); + String id = checks.getPublishedId(family.working(), original.getVersion()); + EligibilityCheck metadata = existing.stream().filter(check -> id.equals(check.getId())).findFirst().orElse(null); + EligibilityCheck target; + if (metadata != null) { + requireOwnedCheck(owner, metadata); + target = loadCheck(owner, id); + requireSameContent(original, target, false); + } else { + target = copy(original, EligibilityCheck.class); + target.setId(id); + target.setOriginCheckId(originId(original)); + target.setModule(family.working().getModule()); + resetCheckOwnership(target, owner); + publishedToCreate.add(target); + } + destinations.put(original.getId(), target); + } + return new CheckImportPlan(families, destinations, publishedToCreate); + } + + private void requireSameContent(EligibilityCheck incoming, EligibilityCheck existing, boolean working) { + boolean same = false; + try { + same = Objects.equals(incoming.getName(), existing.getName()) + && Objects.equals(incoming.getVersion(), existing.getVersion()) + && Objects.equals(normalizeInputDefinition(incoming), normalizeInputDefinition(existing)) + && mapper.valueToTree(incoming.getParameterDefinitions() == null ? List.of() : incoming.getParameterDefinitions()) + .equals(mapper.valueToTree(existing.getParameterDefinitions() == null ? List.of() : existing.getParameterDefinitions())) + && !blank(existing.getDmnModel()) + && new CustomCheckDmnRenameValidator().equivalentModels(incoming.getDmnModel(), existing.getDmnModel()); + } catch (Exception invalidModel) { + // An unreadable existing model cannot safely be reused. + } + if (!same) throw new CustomCheckImportConflictException(working + ? "The imported screener uses a draft of check " + incoming.getName() + + " that differs from your draft. Publish and use a check version before exporting." + : "Check " + incoming.getName() + " version " + incoming.getVersion() + + " already exists with different rules or parameter definitions. The screener was not imported."); + } + + private com.fasterxml.jackson.databind.JsonNode normalizeInputDefinition(EligibilityCheck check) { + var definition = check.getInputDefinition(); + return definition == null || definition.isNull() ? null : definition; + } + private void saveCheck(EligibilityCheck check, boolean published, Deque cleanup) throws Exception { String dmn = check.getDmnModel(); - check.setDmnModel(null); + EligibilityCheck metadata = copy(check, EligibilityCheck.class); + metadata.setDmnModel(null); + // Claim the document before touching its XML. A losing concurrent create never overwrites it. + try { + if (published) checks.saveNewPublishedCustomCheck(metadata); + else checks.saveNewWorkingCustomCheck(metadata); + } catch (DocumentAlreadyExistsException concurrentCreate) { + if (!published) throw new CustomCheckImportConflictException("Another import created this check. Please retry."); + EligibilityCheck winner = loadCheck(check.getOwnerId(), check.getId()); + requireSameContent(check, winner, false); + check.setModule(winner.getModule()); + return; + } + cleanup.push(() -> { + if (published) checks.deletePublishedCustomCheck(check.getId()); + else checks.deleteWorkingCustomCheck(check.getId()); + }); String path = storage.getCheckDmnModelPath(check.getId()); cleanup.push(() -> storage.deleteFile(path)); storage.writeStringToStorage(path, dmn, "application/xml"); - if (published) checks.saveNewPublishedCustomCheck(check); - else checks.saveNewWorkingCustomCheck(check); } + private record FamilyImport(EligibilityCheck working, boolean createWorking, boolean newFamily) {} + private record CheckImportPlan(Map families, Map destinations, + List publishedToCreate) {} + private void resetCheckOwnership(EligibilityCheck check, String owner) { check.setOwnerId(owner); check.setIsArchived(false); @@ -263,6 +379,17 @@ private Map validate(ScreenerTransfer transfer) { if (byId.putIfAbsent(check.getId(), check) != null) throw new BadRequestException("Duplicate custom check ID."); } + Map familyOrigins = new HashMap<>(); + Map originFamilies = new HashMap<>(); + for (EligibilityCheck check : byId.values()) { + String family = checks.getWorkingId(check); + String origin = originId(check); + String priorOrigin = familyOrigins.putIfAbsent(family, origin); + String priorFamily = originFamilies.putIfAbsent(origin, family); + if ((priorOrigin != null && !priorOrigin.equals(origin)) + || (priorFamily != null && !priorFamily.equals(family))) + throw new BadRequestException("Custom check family identities are inconsistent."); + } Set benefitIds = new HashSet<>(); Set referencedFamilies = new HashSet<>(); Set referencedIds = new HashSet<>(); diff --git a/builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResourceTest.java b/builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResourceTest.java index 49220673..c89c9fd3 100644 --- a/builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResourceTest.java +++ b/builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResourceTest.java @@ -55,6 +55,16 @@ void duplicateNameReturnsConflictWithAnActionableMessage() throws Exception { } } + @Test + void conflictingCheckReturnsAReadableConflict() throws Exception { + when(service.importScreener("recipient", null)).thenThrow( + new org.codeforphilly.bdt.builder.service.CustomCheckImportConflictException("Version rules differ")); + try (var response = resource.importScreener(identity, null)) { + assertEquals(409, response.getStatus()); + assertEquals(Map.of("error", "Version rules differ"), response.getEntity()); + } + } + @Test void forbiddenExportReturnsForbidden() throws Exception { when(service.exportScreener("recipient", "other-screener")).thenThrow(new ForbiddenException()); diff --git a/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java index 2303e63a..5783c317 100644 --- a/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java +++ b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java @@ -31,6 +31,9 @@ class ScreenerTransferServiceTest { @BeforeEach void setup() throws Exception { doReturn(List.of()).when(checks).getAllWorkingCustomChecks(anyString()); + doReturn(List.of()).when(checks).getCustomChecksForImport(anyString()); + doNothing().when(checks).reserveImportIdentity(anyString(), anyString(), anyString()); + doNothing().when(checks).releaseImportIdentity(anyString(), anyString(), anyString()); doNothing().when(checks).reserveCheckName(anyString(), anyString(), anyString(), anyString()); doNothing().when(checks).releaseCheckName(anyString(), anyString(), anyString(), anyString()); doAnswer(invocation -> invocation.getArgument(0).getId()).when(checks).saveNewWorkingCustomCheck(any()); @@ -107,6 +110,7 @@ void roundTripPreservesRulesVersionsFormAndSettingsWithNewOwnershipAndIds() thro assertEquals("recipient", importedDraft.getOwnerId()); assertFalse(importedDraft.getIsArchived()); assertNull(importedDraft.getExampleSourceId()); + assertEquals("example", importedDraft.getOriginCheckId()); ArgumentCaptor savedPublished = ArgumentCaptor.forClass(EligibilityCheck.class); verify(checks, times(2)).saveNewPublishedCustomCheck(savedPublished.capture()); for (EligibilityCheck published : savedPublished.getAllValues()) { @@ -120,18 +124,266 @@ void roundTripPreservesRulesVersionsFormAndSettingsWithNewOwnershipAndIds() thro } @Test - void importingTwiceDoesNotReuseOrOverwriteCustomChecks() throws Exception { + void repeatImportsReuseVersionsAndKeepEditedDraftsEvenAfterReExport() throws Exception { + Map saved = recipientState(); ScreenerTransfer export = service.exportScreener("sender", source.getId()); - doReturn(List.of(draft)).when(checks).getAllWorkingCustomChecks("recipient"); Screener one = service.importScreener("recipient", export); - Screener two = service.importScreener("recipient", export); + EligibilityCheck localDraft = saved.values().stream().filter(c -> c.getId().startsWith("W-")).findFirst().orElseThrow(); + localDraft.setName("My renamed draft"); + localDraft.setModule("My module"); + localDraft.setVersion("9.0.0"); + localDraft.setDmnModel(draft.getDmnModel().replace("Income", "My renamed draft").replace("true", "false")); + clearInvocations(checks, storage, screeners); + Screener two = service.importScreener("recipient", withName(export, "Second copy")); assertNotEquals(one.getId(), two.getId()); - ArgumentCaptor saved = ArgumentCaptor.forClass(EligibilityCheck.class); - verify(checks, times(2)).saveNewWorkingCustomCheck(saved.capture()); - assertNotEquals(saved.getAllValues().get(0).getId(), saved.getAllValues().get(1).getId()); + verify(checks, never()).saveNewWorkingCustomCheck(any()); + verify(checks, never()).saveNewPublishedCustomCheck(any()); + verify(checks, never()).updateWorkingCustomCheck(any()); + verify(checks, never()).reserveCheckName(anyString(), anyString(), anyString(), anyString()); + verify(storage, never()).writeStringToStorage(startsWith("check/"), anyString(), anyString()); + assertEquals("My renamed draft", localDraft.getName()); + assertTrue(localDraft.getDmnModel().contains("false")); + ArgumentCaptor importedBenefits = ArgumentCaptor.forClass(Benefit.class); + verify(screeners, times(2)).saveNewCustomBenefit(eq(two.getId()), importedBenefits.capture()); + assertEquals("income", importedBenefits.getAllValues().getFirst().getChecks().getFirst().getCheckModule()); + + // Re-export from the recipient, then bring that file back to the original account. + // The origin remains stable even though local family IDs and the draft's name changed. + when(screeners.getWorkingScreenerMetaDataOnly(two.getId())).thenReturn(Optional.of(two)); + when(screeners.getWorkingScreener(two.getId())).thenReturn(Optional.of(two)); + when(screeners.getBenefitsInScreener(two)).thenReturn(importedBenefits.getAllValues()); + ScreenerTransfer reexported = service.exportScreener("recipient", two.getId()); + assertTrue(reexported.customChecks().stream().allMatch(c -> "example".equals(c.getOriginCheckId()))); + List senderChecks = new ArrayList<>(); + senderChecks.add(draft); + senderChecks.add(check("P-family-1.0.0", "1.0.0")); + senderChecks.add(check("P-family-2.0.0", "2.0.0")); + doReturn(senderChecks).when(checks).getCustomChecksForImport("sender"); + service.importScreener("sender", withName(reexported, "Back to sender")); + verify(checks, never()).saveNewWorkingCustomCheck(any()); + verify(checks, never()).saveNewPublishedCustomCheck(any()); + } + + @Test + void importIntoOriginalAccountReusesNativeIdsAndArchivedDrafts() throws Exception { + draft.setIsArchived(true); + doReturn(List.of(draft, check("P-family-1.0.0", "1.0.0"), check("P-family-2.0.0", "2.0.0"))) + .when(checks).getCustomChecksForImport("sender"); + service.importScreener("sender", service.exportScreener("sender", source.getId())); + verify(checks, never()).saveNewWorkingCustomCheck(any()); + verify(checks, never()).saveNewPublishedCustomCheck(any()); + assertTrue(draft.getIsArchived()); + ArgumentCaptor imported = ArgumentCaptor.forClass(Benefit.class); + verify(screeners, times(2)).saveNewCustomBenefit(anyString(), imported.capture()); + assertEquals("P-family-1.0.0", imported.getAllValues().getFirst().getChecks().getFirst().getSourceCheckId()); + } + + @Test + void checksInAnotherAccountAreNeverReused() throws Exception { + doReturn(List.of(draft, check("P-family-1.0.0", "1.0.0"), check("P-family-2.0.0", "2.0.0"))) + .when(checks).getCustomChecksForImport("recipient"); + service.importScreener("recipient", service.exportScreener("sender", source.getId())); + var saved = ArgumentCaptor.forClass(EligibilityCheck.class); + verify(checks).saveNewWorkingCustomCheck(saved.capture()); + assertNotEquals(draft.getId(), saved.getValue().getId()); + assertEquals("recipient", saved.getValue().getOwnerId()); + assertEquals("income", saved.getValue().getModule()); + } + + @Test + void unrelatedCheckWithSameNameGetsItsOwnFamilyAndModule() throws Exception { + EligibilityCheck unrelated = check("W-unrelated", "1.0.0"); + unrelated.setOwnerId("recipient"); + unrelated.setExampleSourceId(null); + doReturn(List.of(unrelated)).when(checks).getCustomChecksForImport("recipient"); + service.importScreener("recipient", service.exportScreener("sender", source.getId())); + var saved = ArgumentCaptor.forClass(EligibilityCheck.class); + verify(checks).saveNewWorkingCustomCheck(saved.capture()); + assertNotEquals(unrelated.getId(), saved.getValue().getId()); assertEquals("income (imported 2)", saved.getValue().getModule()); - assertEquals("Income", saved.getValue().getName()); - assertEquals("income", draft.getModule()); + } + + @Test + void missingPublishedVersionIsAddedWithoutReplacingTheDraft() throws Exception { + Map saved = recipientState(); + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + service.importScreener("recipient", export); + var removed = saved.values().stream().filter(c -> c.getId().startsWith("P-") && "2.0.0".equals(c.getVersion())) + .findFirst().orElseThrow(); + saved.remove(removed.getId()); + clearInvocations(checks, storage); + service.importScreener("recipient", withName(export, "With version two")); + verify(checks, never()).saveNewWorkingCustomCheck(any()); + verify(checks, never()).updateWorkingCustomCheck(any()); + verify(checks).saveNewPublishedCustomCheck(argThat(c -> c.getId().equals(removed.getId()))); + verify(storage).writeStringToStorage(eq("check/" + removed.getId() + ".dmn"), anyString(), eq("application/xml")); + } + + @Test + void conflictingPublishedRulesRejectTheWholeImportBeforeWrites() throws Exception { + Map saved = recipientState(); + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + service.importScreener("recipient", export); + saved.values().stream().filter(c -> c.getId().startsWith("P-")).findFirst().orElseThrow() + .setDmnModel(draft.getDmnModel().replace("true", "false")); + clearInvocations(checks, screeners, storage); + var failure = assertThrows(CustomCheckImportConflictException.class, + () -> service.importScreener("recipient", withName(export, "Conflicting copy"))); + assertTrue(failure.getMessage().contains("different rules or parameter definitions")); + verify(storage, never()).writeStringToStorage(anyString(), anyString(), anyString()); + verify(screeners, never()).saveNewCustomBenefit(anyString(), any()); + verify(checks, never()).deletePublishedCustomCheck(anyString()); + } + + @Test + void conflictingParameterDefinitionsRejectReuse() throws Exception { + Map saved = recipientState(); + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + service.importScreener("recipient", export); + ParameterDefinition parameter = new ParameterDefinition(); + parameter.setKey("new-limit"); + saved.values().stream().filter(c -> c.getId().startsWith("P-")).findFirst().orElseThrow() + .setParameterDefinitions(List.of(parameter)); + clearInvocations(storage); + assertThrows(CustomCheckImportConflictException.class, + () -> service.importScreener("recipient", withName(export, "Conflicting parameters"))); + verify(storage, never()).writeStringToStorage(anyString(), anyString(), anyString()); + } + + @Test + void xmlIndentationDoesNotCauseAFalseVersionConflict() throws Exception { + Map saved = recipientState(); + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + service.importScreener("recipient", export); + saved.values().stream().filter(c -> c.getId().startsWith("P-")).forEach(c -> + c.setDmnModel(c.getDmnModel().replace("><", ">\n <"))); + clearInvocations(checks); + service.importScreener("recipient", withName(export, "Formatted copy")); + verify(checks, never()).saveNewPublishedCustomCheck(any()); + } + + @Test + void referencedDraftConflictDoesNotSilentlyChangeTheImportedRules() throws Exception { + Map saved = recipientState(); + service.importScreener("recipient", service.exportScreener("sender", source.getId())); + saved.values().stream().filter(c -> c.getId().startsWith("W-")).findFirst().orElseThrow() + .setDmnModel(draft.getDmnModel().replace("true", "false")); + CheckConfig configured = benefits.getLast().getChecks().getFirst(); + configured.setSourceCheckId(draft.getId()); + configured.setCheckVersion(draft.getVersion()); + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + clearInvocations(storage); + var failure = assertThrows(CustomCheckImportConflictException.class, + () -> service.importScreener("recipient", withName(export, "Draft conflict"))); + assertTrue(failure.getMessage().contains("differs from your draft")); + verify(storage, never()).writeStringToStorage(anyString(), anyString(), anyString()); + } + + @Test + void oldExportsWithoutOriginIdentityRemainImportableAndReusable() throws Exception { + recipientState(); + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + export.customChecks().forEach(c -> c.setOriginCheckId(null)); + service.importScreener("recipient", export); + clearInvocations(checks, screeners); + Screener legacyCopy = service.importScreener("recipient", withName(export, "Legacy copy")); + verify(checks, never()).saveNewWorkingCustomCheck(any()); + verify(checks, never()).saveNewPublishedCustomCheck(any()); + var importedBenefits = ArgumentCaptor.forClass(Benefit.class); + verify(screeners, times(2)).saveNewCustomBenefit(eq(legacyCopy.getId()), importedBenefits.capture()); + when(screeners.getWorkingScreenerMetaDataOnly(legacyCopy.getId())).thenReturn(Optional.of(legacyCopy)); + when(screeners.getWorkingScreener(legacyCopy.getId())).thenReturn(Optional.of(legacyCopy)); + when(screeners.getBenefitsInScreener(legacyCopy)).thenReturn(importedBenefits.getAllValues()); + doReturn(List.of(draft, check("P-family-1.0.0", "1.0.0"), check("P-family-2.0.0", "2.0.0"))) + .when(checks).getCustomChecksForImport("sender"); + service.importScreener("sender", withName(service.exportScreener("recipient", legacyCopy.getId()), "Legacy shared back")); + verify(checks, never()).saveNewWorkingCustomCheck(any()); + verify(checks, never()).saveNewPublishedCustomCheck(any()); + } + + @Test + void rollbackNeverDeletesReusedCheckVersionsOrDrafts() throws Exception { + recipientState(); + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + service.importScreener("recipient", export); + doThrow(new Exception("Storage unavailable")).when(storage) + .writeStringToStorage(startsWith("form/"), anyString(), eq("application/json")); + clearInvocations(checks, storage); + assertThrows(Exception.class, () -> service.importScreener("recipient", withName(export, "Failed copy"))); + verify(checks, never()).deleteWorkingCustomCheck(anyString()); + verify(checks, never()).deletePublishedCustomCheck(anyString()); + verify(storage, never()).deleteFile(startsWith("check/")); + } + + @Test + void concurrentPublishedCreateDoesNotOverwriteOrDeleteTheWinner() throws Exception { + Map saved = recipientState(); + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + service.importScreener("recipient", export); + var version = saved.values().stream().filter(c -> c.getId().startsWith("P-") && "2.0.0".equals(c.getVersion())) + .findFirst().orElseThrow(); + doReturn(saved.values().stream().filter(c -> !c.getId().equals(version.getId())).toList()) + .when(checks).getCustomChecksForImport("recipient"); + doThrow(new DocumentAlreadyExistsException(version.getId(), null)).when(checks) + .saveNewPublishedCustomCheck(argThat(c -> c.getId().equals(version.getId()))); + doThrow(new Exception("Storage unavailable")).when(storage) + .writeStringToStorage(startsWith("form/"), anyString(), eq("application/json")); + clearInvocations(checks, storage); + assertThrows(Exception.class, () -> service.importScreener("recipient", withName(export, "Racing copy"))); + verify(storage, never()).writeStringToStorage(startsWith("check/"), anyString(), anyString()); + verify(checks, never()).deletePublishedCustomCheck(anyString()); + } + + @Test + void concurrentFamilyCreationIsRejectedBeforeNameOrArtifactWrites() throws Exception { + doThrow(new DocumentAlreadyExistsException("origin", null)).when(checks) + .reserveImportIdentity(anyString(), anyString(), anyString()); + var export = service.exportScreener("sender", source.getId()); + assertThrows(CustomCheckImportConflictException.class, () -> service.importScreener("recipient", export)); + verify(checks, never()).reserveCheckName(anyString(), anyString(), anyString(), anyString()); + verify(storage, never()).writeStringToStorage(anyString(), anyString(), anyString()); + } + + @Test + void unavailableMetadataReadDoesNotCreateDuplicateFamilies() throws Exception { + doThrow(new Exception("Firestore unavailable")).when(checks).getCustomChecksForImport("recipient"); + var export = service.exportScreener("sender", source.getId()); + assertThrows(Exception.class, () -> service.importScreener("recipient", export)); + verify(checks, never()).reserveImportIdentity(anyString(), anyString(), anyString()); + verify(storage, never()).writeStringToStorage(anyString(), anyString(), anyString()); + } + + private ScreenerTransfer withName(ScreenerTransfer transfer, String name) { + return new ScreenerTransfer(transfer.format(), transfer.formatVersion(), name, + transfer.formSchema(), transfer.benefits(), transfer.customChecks()); + } + + private Map recipientState() throws Exception { + Map saved = new LinkedHashMap<>(); + doAnswer(i -> new ArrayList<>(saved.values())).when(checks).getCustomChecksForImport("recipient"); + for (boolean published : List.of(false, true)) { + org.mockito.stubbing.Answer save = i -> { + EligibilityCheck check = mapper.convertValue(i.getArgument(0), EligibilityCheck.class); + saved.put(check.getId(), check); + return check.getId(); + }; + if (published) { + doAnswer(save).when(checks).saveNewPublishedCustomCheck(any()); + doAnswer(i -> Optional.ofNullable(saved.get(i.getArgument(1)))) + .when(checks).getPublishedCustomCheck(eq("recipient"), anyString(), eq(true)); + } else { + doAnswer(save).when(checks).saveNewWorkingCustomCheck(any()); + doAnswer(i -> Optional.ofNullable(saved.get(i.getArgument(1)))) + .when(checks).getWorkingCustomCheck(eq("recipient"), anyString(), eq(true)); + } + } + doAnswer(i -> { + String path = i.getArgument(0); + String id = path.substring("check/".length(), path.length() - ".dmn".length()); + saved.get(id).setDmnModel(i.getArgument(1)); + return null; + }).when(storage).writeStringToStorage(startsWith("check/"), anyString(), eq("application/xml")); + return saved; } @Test diff --git a/builder-frontend/README.md b/builder-frontend/README.md index ca6cad39..0f165892 100644 --- a/builder-frontend/README.md +++ b/builder-frontend/README.md @@ -56,6 +56,12 @@ The Screeners page offers **Import screener**. Each screener card's menu offers **Export screener**, which downloads the saved draft as a single `.bdt.json` file. The import dialog prefills the exported screener name and requires a different name if it already exists in the account. Creation, rename, and import enforce per-user name uniqueness, ignoring case and surrounding spaces. -Import creates a new unpublished screener and independent custom checks for the +Import creates a new unpublished screener for the signed-in user. Files contain a `bdt-screener` format marker and `formatVersion: 1`. +Custom checks retain an `originCheckId` across imports and later exports. Existing +families and matching published versions are reused; missing versions are added, +and recipient draft edits are preserved. Conflicting published content (or a +referenced draft that differs) returns a readable 409 error. Unrelated checks +with the same name remain separate and receive an `imported` module suffix. +Older exports without `originCheckId` use their original working family ID. Library check references are resolved against the receiving server's library. diff --git a/docs/src/content/docs/user/user-guide.md b/docs/src/content/docs/user/user-guide.md index fc43142a..caebaa2a 100644 --- a/docs/src/content/docs/user/user-guide.md +++ b/docs/src/content/docs/user/user-guide.md @@ -34,7 +34,7 @@ Save your changes, then return to **Screeners**, open the menu on the screener's To open a shared screener, select **Import screener** on the **Screeners** page, choose the JSON file (up to 10 MB), and review the prefilled **Screener name**. If that name already exists in your account, choose a different name before selecting **Import screener** in the dialog. Screener names are unique within your account, ignoring case and surrounding spaces. BDT opens a new editable draft owned by your account. You can preview, edit, and publish it with its own public URL. -Each import creates independent copies of the custom checks. If a custom check's name and module already exist in your account, the copy goes into a module with an `imported` suffix. The screener keeps the check versions used by the sender. Library checks use your BDT server's library and must be available there; a file with missing checks is rejected before anything is imported. +Custom checks keep their original identity when shared. Import reuses checks already in your account, keeps your draft edits, and adds any missing published versions. If the same check version contains different rules or parameter definitions, BDT rejects the import and explains the conflict. A screener that directly uses a changed draft is also rejected; publish and select a version before sharing it. Checks with a different identity remain separate even if their names match; a module with an `imported` suffix resolves those name collisions. The screener keeps the check versions used by the sender. Library checks use your BDT server's library and must be available there; a file with missing checks is rejected before anything is imported. --- diff --git a/e2e/tests/exampleScreenerTransfer.spec.ts b/e2e/tests/exampleScreenerTransfer.spec.ts index 7fe97966..3c8a4210 100644 --- a/e2e/tests/exampleScreenerTransfer.spec.ts +++ b/e2e/tests/exampleScreenerTransfer.spec.ts @@ -3,7 +3,7 @@ import { randomUUID } from "node:crypto"; // Provision our own local demo account instead of clearing an existing developer's data. // This also exercises the exact bundled example received by a new analyst. -test("the bundled example can be chosen, exported, and imported with its internal library checks", async ({ +test("the bundled example can be chosen, exported, and imported with its internal library checks and reused custom checks", async ({ page, request, }) => { @@ -153,4 +153,164 @@ test("the bundled example can be chosen, exported, and imported with its interna for (const benefit of imported.benefits) { await expect(page.getByText(benefit.name, { exact: true })).toBeVisible(); } + + const readWorking = async (token: string) => { + const response = await request.get( + "http://localhost:8081/api/custom-checks?working=true&includeArchived=true", + { + headers: { Authorization: `Bearer ${token}` }, + }, + ); + expect(response.ok(), await response.text()).toBe(true); + return response.json(); + }; + const originalIds = exported.customChecks + .filter((check: any) => check.id.startsWith("W-")) + .map((check: any) => check.id) + .sort(); + expect(originalIds).toHaveLength(2); + expect( + (await readWorking(account.idToken)).map((check: any) => check.id).sort(), + ).toEqual(originalIds); + expect( + exported.customChecks.every( + (check: any) => typeof check.originCheckId === "string", + ), + ).toBe(true); + + // Two simultaneous transfers to a fresh recipient must create just one family per origin. + const recipientSignUp = await request.post( + "http://localhost:9099/identitytoolkit.googleapis.com/v1/accounts:signUp?key=local-demo", + { + data: { + email: `import-recipient-${randomUUID()}@example.com`, + password, + returnSecureToken: true, + }, + }, + ); + expect(recipientSignUp.ok()).toBe(true); + const recipient = await recipientSignUp.json(); + const recipientHeaders = { Authorization: `Bearer ${recipient.idToken}` }; + const importForRecipient = (name: string) => + request.post("http://localhost:8081/api/screener/import", { + headers: recipientHeaders, + data: { ...exported, screenerName: name }, + }); + const concurrent = await Promise.all([ + importForRecipient("Recipient one"), + importForRecipient("Recipient two"), + ]); + expect(concurrent.some((response) => response.status() === 201)).toBe(true); + for (const [index, response] of concurrent.entries()) { + expect([201, 409], await response.text()).toContain(response.status()); + if (response.status() === 409) { + const retry = await importForRecipient( + index === 0 ? "Recipient one" : "Recipient two", + ); + expect(retry.status(), await retry.text()).toBe(201); + } + } + const recipientDrafts = await readWorking(recipient.idToken); + expect(recipientDrafts).toHaveLength(2); + expect( + recipientDrafts.every((check: any) => !originalIds.includes(check.id)), + ).toBe(true); + const editedDraft = recipientDrafts[0]; + const draftResponse = await request.get( + `http://localhost:8081/api/custom-checks/${editedDraft.id}`, + { headers: recipientHeaders }, + ); + expect(draftResponse.ok()).toBe(true); + const draft = await draftResponse.json(); + const editedXml = draft.dmnModel.replace( + /[\s\S]*?<\/dmn:text>/, + "false", + ); + expect(editedXml).not.toBe(draft.dmnModel); + const edit = await request.put( + `http://localhost:8081/api/custom-checks/${editedDraft.id}/dmn`, + { + headers: recipientHeaders, + data: { dmnModel: editedXml }, + }, + ); + expect(edit.ok(), await edit.text()).toBe(true); + const repeat = await importForRecipient("Recipient three"); + expect(repeat.status(), await repeat.text()).toBe(201); + const repeatScreener = await repeat.json(); + expect( + (await readWorking(recipient.idToken)).map((check: any) => check.id).sort(), + ).toEqual(recipientDrafts.map((check: any) => check.id).sort()); + const retained = await request.get( + `http://localhost:8081/api/custom-checks/${editedDraft.id}`, + { headers: recipientHeaders }, + ); + expect((await retained.json()).dmnModel).toBe(editedXml); + const reexportResponse = await request.get( + `http://localhost:8081/api/screener/${repeatScreener.id}/export`, + { headers: recipientHeaders }, + ); + expect(reexportResponse.ok()).toBe(true); + const reexported = await reexportResponse.json(); + expect( + reexported.customChecks.map((check: any) => check.originCheckId).sort(), + ).toEqual( + exported.customChecks.map((check: any) => check.originCheckId).sort(), + ); + const sharedBack = await request.post( + "http://localhost:8081/api/screener/import", + { + headers, + data: { ...reexported, screenerName: "Shared back" }, + }, + ); + expect(sharedBack.status(), await sharedBack.text()).toBe(201); + expect( + (await readWorking(account.idToken)).map((check: any) => check.id).sort(), + ).toEqual(originalIds); + + // A file that claims the same published identity/version with changed rules gets a readable UI error. + const conflicting = structuredClone(exported); + const conflictingVersion = conflicting.customChecks.find((check: any) => + check.id.startsWith("P-"), + ); + conflictingVersion.dmnModel = conflictingVersion.dmnModel.replace( + /[\s\S]*?<\/dmn:text>/, + "false", + ); + conflicting.screenerName = "Conflicting version"; + await page.goto("/screeners"); + await page + .getByRole("button", { name: "Import screener", exact: true }) + .click(); + await page.getByLabel("Screener file").setInputFiles({ + name: "conflicting.bdt.json", + mimeType: "application/json", + buffer: Buffer.from(JSON.stringify(conflicting)), + }); + await expect(page.getByLabel("Screener name", { exact: true })).toHaveValue( + "Conflicting version", + ); + const conflictPromise = page.waitForResponse( + (response) => + response.url().endsWith("/api/screener/import") && + response.request().method() === "POST", + ); + await page + .locator("form") + .getByRole("button", { name: "Import screener", exact: true }) + .click(); + expect((await conflictPromise).status()).toBe(409); + await expect(page.getByRole("alert")).toContainText( + "different rules or parameter definitions", + ); + await expect( + page + .locator("form") + .getByRole("button", { name: "Import screener", exact: true }), + ).toBeEnabled(); + expect( + (await readWorking(account.idToken)).map((check: any) => check.id).sort(), + ).toEqual(originalIds); }); diff --git a/e2e/tests/screenerTransfer.spec.ts b/e2e/tests/screenerTransfer.spec.ts index 222909a5..b719836e 100644 --- a/e2e/tests/screenerTransfer.spec.ts +++ b/e2e/tests/screenerTransfer.spec.ts @@ -35,7 +35,7 @@ test.describe("Screener sharing", () => { test.beforeEach(authLogin); test.afterEach(resetEmulator); - test("exports and imports a complete screener with independent custom checks", async ({ + test("exports and imports a complete screener while reusing its custom checks", async ({ page, }) => { await seedScreenerWithForm("Shared screener"); @@ -161,12 +161,11 @@ test.describe("Screener sharing", () => { { timeout: 15000 }, ); - // The two working copies must remain distinct, even when importing into the sender's account. + // Importing back into the same account keeps the original custom-check family. const workingChecks = (await getCollection("workingCustomCheck")) as any; - expect(workingChecks.documents).toHaveLength(2); - const copy = workingChecks.documents.find( - (document: any) => document.fields.id.stringValue !== workingCheckId, + expect(workingChecks.documents).toHaveLength(1); + expect(workingChecks.documents[0].fields.id.stringValue).toBe( + workingCheckId, ); - expect(copy.fields.module.stringValue).toBe("e2e (imported 2)"); }); }); From 8c87ec3b4b62de718a7b8929240818523fa1c5c9 Mon Sep 17 00:00:00 2001 From: Preston Cabe Date: Mon, 5 Oct 2026 21:29:28 -0400 Subject: [PATCH 04/14] Check screener name uniqueness only when the name changes Benefit edits, benefit deletes, and publishing saved the working screener through the unique-name transaction. Accounts that already had two screeners with the same name could no longer edit or delete benefits, and a failed delete left the screener listing a removed benefit. Every save also queried all of the owner's screeners and serialized their writes. Plain updates now write the document directly. The PATCH endpoint uses the transactional rename only when the normalized name changes. Co-Authored-By: Claude Opus 5.5 --- .../builder/controller/ScreenerResource.java | 9 +++- .../bdt/builder/model/domain/Screener.java | 5 ++ .../builder/persistence/FirestoreUtils.java | 9 ++-- .../persistence/ScreenerRepository.java | 3 ++ .../impl/ScreenerRepositoryImpl.java | 8 +++ .../controller/ScreenerResourceTest.java | 54 +++++++++++++++++++ 6 files changed, 80 insertions(+), 8 deletions(-) create mode 100644 builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerResourceTest.java diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerResource.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerResource.java index 10c39179..49d455be 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerResource.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerResource.java @@ -144,12 +144,17 @@ public Response updateScreener( Log.info(request.toString()); // Update Screener fields from request + // Only a changed name is checked for uniqueness, so screeners that already share a name stay editable. + boolean renamed = request.screenerName() != null + && !Screener.normalizeName(request.screenerName()) + .equals(Screener.normalizeName(screener.getScreenerName())); if (request.screenerName() != null) { - screener.setScreenerName(request.screenerName()); + screener.setScreenerName(request.screenerName().strip()); } try { - screenerRepository.updateWorkingScreener(screener); + if (renamed) screenerRepository.renameWorkingScreener(screener); + else screenerRepository.updateWorkingScreener(screener); return Response.ok(screener, MediaType.APPLICATION_JSON).build(); } catch (DuplicateScreenerNameException duplicate) { return Response.status(409).entity(Map.of("error", duplicate.getMessage())).build(); diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/model/domain/Screener.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/model/domain/Screener.java index 2d050f45..2016fb2d 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/model/domain/Screener.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/model/domain/Screener.java @@ -34,6 +34,11 @@ public static Screener create(String ownerId, String screenerName, String descri return s; } + /* Screener names are unique per owner, ignoring case and surrounding spaces. */ + public static String normalizeName(String name) { + return name == null ? "" : name.strip().toLowerCase(java.util.Locale.ROOT); + } + public Map getFormSchema() { return formSchema; } diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/FirestoreUtils.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/FirestoreUtils.java index 3d3a491a..e3799896 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/FirestoreUtils.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/FirestoreUtils.java @@ -6,6 +6,7 @@ import com.google.cloud.firestore.*; import com.google.firebase.cloud.FirestoreClient; import io.quarkus.logging.Log; +import org.codeforphilly.bdt.builder.model.domain.Screener; import java.util.*; import java.util.concurrent.ExecutionException; @@ -251,8 +252,8 @@ public static void saveScreenerWithUniqueName(String collection, String id, Map< QuerySnapshot screeners = transaction.get(db.collection(collection) .whereEqualTo("ownerId", owner)).get(); for (QueryDocumentSnapshot existing : screeners.getDocuments()) { - if (!id.equals(existing.getId()) && normalizeScreenerName(name) - .equals(normalizeScreenerName(existing.getString("screenerName")))) { + if (!id.equals(existing.getId()) && Screener.normalizeName(name) + .equals(Screener.normalizeName(existing.getString("screenerName")))) { throw new DuplicateScreenerNameException(); } } @@ -274,10 +275,6 @@ public static void saveScreenerWithUniqueName(String collection, String id, Map< } } - public static String normalizeScreenerName(String name) { - return name == null ? "" : name.strip().toLowerCase(Locale.ROOT); - } - public static void updateDocument(String collectionName, Map data, String docId) throws Exception { try { WriteResult result = db.collection(collectionName) diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/ScreenerRepository.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/ScreenerRepository.java index c8eddb1a..313949a7 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/ScreenerRepository.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/ScreenerRepository.java @@ -18,6 +18,9 @@ public interface ScreenerRepository { public void updateWorkingScreener(Screener screener) throws Exception; + /** Saves a new name, rejecting one already used by another of the owner's screeners. */ + public void renameWorkingScreener(Screener screener) throws Exception; + public void addBenefitDetailToWorkingScreener(String screenerId, BenefitDetail benefitDetail) throws Exception; public void deleteWorkingScreener(String screenerId) throws Exception; diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/ScreenerRepositoryImpl.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/ScreenerRepositoryImpl.java index 82a6a09e..dabb5456 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/ScreenerRepositoryImpl.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/impl/ScreenerRepositoryImpl.java @@ -105,6 +105,14 @@ public void updateWorkingScreener(Screener screener) throws Exception { // It is saved separately on in cloud storage. data.remove("formSchema"); + FirestoreUtils.updateDocument(CollectionNames.WORKING_SCREENER_COLLECTION, data, screener.getId()); + } + + @Override + public void renameWorkingScreener(Screener screener) throws Exception { + ObjectMapper mapper = new ObjectMapper().setSerializationInclusion(JsonInclude.Include.NON_NULL); + Map data = mapper.convertValue(screener, Map.class); + data.remove("formSchema"); validateName(screener); data.put("screenerName", screener.getScreenerName()); FirestoreUtils.saveScreenerWithUniqueName(CollectionNames.WORKING_SCREENER_COLLECTION, diff --git a/builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerResourceTest.java b/builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerResourceTest.java new file mode 100644 index 00000000..c08fe5e5 --- /dev/null +++ b/builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerResourceTest.java @@ -0,0 +1,54 @@ +package org.codeforphilly.bdt.builder.controller; + +import io.quarkus.security.identity.SecurityIdentity; +import org.codeforphilly.bdt.builder.model.domain.Screener; +import org.codeforphilly.bdt.builder.model.dto.screener.EditScreenerRequest; +import org.codeforphilly.bdt.builder.persistence.DuplicateScreenerNameException; +import org.codeforphilly.bdt.builder.persistence.ScreenerRepository; +import org.eclipse.microprofile.jwt.JsonWebToken; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.util.Optional; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.*; + +class ScreenerResourceTest { + private final ScreenerRepository screenerRepository = mock(ScreenerRepository.class); + private final SecurityIdentity identity = mock(SecurityIdentity.class); + private final ScreenerResource resource = new ScreenerResource(); + private Screener screener; + + @BeforeEach + void setUp() { + resource.screenerRepository = screenerRepository; + JsonWebToken token = mock(JsonWebToken.class); + when(identity.getPrincipal()).thenReturn(token); + when(token.getClaim("user_id")).thenReturn("owner"); + screener = Screener.create("owner", "Housing", null); + screener.setId("screener-1"); + when(screenerRepository.getWorkingScreener("screener-1")).thenReturn(Optional.of(screener)); + } + + @Test + void changedNameIsCheckedForUniqueness() throws Exception { + doThrow(new DuplicateScreenerNameException()).when(screenerRepository).renameWorkingScreener(any()); + try (var response = resource.updateScreener(identity, "screener-1", new EditScreenerRequest("Food"))) { + assertEquals(409, response.getStatus()); + } + verify(screenerRepository, never()).updateWorkingScreener(any()); + } + + @Test + void unchangedNameSavesWithoutTheUniquenessCheck() throws Exception { + // Screeners created before names were unique may already share this name. + try (var response = resource.updateScreener(identity, "screener-1", new EditScreenerRequest(" housing "))) { + assertEquals(200, response.getStatus()); + } + verify(screenerRepository).updateWorkingScreener(screener); + assertEquals("housing", screener.getScreenerName()); + verify(screenerRepository, never()).renameWorkingScreener(any()); + } +} From 34c0712c3da26f6b5b53e54400e9ee1f11e7261d Mon Sep 17 00:00:00 2001 From: Preston Cabe Date: Mon, 5 Oct 2026 21:29:57 -0400 Subject: [PATCH 05/14] Export the editor's benefit list when stored benefits disagree Creating and deleting a benefit writes the benefit document and the screener's benefit list separately, so a partial failure can leave them out of step. Export then failed with a generic 500 the user could not fix. It now exports the benefits the editor lists, skipping list entries with no stored benefit and stored benefits the list no longer includes, and logs what it skipped. Co-Authored-By: Claude Opus 5.5 --- .../bdt/builder/service/ScreenerTransferService.java | 10 +++++++--- .../builder/service/ScreenerTransferServiceTest.java | 10 ++++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java index d403ca17..e91a3c3e 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java @@ -38,15 +38,19 @@ public ScreenerTransfer exportScreener(String owner, String id) throws Exception List benefits = new ArrayList<>(); Map byId = new HashMap<>(); for (Benefit benefit : screeners.getBenefitsInScreener(screener)) byId.put(benefit.getId(), benefit); - // The screener's benefit list determines the order shown in the editor. + // Export what the editor shows: its benefit list, in order. Benefit writes are not atomic with that + // list, so skip entries without a stored benefit and stored benefits the list no longer includes. if (screener.getBenefits() != null) { for (BenefitDetail detail : screener.getBenefits()) { Benefit benefit = byId.remove(detail.getId()); - if (benefit == null) throw new IllegalStateException("Missing screener benefit"); + if (benefit == null) { + Log.warn("Exporting screener " + id + " without missing benefit " + detail.getId()); + continue; + } benefits.add(copy(benefit, Benefit.class)); } } - if (!byId.isEmpty()) throw new IllegalStateException("Screener benefit list is inconsistent"); + if (!byId.isEmpty()) Log.warn("Exporting screener " + id + " without unlisted benefits " + byId.keySet()); Map customChecks = new LinkedHashMap<>(); for (Benefit benefit : benefits) { benefit.setOwnerId(null); diff --git a/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java index 5783c317..3e3fef34 100644 --- a/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java +++ b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java @@ -419,6 +419,16 @@ void rejectsUnsupportedFormatAndUnresolvedReferences() throws Exception { verify(screeners, never()).saveNewWorkingScreener(any()); } + @Test + void exportFollowsTheEditorsBenefitListWhenStoredBenefitsDisagree() throws Exception { + // A partly failed benefit create or delete leaves the list and the stored benefits out of step. + Benefit unlisted = new Benefit("benefit-orphan", "Orphan", null, "sender", List.of()); + when(screeners.getBenefitsInScreener(source)).thenReturn(List.of(benefits.get(0), unlisted)); + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + assertEquals(List.of("benefit-b"), export.benefits().stream().map(Benefit::getId).toList()); + assertEquals(2, export.customChecks().size()); + } + @Test void refusesExportsForOtherUsersBeforeReadingArtifacts() throws Exception { assertThrows(ForbiddenException.class, () -> service.exportScreener("recipient", source.getId())); From 04ad8ad7bf734a70e0ff379351afe7ef7291e1e1 Mon Sep 17 00:00:00 2001 From: Preston Cabe Date: Mon, 5 Oct 2026 21:31:14 -0400 Subject: [PATCH 06/14] Export only the custom-check versions a screener uses Export also bundled the author's current working draft of each check. A draft that was mid-edit could fail import validation and reject a file whose published versions were fine, and a successful import shared the author's unpublished edits as the recipient's draft. The file now holds only the referenced versions; import already seeds a new family's draft from the latest version it receives. Co-Authored-By: Claude Opus 5.5 --- .../builder/service/ScreenerTransferService.java | 12 ++---------- .../service/ScreenerTransferServiceTest.java | 16 +++++++++------- builder-frontend/README.md | 4 +++- docs/src/content/docs/user/user-guide.md | 4 ++-- e2e/tests/exampleScreenerTransfer.spec.ts | 16 ++++++++++++---- 5 files changed, 28 insertions(+), 24 deletions(-) diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java index e91a3c3e..194270b2 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java @@ -57,17 +57,9 @@ public ScreenerTransfer exportScreener(String owner, String id) throws Exception for (CheckConfig config : configs(benefit)) { String source = sourceId(config); if (source.startsWith("L")) continue; + // Only the versions the screener uses are shared, never the author's unpublished draft. if (customChecks.containsKey(source)) continue; - EligibilityCheck check = loadCheck(owner, source); - customChecks.put(source, portableCheck(check)); - String workingId = checks.getWorkingId(check); - if (!customChecks.containsKey(workingId)) { - Optional working = checks.getWorkingCustomCheck(owner, workingId, true); - if (working.isPresent()) { - requireOwnedCheck(owner, working.get()); - customChecks.put(workingId, portableCheck(working.get())); - } - } + customChecks.put(source, portableCheck(loadCheck(owner, source))); } } return new ScreenerTransfer(ScreenerTransfer.FORMAT, ScreenerTransfer.VERSION, diff --git a/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java index 3e3fef34..8fd3100f 100644 --- a/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java +++ b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java @@ -80,7 +80,8 @@ void roundTripPreservesRulesVersionsFormAndSettingsWithNewOwnershipAndIds() thro String json = mapper.writeValueAsString(export); assertFalse(json.contains("ownerId\":\"sender")); assertFalse(json.contains("public-link")); - assertEquals(3, export.customChecks().size()); + assertEquals(List.of("P-family-1.0.0", "P-family-2.0.0"), + export.customChecks().stream().map(EligibilityCheck::getId).sorted().toList()); // Exercise the exact single-file representation, including embedded XML. Screener imported = service.importScreener("recipient", mapper.readValue(json, ScreenerTransfer.class)); assertNotEquals(source.getId(), imported.getId()); @@ -106,7 +107,7 @@ void roundTripPreservesRulesVersionsFormAndSettingsWithNewOwnershipAndIds() thro ArgumentCaptor savedDraft = ArgumentCaptor.forClass(EligibilityCheck.class); verify(checks).saveNewWorkingCustomCheck(savedDraft.capture()); EligibilityCheck importedDraft = savedDraft.getValue(); - assertEquals("3.0.0", importedDraft.getVersion()); + assertEquals("2.0.0", importedDraft.getVersion()); assertEquals("recipient", importedDraft.getOwnerId()); assertFalse(importedDraft.getIsArchived()); assertNull(importedDraft.getExampleSourceId()); @@ -426,7 +427,7 @@ void exportFollowsTheEditorsBenefitListWhenStoredBenefitsDisagree() throws Excep when(screeners.getBenefitsInScreener(source)).thenReturn(List.of(benefits.get(0), unlisted)); ScreenerTransfer export = service.exportScreener("sender", source.getId()); assertEquals(List.of("benefit-b"), export.benefits().stream().map(Benefit::getId).toList()); - assertEquals(2, export.customChecks().size()); + assertEquals(1, export.customChecks().size()); } @Test @@ -471,7 +472,7 @@ void emptyScreenerCanBeSharedWithoutAFormOrCustomChecks() throws Exception { void archivedChecksAlreadyUsedByAScreenerCanBeShared() throws Exception { draft.setIsArchived(true); ScreenerTransfer export = service.exportScreener("sender", source.getId()); - assertTrue(export.customChecks().stream().anyMatch(EligibilityCheck::getIsArchived)); + assertEquals(2, export.customChecks().size()); service.importScreener("recipient", export); ArgumentCaptor saved = ArgumentCaptor.forClass(EligibilityCheck.class); verify(checks).saveNewWorkingCustomCheck(saved.capture()); @@ -479,10 +480,11 @@ void archivedChecksAlreadyUsedByAScreenerCanBeShared() throws Exception { } @Test - void publishedModelCanSupplyAnEditableDraftWhenTheOriginalDraftIsMissing() throws Exception { - doReturn(Optional.empty()).when(checks).getWorkingCustomCheck("sender", "W-family", true); + void unpublishedDraftIsNotSharedAndTheLatestUsedVersionSeedsTheRecipientDraft() throws Exception { + // The author's draft is mid-edit and no longer has a decision named after the check. + draft.setDmnModel(""); ScreenerTransfer export = service.exportScreener("sender", source.getId()); - assertEquals(2, export.customChecks().size()); + assertTrue(export.customChecks().stream().noneMatch(c -> c.getId().startsWith("W-"))); service.importScreener("recipient", export); ArgumentCaptor saved = ArgumentCaptor.forClass(EligibilityCheck.class); verify(checks).saveNewWorkingCustomCheck(saved.capture()); diff --git a/builder-frontend/README.md b/builder-frontend/README.md index 0f165892..ae4724d6 100644 --- a/builder-frontend/README.md +++ b/builder-frontend/README.md @@ -58,7 +58,9 @@ The import dialog prefills the exported screener name and requires a different n Import creates a new unpublished screener for the signed-in user. Files contain a `bdt-screener` format marker and `formatVersion: 1`. -Custom checks retain an `originCheckId` across imports and later exports. Existing +Exports include only the custom-check versions the benefits reference, never the +author's unpublished draft; a new family's draft is seeded from its latest included +version. Custom checks retain an `originCheckId` across imports and later exports. Existing families and matching published versions are reused; missing versions are added, and recipient draft edits are preserved. Conflicting published content (or a referenced draft that differs) returns a readable 409 error. Unrelated checks diff --git a/docs/src/content/docs/user/user-guide.md b/docs/src/content/docs/user/user-guide.md index caebaa2a..3db36902 100644 --- a/docs/src/content/docs/user/user-guide.md +++ b/docs/src/content/docs/user/user-guide.md @@ -30,11 +30,11 @@ After the screener is created, you are automatically taken to the **Screener Das ### Sharing a screener -Save your changes, then return to **Screeners**, open the menu on the screener's card, and select **Export screener**. BDT downloads one `.bdt.json` file containing the saved benefits, configured check versions, parameters, aliases, custom-check rules and drafts, and form. Share this file with the recipient. +Save your changes, then return to **Screeners**, open the menu on the screener's card, and select **Export screener**. BDT downloads one `.bdt.json` file containing the saved benefits, configured check versions, parameters, aliases, the rules of each custom-check version it uses, and form. Unpublished edits to your custom checks are not included. Share this file with the recipient. To open a shared screener, select **Import screener** on the **Screeners** page, choose the JSON file (up to 10 MB), and review the prefilled **Screener name**. If that name already exists in your account, choose a different name before selecting **Import screener** in the dialog. Screener names are unique within your account, ignoring case and surrounding spaces. BDT opens a new editable draft owned by your account. You can preview, edit, and publish it with its own public URL. -Custom checks keep their original identity when shared. Import reuses checks already in your account, keeps your draft edits, and adds any missing published versions. If the same check version contains different rules or parameter definitions, BDT rejects the import and explains the conflict. A screener that directly uses a changed draft is also rejected; publish and select a version before sharing it. Checks with a different identity remain separate even if their names match; a module with an `imported` suffix resolves those name collisions. The screener keeps the check versions used by the sender. Library checks use your BDT server's library and must be available there; a file with missing checks is rejected before anything is imported. +Custom checks keep their original identity when shared. Import reuses checks already in your account, keeps your draft edits, and adds any missing published versions. A check that is new to your account gets an editable draft based on the latest version the screener uses. If the same check version contains different rules or parameter definitions, BDT rejects the import and explains the conflict. A screener that directly uses a changed draft is also rejected; publish and select a version before sharing it. Checks with a different identity remain separate even if their names match; a module with an `imported` suffix resolves those name collisions. The screener keeps the check versions used by the sender. Library checks use your BDT server's library and must be available there; a file with missing checks is rejected before anything is imported. --- diff --git a/e2e/tests/exampleScreenerTransfer.spec.ts b/e2e/tests/exampleScreenerTransfer.spec.ts index 3c8a4210..f4e513d3 100644 --- a/e2e/tests/exampleScreenerTransfer.spec.ts +++ b/e2e/tests/exampleScreenerTransfer.spec.ts @@ -164,10 +164,18 @@ test("the bundled example can be chosen, exported, and imported with its interna expect(response.ok(), await response.text()).toBe(true); return response.json(); }; - const originalIds = exported.customChecks - .filter((check: any) => check.id.startsWith("W-")) - .map((check: any) => check.id) - .sort(); + // Only the versions the screener uses are shared, never the author's drafts. + expect( + exported.customChecks.every((check: any) => check.id.startsWith("P-")), + ).toBe(true); + const originalIds = [ + ...new Set( + exported.customChecks.map( + (check: any) => + `W-${check.id.slice(2, -(check.version.length + 1))}`, + ), + ), + ].sort(); expect(originalIds).toHaveLength(2); expect( (await readWorking(account.idToken)).map((check: any) => check.id).sort(), From 0ce33dc512fb3304d12a20395732dd34f6f07d17 Mon Sep 17 00:00:00 2001 From: Preston Cabe Date: Mon, 5 Oct 2026 21:32:07 -0400 Subject: [PATCH 07/14] Fail the export when the form can't be read Export read the form through getFormSchemaFromStorage, which logs and returns null on any storage error, so a transient failure produced a "complete" file with no form. Export now uses a read that distinguishes a screener with no saved form from a failed read and lets the failure reach the caller. Export also now reads the screener document once: it authorizes with the metadata and reads the form directly instead of fetching the screener a second time. Co-Authored-By: Claude Opus 5.5 --- .../persistence/GoogleStorageService.java | 27 +++++++++---------- .../builder/persistence/StorageService.java | 3 +++ .../service/ScreenerTransferService.java | 9 ++++--- .../service/ScreenerTransferServiceTest.java | 23 ++++++++++++---- 4 files changed, 38 insertions(+), 24 deletions(-) diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/GoogleStorageService.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/GoogleStorageService.java index 7b564fce..2f1c7995 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/GoogleStorageService.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/GoogleStorageService.java @@ -153,27 +153,24 @@ public String getCheckDmnModelPath(String checkId){ @Override public Map getFormSchemaFromStorage(String filePath) { try { - BlobId blobId = BlobId.of(bucketName, filePath); - Blob blob = storage.get(blobId); - - if (blob == null || !blob.exists()) { - return null; - } - - byte[] content = blob.getContent(); - - ObjectMapper mapper = new ObjectMapper(); - Map formSchema = mapper.readValue(new ByteArrayInputStream(content), new TypeReference>() { - }); - - return formSchema; - + return readFormSchema(filePath).orElse(null); } catch (Exception e){ Log.error("Error fetching form model from firebase storage: ", e); return null; } } + @Override + public Optional> readFormSchema(String filePath) throws Exception { + Blob blob = storage.get(BlobId.of(bucketName, filePath)); + if (blob == null || !blob.exists()) { + return Optional.empty(); + } + ObjectMapper mapper = new ObjectMapper(); + return Optional.of(mapper.readValue(new ByteArrayInputStream(blob.getContent()), + new TypeReference>() {})); + } + @Override public void updatePublishedFormSchemaArtifact(String screenerId, String publishedScreenerId) throws Exception { try { diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/StorageService.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/StorageService.java index b48a24bd..0d09ce75 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/StorageService.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/persistence/StorageService.java @@ -29,5 +29,8 @@ public interface StorageService { Map getFormSchemaFromStorage(String filePath); + /* Empty when no form has been saved. Unlike getFormSchemaFromStorage, read failures are thrown. */ + Optional> readFormSchema(String filePath) throws Exception; + void updatePublishedFormSchemaArtifact(String screenerId, String publishedScreenerId) throws Exception; } diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java index 194270b2..5fd09606 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java @@ -32,9 +32,10 @@ public ScreenerTransferService(ScreenerRepository screeners, EligibilityCheckRep public ScreenerTransfer exportScreener(String owner, String id) throws Exception { // Authorize before fetching the form, benefits, or check models. - Screener metadata = screeners.getWorkingScreenerMetaDataOnly(id).orElseThrow(NotFoundException::new); - if (!owner.equals(metadata.getOwnerId())) throw new ForbiddenException(); - Screener screener = screeners.getWorkingScreener(id).orElseThrow(NotFoundException::new); + Screener screener = screeners.getWorkingScreenerMetaDataOnly(id).orElseThrow(NotFoundException::new); + if (!owner.equals(screener.getOwnerId())) throw new ForbiddenException(); + // A failed read must fail the export rather than silently drop the form. + Map formSchema = storage.readFormSchema(storage.getScreenerWorkingFormSchemaPath(id)).orElse(null); List benefits = new ArrayList<>(); Map byId = new HashMap<>(); for (Benefit benefit : screeners.getBenefitsInScreener(screener)) byId.put(benefit.getId(), benefit); @@ -63,7 +64,7 @@ public ScreenerTransfer exportScreener(String owner, String id) throws Exception } } return new ScreenerTransfer(ScreenerTransfer.FORMAT, ScreenerTransfer.VERSION, - screener.getScreenerName(), mapper.valueToTree(screener.getFormSchema()), + screener.getScreenerName(), mapper.valueToTree(formSchema), benefits, new ArrayList<>(customChecks.values())); } diff --git a/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java index 8fd3100f..e24d52d1 100644 --- a/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java +++ b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java @@ -70,7 +70,8 @@ void setup() throws Exception { source.setBenefits(List.of(new BenefitDetail("benefit-a", "First", "A description"), new BenefitDetail("benefit-b", "Second", "Another description"))); when(screeners.getWorkingScreenerMetaDataOnly(source.getId())).thenReturn(Optional.of(source)); - when(screeners.getWorkingScreener(source.getId())).thenReturn(Optional.of(source)); + when(storage.readFormSchema("form/working/" + source.getId() + ".json")) + .thenReturn(Optional.of(source.getFormSchema())); when(screeners.getBenefitsInScreener(source)).thenReturn(benefits); } @@ -151,7 +152,6 @@ void repeatImportsReuseVersionsAndKeepEditedDraftsEvenAfterReExport() throws Exc // Re-export from the recipient, then bring that file back to the original account. // The origin remains stable even though local family IDs and the draft's name changed. when(screeners.getWorkingScreenerMetaDataOnly(two.getId())).thenReturn(Optional.of(two)); - when(screeners.getWorkingScreener(two.getId())).thenReturn(Optional.of(two)); when(screeners.getBenefitsInScreener(two)).thenReturn(importedBenefits.getAllValues()); ScreenerTransfer reexported = service.exportScreener("recipient", two.getId()); assertTrue(reexported.customChecks().stream().allMatch(c -> "example".equals(c.getOriginCheckId()))); @@ -293,7 +293,6 @@ void oldExportsWithoutOriginIdentityRemainImportableAndReusable() throws Excepti var importedBenefits = ArgumentCaptor.forClass(Benefit.class); verify(screeners, times(2)).saveNewCustomBenefit(eq(legacyCopy.getId()), importedBenefits.capture()); when(screeners.getWorkingScreenerMetaDataOnly(legacyCopy.getId())).thenReturn(Optional.of(legacyCopy)); - when(screeners.getWorkingScreener(legacyCopy.getId())).thenReturn(Optional.of(legacyCopy)); when(screeners.getBenefitsInScreener(legacyCopy)).thenReturn(importedBenefits.getAllValues()); doReturn(List.of(draft, check("P-family-1.0.0", "1.0.0"), check("P-family-2.0.0", "2.0.0"))) .when(checks).getCustomChecksForImport("sender"); @@ -430,10 +429,23 @@ void exportFollowsTheEditorsBenefitListWhenStoredBenefitsDisagree() throws Excep assertEquals(1, export.customChecks().size()); } + @Test + void unreadableFormFailsTheExportInsteadOfDroppingTheForm() throws Exception { + when(storage.readFormSchema(anyString())).thenThrow(new Exception("Storage unavailable")); + assertThrows(Exception.class, () -> service.exportScreener("sender", source.getId())); + } + + @Test + void screenerWithoutASavedFormExportsWithoutOne() throws Exception { + when(storage.readFormSchema(anyString())).thenReturn(Optional.empty()); + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + assertTrue(export.formSchema() == null || export.formSchema().isNull()); + } + @Test void refusesExportsForOtherUsersBeforeReadingArtifacts() throws Exception { assertThrows(ForbiddenException.class, () -> service.exportScreener("recipient", source.getId())); - verify(screeners, never()).getWorkingScreener(anyString()); + verify(storage, never()).readFormSchema(anyString()); verify(screeners, never()).getBenefitsInScreener(any()); } @@ -513,7 +525,8 @@ void bundledExampleCanBeExportedAndImportedWithItsInternalLibraryCheck() throws exampleBenefits.add(readResource(path.asText(), Benefit.class)); } when(screeners.getWorkingScreenerMetaDataOnly(example.getId())).thenReturn(Optional.of(example)); - when(screeners.getWorkingScreener(example.getId())).thenReturn(Optional.of(example)); + when(storage.readFormSchema("form/working/" + example.getId() + ".json")) + .thenReturn(Optional.of(example.getFormSchema())); when(screeners.getBenefitsInScreener(example)).thenReturn(exampleBenefits); for (String group : List.of("workingCustomChecks", "publishedCustomChecks")) { for (var path : manifest.path(group)) { From 17b1387bc9ad1bc257e61059e7babbd240361970 Mon Sep 17 00:00:00 2001 From: Preston Cabe Date: Mon, 5 Oct 2026 21:33:13 -0400 Subject: [PATCH 08/14] Validate imported DMN models and derive their inputs Import only checked that each model parsed and had a decision named after the check, and trusted the file's inputDefinition. A hand-edited or corrupted file could create check versions that fail at evaluation, or ask the form for the wrong inputs. Import now compiles each model as publishing does and replaces the file's inputDefinition with the one derived from the model. Each distinct model is compiled once, after the cheaper checks of the file and the screener name. Reuse no longer compares input definitions: they are derived from the model, which is already compared. Co-Authored-By: Claude Opus 5.5 --- .../service/ScreenerTransferService.java | 35 +++++++++++++++---- .../service/ScreenerTransferServiceTest.java | 32 +++++++++++++++-- builder-frontend/README.md | 2 ++ 3 files changed, 60 insertions(+), 9 deletions(-) diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java index 5fd09606..5540d316 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java @@ -19,15 +19,17 @@ public class ScreenerTransferService { private final EligibilityCheckRepository checks; private final StorageService storage; private final LibraryApiService library; + private final DmnService dmn; private final ObjectMapper mapper = new ObjectMapper(); @Inject public ScreenerTransferService(ScreenerRepository screeners, EligibilityCheckRepository checks, - StorageService storage, LibraryApiService library) { + StorageService storage, LibraryApiService library, DmnService dmn) { this.screeners = screeners; this.checks = checks; this.storage = storage; this.library = library; + this.dmn = dmn; } public ScreenerTransfer exportScreener(String owner, String id) throws Exception { @@ -101,6 +103,7 @@ public Screener importScreener(String owner, ScreenerTransfer transfer) throws E name.equalsIgnoreCase(existing.getScreenerName() == null ? "" : existing.getScreenerName().strip()))) { throw new DuplicateScreenerNameException(); } + validateModels(originals.values()); Map libraryChecks = new HashMap<>(); for (Benefit benefit : transfer.benefits()) { for (CheckConfig config : configs(benefit)) { @@ -280,7 +283,6 @@ private void requireSameContent(EligibilityCheck incoming, EligibilityCheck exis try { same = Objects.equals(incoming.getName(), existing.getName()) && Objects.equals(incoming.getVersion(), existing.getVersion()) - && Objects.equals(normalizeInputDefinition(incoming), normalizeInputDefinition(existing)) && mapper.valueToTree(incoming.getParameterDefinitions() == null ? List.of() : incoming.getParameterDefinitions()) .equals(mapper.valueToTree(existing.getParameterDefinitions() == null ? List.of() : existing.getParameterDefinitions())) && !blank(existing.getDmnModel()) @@ -295,11 +297,6 @@ private void requireSameContent(EligibilityCheck incoming, EligibilityCheck exis + " already exists with different rules or parameter definitions. The screener was not imported."); } - private com.fasterxml.jackson.databind.JsonNode normalizeInputDefinition(EligibilityCheck check) { - var definition = check.getInputDefinition(); - return definition == null || definition.isNull() ? null : definition; - } - private void saveCheck(EligibilityCheck check, boolean published, Deque cleanup) throws Exception { String dmn = check.getDmnModel(); EligibilityCheck metadata = copy(check, EligibilityCheck.class); @@ -414,6 +411,30 @@ private Map validate(ScreenerTransfer transfer) { return byId; } + /* Compiles each model as publishing does and replaces the file's input definition with the one derived + from it, so a check whose model cannot run, or whose inputs disagree with it, is never created. */ + private void validateModels(Collection imported) { + Map derived = new HashMap<>(); + for (EligibilityCheck check : imported) { + // Versions of one check often share a model, so compile each distinct model once. + String key = check.getName() + "\u0000" + check.getDmnModel(); + if (!derived.containsKey(key)) { + List errors; + try { + errors = dmn.validateDmnXml(check.getDmnModel(), Map.of(), check.getName(), check.getName()); + if (errors.isEmpty()) + derived.put(key, dmn.extractInputSchema(check.getDmnModel(), Map.of(), check.getName())); + } catch (Exception failure) { + errors = List.of("the model could not be compiled."); + } + if (!errors.isEmpty()) + throw new BadRequestException("Invalid DMN model for check " + check.getName() + ": " + + errors.getFirst()); + } + check.setInputDefinition(derived.get(key)); + } + } + private List configs(Benefit benefit) { return benefit.getChecks() == null ? List.of() : benefit.getChecks(); } diff --git a/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java index e24d52d1..2d08ff01 100644 --- a/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java +++ b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java @@ -22,11 +22,14 @@ class ScreenerTransferServiceTest { private final EligibilityCheckRepository checks = mock(EligibilityCheckRepositoryImpl.class, CALLS_REAL_METHODS); private final StorageService storage = mock(StorageService.class); private final LibraryApiService library = mock(LibraryApiService.class); - private final ScreenerTransferService service = new ScreenerTransferService(screeners, checks, storage, library); + private final DmnService dmn = mock(DmnService.class); + private final ScreenerTransferService service = new ScreenerTransferService(screeners, checks, storage, library, dmn); private final ObjectMapper mapper = new ObjectMapper(); private Screener source; private List benefits; private EligibilityCheck draft; + private final com.fasterxml.jackson.databind.JsonNode derivedInputs = + new ObjectMapper().createObjectNode().put("type", "object"); @BeforeEach void setup() throws Exception { @@ -43,6 +46,8 @@ void setup() throws Exception { when(storage.getCheckDmnModelPath(anyString())).thenAnswer(i -> "check/" + i.getArgument(0) + ".dmn"); when(storage.getScreenerWorkingFormSchemaPath(anyString())).thenAnswer(i -> "form/working/" + i.getArgument(0) + ".json"); when(screeners.saveNewWorkingScreener(any())).thenAnswer(i -> i.getArgument(0).getId()); + when(dmn.validateDmnXml(anyString(), anyMap(), anyString(), anyString())).thenReturn(List.of()); + when(dmn.extractInputSchema(anyString(), anyMap(), anyString())).thenReturn(derivedInputs); source = Screener.create("sender", "Housing screener", null); source.setId("source-screener"); source.setPublishedScreenerId("public-link"); @@ -409,6 +414,29 @@ void missingModelIsRejectedBeforeAnyWrites() throws Exception { verify(storage, never()).writeStringToStorage(anyString(), anyString(), anyString()); } + @Test + void modelThatDoesNotCompileIsRejectedBeforeAnyWrites() throws Exception { + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + when(dmn.validateDmnXml(anyString(), anyMap(), anyString(), anyString())) + .thenReturn(List.of("The Result DataType of Decision 'Income' must be of type 'boolean'.")); + var failure = assertThrows(BadRequestException.class, () -> service.importScreener("recipient", export)); + assertTrue(failure.getMessage().contains("must be of type 'boolean'")); + verify(checks, never()).reserveCheckName(anyString(), anyString(), anyString(), anyString()); + verify(storage, never()).writeStringToStorage(anyString(), anyString(), anyString()); + } + + @Test + void inputDefinitionsAreDerivedFromTheModelRatherThanTakenFromTheFile() throws Exception { + ScreenerTransfer export = service.exportScreener("sender", source.getId()); + export.customChecks().forEach(c -> c.setInputDefinition(mapper.createObjectNode().put("forged", true))); + service.importScreener("recipient", export); + var saved = ArgumentCaptor.forClass(EligibilityCheck.class); + verify(checks, times(2)).saveNewPublishedCustomCheck(saved.capture()); + assertTrue(saved.getAllValues().stream().allMatch(c -> derivedInputs.equals(c.getInputDefinition()))); + // Both versions share one model, so it is compiled once. + verify(dmn).validateDmnXml(anyString(), anyMap(), anyString(), anyString()); + } + @Test void rejectsUnsupportedFormatAndUnresolvedReferences() throws Exception { assertThrows(BadRequestException.class, () -> service.importScreener("recipient", @@ -547,7 +575,7 @@ void bundledExampleCanBeExportedAndImportedWithItsInternalLibraryCheck() throws actualLibrary.loadBenefitsMetadata(mapper.writeValueAsString(exampleBenefits)); String internalId = "L-internal-sctf-age-requirement-0.9.0"; assertTrue(actualLibrary.getById(internalId).isEmpty()); - ScreenerTransferService transferService = new ScreenerTransferService(screeners, checks, storage, actualLibrary); + ScreenerTransferService transferService = new ScreenerTransferService(screeners, checks, storage, actualLibrary, dmn); var exported = transferService.exportScreener("sender", example.getId()); var imported = transferService.importScreener("recipient", mapper.readValue(mapper.writeValueAsString(exported), ScreenerTransfer.class)); diff --git a/builder-frontend/README.md b/builder-frontend/README.md index ae4724d6..9ea30a65 100644 --- a/builder-frontend/README.md +++ b/builder-frontend/README.md @@ -66,4 +66,6 @@ and recipient draft edits are preserved. Conflicting published content (or a referenced draft that differs) returns a readable 409 error. Unrelated checks with the same name remain separate and receive an `imported` module suffix. Older exports without `originCheckId` use their original working family ID. +Imported DMN models must compile as they would on publish, and each check's input +definition is derived from its model rather than taken from the file. Library check references are resolved against the receiving server's library. From 4e14adf36c196e62be96104ed9e269545ec6b25f Mon Sep 17 00:00:00 2001 From: Preston Cabe Date: Mon, 5 Oct 2026 21:33:51 -0400 Subject: [PATCH 09/14] Return rejected imports with their HTTP status The import endpoint turned every WebApplicationException other than a bad request into a generic "please try again" 500, so ownership and not-found rejections looked retryable. It now returns their response, as export already does. Planning an import also threw when one of the account's stored checks had an ID that did not match its version. Such checks are now left out of matching and logged, instead of failing every import. Co-Authored-By: Claude Opus 5.5 --- .../controller/ScreenerTransferResource.java | 2 ++ .../builder/service/ScreenerTransferService.java | 16 +++++++++++++++- .../controller/ScreenerTransferResourceTest.java | 8 ++++++++ .../service/ScreenerTransferServiceTest.java | 9 +++++++++ 4 files changed, 34 insertions(+), 1 deletion(-) diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResource.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResource.java index 1a43a827..e04b8501 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResource.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResource.java @@ -46,6 +46,8 @@ public Response importScreener(@Context SecurityIdentity identity, ScreenerTrans return Response.status(409).entity(Map.of("error", conflict.getMessage())).build(); } catch (BadRequestException rejected) { return Response.status(Response.Status.BAD_REQUEST).entity(Map.of("error", rejected.getMessage())).build(); + } catch (WebApplicationException rejected) { + return rejected.getResponse(); } catch (Exception failure) { Log.error("Could not import screener", failure); return Response.serverError().entity(Map.of("error", "Could not import the screener. Please try again.")).build(); diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java index 5540d316..13704587 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java @@ -115,7 +115,7 @@ public Screener importScreener(String owner, ScreenerTransfer transfer) throws E } } // Decide every reuse/conflict before reserving names or writing any artifact. - List existing = checks.getCustomChecksForImport(owner); + List existing = matchableChecks(checks.getCustomChecksForImport(owner)); CheckImportPlan plan = planChecks(owner, originals, existing, transfer); Deque cleanup = new ArrayDeque<>(); try { @@ -198,6 +198,20 @@ public Screener importScreener(String owner, ScreenerTransfer transfer) throws E } } + /* A stored check whose ID does not match its version has no family to match, so import leaves it alone. */ + private List matchableChecks(List stored) { + List matchable = new ArrayList<>(); + for (EligibilityCheck check : stored) { + try { + checks.getWorkingId(check); + matchable.add(check); + } catch (IllegalArgumentException malformed) { + Log.warn("Ignoring custom check " + check.getId() + " while importing: " + malformed.getMessage()); + } + } + return matchable; + } + private String originId(EligibilityCheck check) { if (!blank(check.getOriginCheckId())) return check.getOriginCheckId(); if (!blank(check.getExampleSourceId())) return check.getExampleSourceId(); diff --git a/builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResourceTest.java b/builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResourceTest.java index c89c9fd3..22e54e37 100644 --- a/builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResourceTest.java +++ b/builder-api/src/test/java/org/codeforphilly/bdt/builder/controller/ScreenerTransferResourceTest.java @@ -65,6 +65,14 @@ void conflictingCheckReturnsAReadableConflict() throws Exception { } } + @Test + void rejectedImportKeepsItsStatusInsteadOfAskingForARetry() throws Exception { + when(service.importScreener("recipient", null)).thenThrow(new ForbiddenException()); + try (var response = resource.importScreener(identity, null)) { + assertEquals(403, response.getStatus()); + } + } + @Test void forbiddenExportReturnsForbidden() throws Exception { when(service.exportScreener("recipient", "other-screener")).thenThrow(new ForbiddenException()); diff --git a/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java index 2d08ff01..5b6b700a 100644 --- a/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java +++ b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java @@ -196,6 +196,15 @@ void checksInAnotherAccountAreNeverReused() throws Exception { assertEquals("income", saved.getValue().getModule()); } + @Test + void storedCheckWithAMalformedIdDoesNotBlockImport() throws Exception { + EligibilityCheck malformed = check("P-legacy", "1.0.0"); + malformed.setOwnerId("recipient"); + doReturn(List.of(malformed)).when(checks).getCustomChecksForImport("recipient"); + service.importScreener("recipient", service.exportScreener("sender", source.getId())); + verify(checks).saveNewWorkingCustomCheck(any()); + } + @Test void unrelatedCheckWithSameNameGetsItsOwnFamilyAndModule() throws Exception { EligibilityCheck unrelated = check("W-unrelated", "1.0.0"); From 6e4e755b232f287553fcafe73cef80d83215be8f Mon Sep 17 00:00:00 2001 From: Preston Cabe Date: Mon, 5 Oct 2026 21:34:58 -0400 Subject: [PATCH 10/14] Use the save-time name normalization in the import pre-check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The import's early duplicate-name check compared stripped names with equalsIgnoreCase, while saving compares strip().toLowerCase(Locale.ROOT). For names such as "İncome" and "income" the two disagree, so the early check could reject a name the save would accept, or the reverse, after all checks and benefits had already been written. Both now use Screener.normalizeName. Co-Authored-By: Claude Opus 5.5 --- .../bdt/builder/service/ScreenerTransferService.java | 3 ++- .../bdt/builder/service/ScreenerTransferServiceTest.java | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java index 13704587..25c4b142 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java @@ -99,8 +99,9 @@ public Screener importScreener(String owner, ScreenerTransfer transfer) throws E // Validate the complete file and resolve library references before any writes. Map originals = validate(transfer); String name = transfer.screenerName().strip(); + // Match the transactional check on save, so this early rejection never disagrees with it. if (screeners.getWorkingScreeners(owner).stream().anyMatch(existing -> - name.equalsIgnoreCase(existing.getScreenerName() == null ? "" : existing.getScreenerName().strip()))) { + Screener.normalizeName(name).equals(Screener.normalizeName(existing.getScreenerName())))) { throw new DuplicateScreenerNameException(); } validateModels(originals.values()); diff --git a/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java index 5b6b700a..17f79b24 100644 --- a/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java +++ b/builder-api/src/test/java/org/codeforphilly/bdt/builder/service/ScreenerTransferServiceTest.java @@ -413,6 +413,14 @@ void duplicateNameRejectsImportBeforeWritingArtifacts() throws Exception { assertEquals("Housing screener - Copy", service.importScreener("recipient", renamed).getScreenerName()); } + @Test + void nameCheckMatchesTheSavedNameNormalization() throws Exception { + // equalsIgnoreCase treats "İ" as "i", but the save-time check lowercases it to "i" plus a combining dot. + ScreenerTransfer export = withName(service.exportScreener("sender", source.getId()), "income"); + when(screeners.getWorkingScreeners("recipient")).thenReturn(List.of(Screener.create("recipient", "İncome", null))); + assertEquals("income", service.importScreener("recipient", export).getScreenerName()); + } + @Test void missingModelIsRejectedBeforeAnyWrites() throws Exception { ScreenerTransfer export = service.exportScreener("sender", source.getId()); From 5ae778dc2ce79dc77d00f65318e18b80fb59c0e1 Mon Sep 17 00:00:00 2001 From: Preston Cabe Date: Mon, 5 Oct 2026 21:36:21 -0400 Subject: [PATCH 11/14] Reuse the existing source-check and API-error helpers Screener transfer had its own copies of the rules for a configuration's source check ID and for library check IDs, and the frontend had a second server-error parser beside apiErrorFrom. Those rules now live once on CheckConfig, used by both example and transfer import, and the screener API calls use apiErrorFrom, which also carries the response status. Co-Authored-By: Claude Opus 5.5 --- .../bdt/builder/model/domain/CheckConfig.java | 9 +++++++++ .../service/ExampleScreenerImportService.java | 20 ++++--------------- .../service/ScreenerTransferService.java | 10 +++++----- builder-frontend/src/api/check.ts | 2 +- builder-frontend/src/api/screener.ts | 10 +++------- builder-frontend/src/api/screenerTransfer.ts | 19 +++--------------- 6 files changed, 25 insertions(+), 45 deletions(-) diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/model/domain/CheckConfig.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/model/domain/CheckConfig.java index 43517bbb..49e198a7 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/model/domain/CheckConfig.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/model/domain/CheckConfig.java @@ -28,6 +28,15 @@ public class CheckConfig { public CheckConfig() { } + /* The check this configuration evaluates. Older configurations kept it in checkId. */ + public String resolveSourceCheckId() { + return sourceCheckId != null && !sourceCheckId.isBlank() ? sourceCheckId : checkId; + } + + public static boolean isLibraryCheckId(String checkId) { + return checkId != null && checkId.startsWith("L"); + } + public CheckConfig( String checkId, String sourceCheckId, diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ExampleScreenerImportService.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ExampleScreenerImportService.java index 3aaf87a3..0545731b 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ExampleScreenerImportService.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ExampleScreenerImportService.java @@ -121,9 +121,9 @@ private Map importReferencedCustomChecks(SeedData seedData, continue; } for (CheckConfig checkConfig : benefit.getChecks()) { - String sourceCheckId = resolveSourceCheckId(checkConfig); + String sourceCheckId = checkConfig.resolveSourceCheckId(); if (sourceCheckId != null - && !isLibraryCheckId(sourceCheckId)) { + && !CheckConfig.isLibraryCheckId(sourceCheckId)) { referencedCustomCheckIds.add(sourceCheckId); } } @@ -340,9 +340,9 @@ private List remapCheckConfigs(List seedChecks, .convertValue(seedCheck, CheckConfig.class); importedCheck.setCheckId(UUID.randomUUID().toString()); - String sourceCheckId = resolveSourceCheckId(seedCheck); + String sourceCheckId = seedCheck.resolveSourceCheckId(); if (sourceCheckId != null) { - if (isLibraryCheckId(sourceCheckId)) { + if (CheckConfig.isLibraryCheckId(sourceCheckId)) { importedCheck.setSourceCheckId(sourceCheckId); } else { String remappedSourceCheckId = importedCustomCheckIds @@ -366,18 +366,6 @@ private EligibilityCheck cloneEligibilityCheck(EligibilityCheck seedCheck) { return objectMapper.convertValue(seedCheck, EligibilityCheck.class); } - private String resolveSourceCheckId(CheckConfig checkConfig) { - if (checkConfig.getSourceCheckId() != null - && !checkConfig.getSourceCheckId().isBlank()) { - return checkConfig.getSourceCheckId(); - } - return checkConfig.getCheckId(); - } - - private boolean isLibraryCheckId(String checkId) { - return checkId != null && checkId.startsWith("L"); - } - private SeedData loadSeedData(Manifest manifest) throws IOException { List screenerFiles = manifest.screeners(); diff --git a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java index 25c4b142..3c366fb1 100644 --- a/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java +++ b/builder-api/src/main/java/org/codeforphilly/bdt/builder/service/ScreenerTransferService.java @@ -59,7 +59,7 @@ public ScreenerTransfer exportScreener(String owner, String id) throws Exception benefit.setOwnerId(null); for (CheckConfig config : configs(benefit)) { String source = sourceId(config); - if (source.startsWith("L")) continue; + if (CheckConfig.isLibraryCheckId(source)) continue; // Only the versions the screener uses are shared, never the author's unpublished draft. if (customChecks.containsKey(source)) continue; customChecks.put(source, portableCheck(loadCheck(owner, source))); @@ -109,7 +109,7 @@ public Screener importScreener(String owner, ScreenerTransfer transfer) throws E for (Benefit benefit : transfer.benefits()) { for (CheckConfig config : configs(benefit)) { String source = sourceId(config); - if (source.startsWith("L") && !libraryChecks.containsKey(source)) { + if (CheckConfig.isLibraryCheckId(source) && !libraryChecks.containsKey(source)) { libraryChecks.put(source, library.getScreenerCheckById(source).orElseThrow(() -> new BadRequestException("Library check " + source + " is unavailable on this server."))); } @@ -159,7 +159,7 @@ public Screener importScreener(String owner, ScreenerTransfer transfer) throws E for (CheckConfig config : configs(benefit)) { String source = sourceId(config); config.setCheckId(UUID.randomUUID().toString()); - if (source.startsWith("L")) { + if (CheckConfig.isLibraryCheckId(source)) { // Use this server's endpoint, never a URL supplied by the file. config.setSourceCheckId(source); config.setEvaluationUrl(libraryChecks.get(source).getEvaluationUrl()); @@ -408,7 +408,7 @@ private Map validate(ScreenerTransfer transfer) { for (CheckConfig config : configs(benefit)) { if (config == null || blank(config.getCheckName())) throw new BadRequestException("Invalid configured check."); String source = sourceId(config); - if (source.startsWith("L")) continue; + if (CheckConfig.isLibraryCheckId(source)) continue; EligibilityCheck check = byId.get(source); if (check == null) throw new BadRequestException("Missing custom check " + source); if (!Objects.equals(config.getCheckName(), check.getName()) @@ -455,7 +455,7 @@ private List configs(Benefit benefit) { } private String sourceId(CheckConfig config) { - String id = blank(config.getSourceCheckId()) ? config.getCheckId() : config.getSourceCheckId(); + String id = config.resolveSourceCheckId(); if (blank(id)) throw new BadRequestException("A configured check is missing its source ID."); return id; } diff --git a/builder-frontend/src/api/check.ts b/builder-frontend/src/api/check.ts index 39c8ac12..61da6be5 100644 --- a/builder-frontend/src/api/check.ts +++ b/builder-frontend/src/api/check.ts @@ -22,7 +22,7 @@ export class ApiError extends Error { } // Prefer the server's error message, falling back to the status. -async function apiErrorFrom( +export async function apiErrorFrom( response: Response, fallback: string, ): Promise { diff --git a/builder-frontend/src/api/screener.ts b/builder-frontend/src/api/screener.ts index c8fd459a..80e59813 100644 --- a/builder-frontend/src/api/screener.ts +++ b/builder-frontend/src/api/screener.ts @@ -8,7 +8,7 @@ import type { ScreenerResult, } from "@/types"; -import { errorMessage } from "./screenerTransfer"; +import { apiErrorFrom } from "./check"; const apiUrl = env.apiUrl; @@ -53,9 +53,7 @@ export const createNewScreener = async (request: { const response = await authPost(url.toString(), request); if (!response.ok) { - throw new Error( - await errorMessage(response, "Could not create the screener."), - ); + throw await apiErrorFrom(response, "Could not create the screener"); } const data = await response.json(); return data; @@ -75,9 +73,7 @@ export const updateScreener = async ( const response = await authPatch(url.toString(), request); if (!response.ok) { - throw new Error( - await errorMessage(response, "Could not update the screener."), - ); + throw await apiErrorFrom(response, "Could not update the screener"); } } catch (error) { console.error("Error updating screener:", error); diff --git a/builder-frontend/src/api/screenerTransfer.ts b/builder-frontend/src/api/screenerTransfer.ts index e188f033..e94724f1 100644 --- a/builder-frontend/src/api/screenerTransfer.ts +++ b/builder-frontend/src/api/screenerTransfer.ts @@ -1,26 +1,15 @@ import { authGet, authPost } from "@/api/auth"; +import { apiErrorFrom } from "@/api/check"; import { env } from "@/config/environment"; export const MAX_SCREENER_FILE_BYTES = 10 * 1024 * 1024; -export async function errorMessage(response: Response, fallback: string) { - try { - const body = await response.json(); - if (typeof body.error === "string") return body.error; - } catch { - // Keep a readable fallback for non-JSON responses. - } - return fallback; -} - export async function exportScreener(screenerId: string): Promise { const response = await authGet( `${env.apiUrl}/screener/${encodeURIComponent(screenerId)}/export`, ); if (!response.ok) { - throw new Error( - await errorMessage(response, "Could not export the screener."), - ); + throw await apiErrorFrom(response, "Could not export the screener"); } return response.blob(); } @@ -58,9 +47,7 @@ export async function importScreener( if (name !== undefined) data.screenerName = name.trim(); const response = await authPost(`${env.apiUrl}/screener/import`, data); if (!response.ok) { - throw new Error( - await errorMessage(response, "Could not import the screener."), - ); + throw await apiErrorFrom(response, "Could not import the screener"); } return response.json(); } From b3e38e92ff92eba7109f3d7d3bb59f454bfa24fe Mon Sep 17 00:00:00 2001 From: Preston Cabe Date: Mon, 5 Oct 2026 21:37:28 -0400 Subject: [PATCH 12/14] Parse the import file only once The import dialog parsed the chosen file to prefill its name, then importScreener read and parsed the same file again on submit, for files of up to 10 MB. The dialog now keeps the parsed object and importScreener uploads it, applying the chosen name to a copy. Co-Authored-By: Claude Opus 5.5 --- .../src/api/screenerTransfer.test.ts | 29 ++++++++++--------- builder-frontend/src/api/screenerTransfer.ts | 9 +++--- .../homeScreen/ImportScreenerForm.test.tsx | 10 +++++-- .../homeScreen/ImportScreenerForm.tsx | 15 ++++++---- 4 files changed, 39 insertions(+), 24 deletions(-) diff --git a/builder-frontend/src/api/screenerTransfer.test.ts b/builder-frontend/src/api/screenerTransfer.test.ts index 738060bb..52d79fba 100644 --- a/builder-frontend/src/api/screenerTransfer.test.ts +++ b/builder-frontend/src/api/screenerTransfer.test.ts @@ -6,6 +6,7 @@ import { exportScreener, importScreener, MAX_SCREENER_FILE_BYTES, + readScreenerFile, } from "./screenerTransfer"; const file = (content: string) => @@ -34,7 +35,9 @@ describe("screener transfer API", () => { vi.mocked(authPost).mockResolvedValue( new Response(JSON.stringify({ id: "new-screener" }), { status: 201 }), ); - await expect(importScreener(file(JSON.stringify(data)))).resolves.toEqual({ + await expect( + importScreener(await readScreenerFile(file(JSON.stringify(data)))), + ).resolves.toEqual({ id: "new-screener", }); expect(authPost).toHaveBeenCalledWith( @@ -48,16 +51,14 @@ describe("screener transfer API", () => { size: MAX_SCREENER_FILE_BYTES + 1, text: vi.fn(), } as unknown as File; - await expect(importScreener(oversized)).rejects.toThrow("10 MB"); + await expect(readScreenerFile(oversized)).rejects.toThrow("10 MB"); expect(oversized.text).not.toHaveBeenCalled(); - expect(authPost).not.toHaveBeenCalled(); }); it("reports invalid JSON without uploading it", async () => { - await expect(importScreener(file("not JSON"))).rejects.toThrow( + await expect(readScreenerFile(file("not JSON"))).rejects.toThrow( "not valid JSON", ); - expect(authPost).not.toHaveBeenCalled(); }); it("shows the server's validation error", async () => { @@ -66,9 +67,9 @@ describe("screener transfer API", () => { status: 400, }), ); - await expect( - importScreener(file('{"screenerName":"Example"}')), - ).rejects.toThrow("Missing custom check"); + await expect(importScreener({ screenerName: "Example" })).rejects.toThrow( + "Missing custom check", + ); }); it("reports non-JSON export failures", async () => { @@ -81,18 +82,20 @@ describe("screener transfer API", () => { }); }); -it("uses the chosen name without changing the file", async () => { +it("uses the chosen name without changing the parsed file", async () => { vi.mocked(authPost).mockResolvedValue( new Response('{"id":"copy"}', { status: 201 }), ); - const original = file( - '{"screenerName":"Example","benefits":[],"customChecks":[]}', - ); + const original = { + screenerName: "Example", + benefits: [], + customChecks: [], + }; await importScreener(original, " Example - Copy "); expect(authPost).toHaveBeenLastCalledWith(expect.any(String), { screenerName: "Example - Copy", benefits: [], customChecks: [], }); - expect(JSON.parse(await original.text()).screenerName).toBe("Example"); + expect(original.screenerName).toBe("Example"); }); diff --git a/builder-frontend/src/api/screenerTransfer.ts b/builder-frontend/src/api/screenerTransfer.ts index e94724f1..05805611 100644 --- a/builder-frontend/src/api/screenerTransfer.ts +++ b/builder-frontend/src/api/screenerTransfer.ts @@ -39,13 +39,14 @@ export async function readScreenerFile( return data as Record; } +/* Uploads a file already parsed by readScreenerFile, optionally under a new name. */ export async function importScreener( - file: File, + data: Record, name?: string, ): Promise<{ id: string }> { - const data = await readScreenerFile(file); - if (name !== undefined) data.screenerName = name.trim(); - const response = await authPost(`${env.apiUrl}/screener/import`, data); + const body = + name === undefined ? data : { ...data, screenerName: name.trim() }; + const response = await authPost(`${env.apiUrl}/screener/import`, body); if (!response.ok) { throw await apiErrorFrom(response, "Could not import the screener"); } diff --git a/builder-frontend/src/components/homeScreen/ImportScreenerForm.test.tsx b/builder-frontend/src/components/homeScreen/ImportScreenerForm.test.tsx index dc78cac5..fa74b2d9 100644 --- a/builder-frontend/src/components/homeScreen/ImportScreenerForm.test.tsx +++ b/builder-frontend/src/components/homeScreen/ImportScreenerForm.test.tsx @@ -59,7 +59,13 @@ it("requires a file and opens the imported screener", async () => { await vi.waitFor(() => expect(navigate).toHaveBeenCalledWith("/screeners/new-screener"), ); - expect(importScreener).toHaveBeenCalledWith(file, "Example"); + // The file is parsed once, when chosen, and that result is uploaded. + expect(readScreenerFile).toHaveBeenCalledOnce(); + expect(readScreenerFile).toHaveBeenCalledWith(file); + expect(importScreener).toHaveBeenCalledWith( + { screenerName: "Example" }, + "Example", + ); }); it("shows a failure and allows retrying the same file", async () => { @@ -127,7 +133,7 @@ it("requires a different name on collision, treating case and spaces alike", asy expect(navigate).toHaveBeenCalledWith("/screeners/copy"), ); expect(importScreener).toHaveBeenCalledWith( - expect.any(File), + { screenerName: "Example" }, "Example - Copy", ); }); diff --git a/builder-frontend/src/components/homeScreen/ImportScreenerForm.tsx b/builder-frontend/src/components/homeScreen/ImportScreenerForm.tsx index ed9415e1..4307055d 100644 --- a/builder-frontend/src/components/homeScreen/ImportScreenerForm.tsx +++ b/builder-frontend/src/components/homeScreen/ImportScreenerForm.tsx @@ -8,6 +8,7 @@ export default function ImportScreenerForm(props: { }) { const navigate = useNavigate(); const [file, setFile] = createSignal(); + const [data, setData] = createSignal>(); const [loading, setLoading] = createSignal(false); const [error, setError] = createSignal(""); const [name, setName] = createSignal(""); @@ -23,12 +24,12 @@ export default function ImportScreenerForm(props: {
{ event.preventDefault(); - if (!file() || !name().trim() || reading() || duplicate() || loading()) + if (!data() || !name().trim() || reading() || duplicate() || loading()) return; setError(""); setLoading(true); try { - const screener = await importScreener(file()!, name()); + const screener = await importScreener(data()!, name()); navigate(`/screeners/${screener.id}`); } catch (failure) { setError( @@ -58,6 +59,7 @@ export default function ImportScreenerForm(props: { onChange={async (event) => { const selected = event.currentTarget.files?.[0]; setFile(selected); + setData(undefined); setName(""); setError(""); if (!selected) { @@ -66,8 +68,11 @@ export default function ImportScreenerForm(props: { } setReading(true); try { - const data = await readScreenerFile(selected); - if (file() === selected) setName(data.screenerName as string); + const parsed = await readScreenerFile(selected); + if (file() === selected) { + setData(parsed); + setName(parsed.screenerName as string); + } } catch (failure) { if (file() === selected) setError( @@ -130,7 +135,7 @@ export default function ImportScreenerForm(props: {