From d236a7aca4eff1993eea48321aba002b651e4241 Mon Sep 17 00:00:00 2001 From: Michael Hoennig Date: Wed, 27 Mar 2024 18:17:10 +0100 Subject: [PATCH] fix fixme's --- .../debitor/HsOfficeDebitorController.java | 2 + ...OfficeDebitorControllerAcceptanceTest.java | 96 +++++++++++-------- ...fficeDebitorRepositoryIntegrationTest.java | 47 ++++----- .../hs/office/migration/ImportOfficeData.java | 5 - 4 files changed, 81 insertions(+), 69 deletions(-) diff --git a/src/main/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorController.java b/src/main/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorController.java index d937ef50..5455b99b 100644 --- a/src/main/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorController.java +++ b/src/main/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorController.java @@ -9,6 +9,7 @@ import net.hostsharing.hsadminng.hs.office.relation.HsOfficeRelationEntity; import net.hostsharing.hsadminng.hs.office.relation.HsOfficeRelationRepository; import net.hostsharing.hsadminng.mapper.Mapper; import org.apache.commons.lang3.Validate; +import org.hibernate.Hibernate; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.http.ResponseEntity; import org.springframework.transaction.annotation.Transactional; @@ -150,6 +151,7 @@ public class HsOfficeDebitorController implements HsOfficeDebitorsApi { new HsOfficeDebitorEntityPatcher(em, current).apply(body); final var saved = debitorRepo.save(current); + Hibernate.initialize(saved); final var mapped = mapper.map(saved, HsOfficeDebitorResource.class); return ResponseEntity.ok(mapped); } diff --git a/src/test/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorControllerAcceptanceTest.java b/src/test/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorControllerAcceptanceTest.java index 6819f4b9..975ad961 100644 --- a/src/test/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorControllerAcceptanceTest.java +++ b/src/test/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorControllerAcceptanceTest.java @@ -545,7 +545,7 @@ class HsOfficeDebitorControllerAcceptanceTest extends ContextBasedTestWithCleanu class PatchDebitor { @Test - void globalAdmin_withoutAssumedRole_canPatchAllPropertiesOfArbitraryDebitor() { + void globalAdmin_withoutAssumedRole_canPatchArbitraryDebitor() { context.define("superuser-alex@hostsharing.net"); final var givenDebitor = givenSomeTemporaryDebitor(); @@ -567,17 +567,48 @@ class HsOfficeDebitorControllerAcceptanceTest extends ContextBasedTestWithCleanu .port(port) .when() .patch("http://localhost/api/hs/office/debitors/" + givenDebitor.getUuid()) - .then().assertThat() + .then().log().all().assertThat() .statusCode(200) .contentType(ContentType.JSON) - .body("uuid", isUuidValid()) - .body("vatId", is("VAT222222")) - .body("vatCountryCode", is("AA")) - .body("vatBusiness", is(true)) - .body("defaultPrefix", is("for")) - // FIXME .body("billingContact.label", is(givenContact.getLabel())) - // FIXME .body("partner.partnerRel.holder.tradeName", is(givenDebitor.getPartner().getPartnerRel().getHolder().getTradeName())) - ; + .body("", lenientlyEquals(""" + { + "debitorRel": { + "anchor": { "tradeName": "Fourth eG" }, + "holder": { "tradeName": "Fourth eG" }, + "type": "DEBITOR", + "mark": null, + "contact": { "label": "fourth contact" } + }, + "debitorNumber": 10004${debitorNumberSuffix}, + "debitorNumberSuffix": ${debitorNumberSuffix}, + "partner": { + "partnerNumber": 10004, + "partnerRel": { + "anchor": { "tradeName": "Hostsharing eG" }, + "holder": { "tradeName": "Fourth eG" }, + "type": "PARTNER", + "mark": null, + "contact": { "label": "fourth contact" } + }, + "details": { + "registrationOffice": "Hamburg", + "registrationNumber": "RegNo123456789", + "birthName": null, + "birthPlace": null, + "birthday": null, + "dateOfDeath": null + } + }, + "billable": true, + "vatId": "VAT222222", + "vatCountryCode": "AA", + "vatBusiness": true, + "vatReverseCharge": false, + "defaultPrefix": "for" + } + """ + .replace("${debitorNumberSuffix}", givenDebitor.getDebitorNumberSuffix().toString())) + ); // @formatter:on // finally, the debitor is actually updated @@ -595,48 +626,31 @@ class HsOfficeDebitorControllerAcceptanceTest extends ContextBasedTestWithCleanu } @Test - void globalAdmin_withoutAssumedRole_canPatchPartialPropertiesOfArbitraryDebitor() { + void theContactOwner_canNotPatchARelatedDebitor() { context.define("superuser-alex@hostsharing.net"); final var givenDebitor = givenSomeTemporaryDebitor(); - final var newBillingContact = contactRepo.findContactByOptionalLabelLike("sixth").get(0); - final var location = RestAssured // @formatter:off - .given() + // @formatter:on + RestAssured // @formatter:off + .given() .header("current-user", "superuser-alex@hostsharing.net") + .header("assumed-roles", "hs_office_contact#fourthcontact.admin") .contentType(ContentType.JSON) .body(""" - { - "billingContactUuid": "%s", - "vatId": "VAT999999" - } - """.formatted(newBillingContact.getUuid())) + { + "vatId": "VAT999999" + } + """) .port(port) - .when() + .when() .patch("http://localhost/api/hs/office/debitors/" + givenDebitor.getUuid()) - .then().assertThat() - .statusCode(200) - .contentType(ContentType.JSON) - .body("uuid", isUuidValid()) -// FIXME .body("billingContact.label", is("sixth contact")) - .body("vatId", is("VAT999999")) - .body("vatCountryCode", is(givenDebitor.getVatCountryCode())) - .body("vatBusiness", is(givenDebitor.isVatBusiness())); - // @formatter:on + .then().log().all().assertThat() + .statusCode(403) + .body("message", containsString("ERROR: [403] Subject")) + .body("message", containsString("is not allowed to update hs_office_debitor uuid ")); - // finally, the debitor is actually updated - assertThat(debitorRepo.findByUuid(givenDebitor.getUuid())).isPresent().get() - .matches(partner -> { - assertThat(partner.getDebitorRel().getHolder().getTradeName()) - .isEqualTo(givenDebitor.getDebitorRel().getHolder().getTradeName()); - // FIXME assertThat(partner.getDebitorRel().getContact().getLabel()).isEqualTo("sixth contact"); - assertThat(partner.getVatId()).isEqualTo("VAT999999"); - assertThat(partner.getVatCountryCode()).isEqualTo(givenDebitor.getVatCountryCode()); - assertThat(partner.isVatBusiness()).isEqualTo(givenDebitor.isVatBusiness()); - return true; - }); } - } @Nested diff --git a/src/test/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorRepositoryIntegrationTest.java b/src/test/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorRepositoryIntegrationTest.java index d40aaaee..5f53df24 100644 --- a/src/test/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorRepositoryIntegrationTest.java +++ b/src/test/java/net/hostsharing/hsadminng/hs/office/debitor/HsOfficeDebitorRepositoryIntegrationTest.java @@ -13,6 +13,7 @@ import net.hostsharing.hsadminng.rbac.rbacgrant.RbacGrantsDiagramService; import net.hostsharing.hsadminng.rbac.rbacrole.RawRbacRoleRepository; import net.hostsharing.test.Array; import net.hostsharing.test.JpaAttempt; +import org.hibernate.Hibernate; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -316,7 +317,7 @@ class HsOfficeDebitorRepositoryIntegrationTest extends ContextBasedTestWithClean assertThatDebitorIsVisibleForUserWithRole( givenDebitor, - "hs_office_relation#FourtheG-with-DEBITOR-FourtheG.admin"); + "hs_office_relation#FourtheG-with-DEBITOR-FourtheG.admin", true); final var givenNewPartnerPerson = one(personRepo.findPersonByOptionalNameLike("First")); final var givenNewBillingPerson = one(personRepo.findPersonByOptionalNameLike("Firby")); final var givenNewContact = one(contactRepo.findContactByOptionalLabelLike("sixth contact")); @@ -345,7 +346,7 @@ class HsOfficeDebitorRepositoryIntegrationTest extends ContextBasedTestWithClean result.assertSuccessful(); assertThatDebitorIsVisibleForUserWithRole( result.returnedValue(), - "global#global.admin"); + "global#global.admin", true); // ... partner role was reassigned: assertThatDebitorIsNotVisibleForUserWithRole( @@ -353,7 +354,7 @@ class HsOfficeDebitorRepositoryIntegrationTest extends ContextBasedTestWithClean "hs_office_relation#FourtheG-with-DEBITOR-FourtheG.admin"); assertThatDebitorIsVisibleForUserWithRole( result.returnedValue(), - "hs_office_relation#FirstGmbH-with-DEBITOR-FirbySusan.agent"); + "hs_office_relation#FirstGmbH-with-DEBITOR-FirbySusan.agent", true); // ... contact role was reassigned: assertThatDebitorIsNotVisibleForUserWithRole( @@ -361,7 +362,7 @@ class HsOfficeDebitorRepositoryIntegrationTest extends ContextBasedTestWithClean "hs_office_contact#fifthcontact.admin"); assertThatDebitorIsVisibleForUserWithRole( result.returnedValue(), - "hs_office_contact#sixthcontact.admin"); + "hs_office_contact#sixthcontact.admin", false); // ... bank-account role was reassigned: assertThatDebitorIsNotVisibleForUserWithRole( @@ -369,7 +370,7 @@ class HsOfficeDebitorRepositoryIntegrationTest extends ContextBasedTestWithClean "hs_office_bankaccount#DE02200505501015871393.admin"); assertThatDebitorIsVisibleForUserWithRole( result.returnedValue(), - "hs_office_bankaccount#DE02120300000000202051.admin"); + "hs_office_bankaccount#DE02120300000000202051.admin", true); } @Test @@ -379,8 +380,8 @@ class HsOfficeDebitorRepositoryIntegrationTest extends ContextBasedTestWithClean final var givenDebitor = givenSomeTemporaryDebitor("Fourth", "fifth contact", null, "fig"); assertThatDebitorIsVisibleForUserWithRole( givenDebitor, - "hs_office_relation#FourtheG-with-DEBITOR-FourtheG.admin"); - assertThatDebitorActuallyInDatabase(givenDebitor); + "hs_office_relation#FourtheG-with-DEBITOR-FourtheG.admin", true); + assertThatDebitorActuallyInDatabase(givenDebitor, true); final var givenNewBankAccount = one(bankAccountRepo.findByOptionalHolderLike("first")); // when @@ -394,12 +395,12 @@ class HsOfficeDebitorRepositoryIntegrationTest extends ContextBasedTestWithClean result.assertSuccessful(); assertThatDebitorIsVisibleForUserWithRole( result.returnedValue(), - "global#global.admin"); + "global#global.admin", true); // ... bank-account role was assigned: assertThatDebitorIsVisibleForUserWithRole( result.returnedValue(), - "hs_office_bankaccount#DE02120300000000202051.admin"); + "hs_office_bankaccount#DE02120300000000202051.admin", true); } @Test @@ -409,8 +410,8 @@ class HsOfficeDebitorRepositoryIntegrationTest extends ContextBasedTestWithClean final var givenDebitor = givenSomeTemporaryDebitor("Fourth", "fifth contact", "Fourth", "fih"); assertThatDebitorIsVisibleForUserWithRole( givenDebitor, - "hs_office_relation#HostsharingeG-with-PARTNER-FourtheG.agent"); - assertThatDebitorActuallyInDatabase(givenDebitor); + "hs_office_relation#HostsharingeG-with-PARTNER-FourtheG.agent", true); + assertThatDebitorActuallyInDatabase(givenDebitor, true); // when final var result = jpaAttempt.transacted(() -> { @@ -423,7 +424,7 @@ class HsOfficeDebitorRepositoryIntegrationTest extends ContextBasedTestWithClean result.assertSuccessful(); assertThatDebitorIsVisibleForUserWithRole( result.returnedValue(), - "global#global.admin"); + "global#global.admin", true); // ... bank-account role was removed from previous bank-account admin: assertThatDebitorIsNotVisibleForUserWithRole( @@ -438,8 +439,8 @@ class HsOfficeDebitorRepositoryIntegrationTest extends ContextBasedTestWithClean final var givenDebitor = givenSomeTemporaryDebitor("Fourth", "eighth", "Fourth", "eig"); assertThatDebitorIsVisibleForUserWithRole( givenDebitor, - "hs_office_relation#HostsharingeG-with-PARTNER-FourtheG.agent"); - assertThatDebitorActuallyInDatabase(givenDebitor); + "hs_office_relation#HostsharingeG-with-PARTNER-FourtheG.agent", true); + assertThatDebitorActuallyInDatabase(givenDebitor, true); // when final var result = jpaAttempt.transacted(() -> { @@ -458,10 +459,10 @@ class HsOfficeDebitorRepositoryIntegrationTest extends ContextBasedTestWithClean // given context("superuser-alex@hostsharing.net"); final var givenDebitor = givenSomeTemporaryDebitor("Fourth", "ninth", "Fourth", "nin"); + assertThatDebitorActuallyInDatabase(givenDebitor, true); assertThatDebitorIsVisibleForUserWithRole( givenDebitor, - "hs_office_contact#ninthcontact.admin"); - assertThatDebitorActuallyInDatabase(givenDebitor); + "hs_office_contact#ninthcontact.admin", false); // when final var result = jpaAttempt.transacted(() -> { @@ -471,21 +472,20 @@ class HsOfficeDebitorRepositoryIntegrationTest extends ContextBasedTestWithClean }); // then - // FIXME: This error message would be better: - // result.assertExceptionWithRootCauseMessage(JpaSystemException.class, - // "[403] Subject ", " is not allowed to update hs_office_debitor uuid"); result.assertExceptionWithRootCauseMessage( JpaObjectRetrievalFailureException.class, + // this technical error message gets translated to a [403] error at the controller level "Unable to find net.hostsharing.hsadminng.hs.office.bankaccount.HsOfficeBankAccountEntity with id "); } - private void assertThatDebitorActuallyInDatabase(final HsOfficeDebitorEntity saved) { + private void assertThatDebitorActuallyInDatabase(final HsOfficeDebitorEntity saved, final boolean withPartner) { final var found = debitorRepo.findByUuid(saved.getUuid()); assertThat(found).isNotEmpty(); found.ifPresent(foundEntity -> { em.refresh(foundEntity); + Hibernate.initialize(foundEntity); assertThat(foundEntity).isNotSameAs(saved); - if ( saved.getPartner() != null) { // FIXME: check, why there is no partner for the updated contact + if (withPartner) { assertThat(foundEntity.getPartner()).isNotNull(); } assertThat(foundEntity.getDebitorRel()).extracting(HsOfficeRelationEntity::toString) @@ -495,10 +495,11 @@ class HsOfficeDebitorRepositoryIntegrationTest extends ContextBasedTestWithClean private void assertThatDebitorIsVisibleForUserWithRole( final HsOfficeDebitorEntity entity, - final String assumedRoles) { + final String assumedRoles, + final boolean withPartner) { jpaAttempt.transacted(() -> { context("superuser-alex@hostsharing.net", assumedRoles); - assertThatDebitorActuallyInDatabase(entity); + assertThatDebitorActuallyInDatabase(entity, withPartner); }).assertSuccessful(); } diff --git a/src/test/java/net/hostsharing/hsadminng/hs/office/migration/ImportOfficeData.java b/src/test/java/net/hostsharing/hsadminng/hs/office/migration/ImportOfficeData.java index 7af2a58c..bb42901d 100644 --- a/src/test/java/net/hostsharing/hsadminng/hs/office/migration/ImportOfficeData.java +++ b/src/test/java/net/hostsharing/hsadminng/hs/office/migration/ImportOfficeData.java @@ -1171,11 +1171,6 @@ class Record { return value == null || value.isBlank(); } - Byte getByte(final String columnName) { - final String value = getString(columnName); - return isNotBlank(value) ? Byte.valueOf(value.trim()) : 0; - } - boolean getBoolean(final String columnName) { final String value = getString(columnName); return isNotBlank(value) &&