From e9d566e4972cc0963a0fd5f08f93a95c09e4b41b Mon Sep 17 00:00:00 2001 From: Vivek Ratnavel Subramanian Date: Sat, 6 Aug 2022 09:15:56 -0700 Subject: [PATCH] Fix #6587 Backend: Announcement start and end time should not clash with another announcement (#6618) --- .../exception/CatalogExceptionMessage.java | 8 + .../catalog/jdbi3/CollectionDAO.java | 17 ++ .../catalog/jdbi3/FeedRepository.java | 43 +++- .../resources/feeds/FeedResourceTest.java | 203 +++++++++++++++++- .../AddAnnouncementModal.tsx | 2 +- 5 files changed, 264 insertions(+), 9 deletions(-) diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/exception/CatalogExceptionMessage.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/exception/CatalogExceptionMessage.java index 2009c5ae09e..9298ccc021e 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/exception/CatalogExceptionMessage.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/exception/CatalogExceptionMessage.java @@ -146,4 +146,12 @@ public final class CatalogExceptionMessage { public static String invalidTeamOwner(TeamType teamType) { return String.format("Team of type %s can't own entities. Only Team of type Group can own entities.", teamType); } + + public static String announcementOverlap() { + return "There is already an announcement scheduled that overlaps with the given start time and end time"; + } + + public static String announcementInvalidStartTime() { + return "Announcement start time must be earlier than the end time"; + } } diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/CollectionDAO.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/CollectionDAO.java index 48ea9668cbd..2cc442855a8 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/CollectionDAO.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/CollectionDAO.java @@ -618,6 +618,23 @@ public interface CollectionDAO { return listBefore(limit, before, status, resolved, type); } + default List listAnnouncementBetween(String entityId, long startTs, long endTs) { + return listAnnouncementBetween(null, entityId, startTs, endTs); + } + + @SqlQuery( + "SELECT json FROM thread_entity " + + "WHERE type='Announcement' AND (:threadId IS NULL OR id != :threadId) " + + "AND entityId = :entityId " + + "AND (( :startTs >= announcementStart AND :startTs < announcementEnd) " + + "OR (:endTs > announcementStart AND :endTs < announcementEnd) " + + "OR (:startTs <= announcementStart AND :endTs >= announcementEnd))") + List listAnnouncementBetween( + @Bind("threadId") String threadId, + @Bind("entityId") String entityId, + @Bind("startTs") long startTs, + @Bind("endTs") long endTs); + @ConnectionAwareSqlQuery( value = "SELECT json FROM thread_entity " diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/FeedRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/FeedRepository.java index 68795bf5d2e..0a8f95bfc46 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/FeedRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/FeedRepository.java @@ -37,6 +37,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Comparator; import java.util.List; +import java.util.Objects; import java.util.Optional; import java.util.UUID; import java.util.concurrent.atomic.AtomicInteger; @@ -68,6 +69,7 @@ import org.openmetadata.catalog.resources.feeds.FeedResource; import org.openmetadata.catalog.resources.feeds.FeedUtil; import org.openmetadata.catalog.resources.feeds.MessageParser; import org.openmetadata.catalog.resources.feeds.MessageParser.EntityLink; +import org.openmetadata.catalog.type.AnnouncementDetails; import org.openmetadata.catalog.type.Column; import org.openmetadata.catalog.type.EntityReference; import org.openmetadata.catalog.type.Include; @@ -127,6 +129,18 @@ public class FeedRepository { thread.withTask(thread.getTask().withId(getNextTaskId())); } + // if thread is of type "announcement", validate start and end time + if (thread.getType().equals(ThreadType.Announcement)) { + validateAnnouncement(thread.getAnnouncement()); + long startTime = thread.getAnnouncement().getStartTime(); + long endTime = thread.getAnnouncement().getEndTime(); + List announcements = dao.feedDAO().listAnnouncementBetween(entityId.toString(), startTime, endTime); + if (announcements.size() > 0) { + // There is already an announcement that overlaps the new one + throw new IllegalArgumentException(CatalogExceptionMessage.announcementOverlap()); + } + } + // Insert a new thread dao.feedDAO().insert(JsonUtils.pojoToJson(thread)); @@ -749,6 +763,21 @@ public class FeedRepository { updated.getTask().getAssignees().sort(compareEntityReference); } + if (updated.getAnnouncement() != null) { + validateAnnouncement(updated.getAnnouncement()); + // check if the announcement start and end time clashes with other existing announcements + List announcements = + dao.feedDAO() + .listAnnouncementBetween( + id.toString(), + updated.getEntityId().toString(), + updated.getAnnouncement().getStartTime(), + updated.getAnnouncement().getEndTime()); + if (announcements.size() > 0) { + throw new IllegalArgumentException(CatalogExceptionMessage.announcementOverlap()); + } + } + // Update the attributes String change = patchUpdate(original, updated) ? RestUtil.ENTITY_UPDATED : RestUtil.ENTITY_NO_CHANGE; sortPosts(updated); @@ -756,9 +785,15 @@ public class FeedRepository { return new PatchResponse<>(Status.OK, updatedHref, change); } + private void validateAnnouncement(AnnouncementDetails announcementDetails) { + if (announcementDetails.getStartTime() >= announcementDetails.getEndTime()) { + throw new IllegalArgumentException(CatalogExceptionMessage.announcementInvalidStartTime()); + } + } + private void restorePatchAttributes(Thread original, Thread updated) { // Patch can't make changes to following fields. Ignore the changes - updated.withId(original.getId()).withAbout(original.getAbout()); + updated.withId(original.getId()).withAbout(original.getAbout()).withType(original.getType()); } private void restorePatchAttributes(Post original, Post updated) { @@ -810,13 +845,17 @@ public class FeedRepository { } private boolean fieldsChanged(Thread original, Thread updated) { - // Patch supports isResolved, message, task assignees, and reactions for now + // Patch supports isResolved, message, task assignees, reactions, and announcements for now return !original.getResolved().equals(updated.getResolved()) || !original.getMessage().equals(updated.getMessage()) || (Collections.isEmpty(original.getReactions()) && !Collections.isEmpty(updated.getReactions())) || (!Collections.isEmpty(original.getReactions()) && Collections.isEmpty(updated.getReactions())) || original.getReactions().size() != updated.getReactions().size() || !original.getReactions().containsAll(updated.getReactions()) + || (original.getAnnouncement() != null + && (!original.getAnnouncement().getDescription().equals(updated.getAnnouncement().getDescription()) + || !Objects.equals(original.getAnnouncement().getStartTime(), updated.getAnnouncement().getStartTime()) + || !Objects.equals(original.getAnnouncement().getEndTime(), updated.getAnnouncement().getEndTime()))) || (original.getTask() != null && (original.getTask().getAssignees().size() != updated.getTask().getAssignees().size() || !original.getTask().getAssignees().containsAll(updated.getTask().getAssignees()))); diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/feeds/FeedResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/feeds/FeedResourceTest.java index 523c88f814b..522a5f37c87 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/feeds/FeedResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/feeds/FeedResourceTest.java @@ -25,6 +25,8 @@ import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.openmetadata.catalog.exception.CatalogExceptionMessage.announcementInvalidStartTime; +import static org.openmetadata.catalog.exception.CatalogExceptionMessage.announcementOverlap; import static org.openmetadata.catalog.exception.CatalogExceptionMessage.entityNotFound; import static org.openmetadata.catalog.exception.CatalogExceptionMessage.noPermission; import static org.openmetadata.catalog.resources.EntityResourceTest.USER_ADDRESS_TAG_LABEL; @@ -426,8 +428,8 @@ public class FeedResourceTest extends CatalogApplicationTest { AnnouncementDetails announcementDetails = new AnnouncementDetails() .withDescription("First announcement") - .withStartTime(now.plusDays(1L).toEpochSecond(ZoneOffset.UTC)) - .withEndTime(now.plusDays(2L).toEpochSecond(ZoneOffset.UTC)); + .withStartTime(now.plusDays(10L).toEpochSecond(ZoneOffset.UTC)) + .withEndTime(now.plusDays(11L).toEpochSecond(ZoneOffset.UTC)); CreateThread create = create() .withMessage("Announcement One") @@ -436,6 +438,9 @@ public class FeedResourceTest extends CatalogApplicationTest { Map userAuthHeaders = authHeaders(USER.getEmail()); createAndCheck(create, userAuthHeaders); + announcementDetails + .withStartTime(now.plusDays(12L).toEpochSecond(ZoneOffset.UTC)) + .withEndTime(now.plusDays(13L).toEpochSecond(ZoneOffset.UTC)); create = create() .withMessage("Announcement Two") @@ -445,8 +450,8 @@ public class FeedResourceTest extends CatalogApplicationTest { // create one expired announcement announcementDetails - .withStartTime(now.minusDays(3L).toEpochSecond(ZoneOffset.UTC)) - .withEndTime(now.minusMinutes(1L).toEpochSecond(ZoneOffset.UTC)); + .withStartTime(now.minusDays(30L).toEpochSecond(ZoneOffset.UTC)) + .withEndTime(now.minusDays(20L).toEpochSecond(ZoneOffset.UTC)); create = create() .withMessage("Announcement Three") @@ -457,8 +462,8 @@ public class FeedResourceTest extends CatalogApplicationTest { // create one active announcement announcementDetails .withDescription("Active Announcement") - .withStartTime(now.minusDays(2L).toEpochSecond(ZoneOffset.UTC)) - .withEndTime(now.plusDays(5L).toEpochSecond(ZoneOffset.UTC)); + .withStartTime(now.minusDays(1L).toEpochSecond(ZoneOffset.UTC)) + .withEndTime(now.plusDays(1L).toEpochSecond(ZoneOffset.UTC)); create = create() .withMessage("Announcement Four") @@ -497,6 +502,65 @@ public class FeedResourceTest extends CatalogApplicationTest { assertEquals(totalAnnouncementCount + 3, announcements.getData().size()); } + @Test + void post_invalidAnnouncement_400() throws IOException { + // create two announcements with same start time in the future + LocalDateTime now = LocalDateTime.now(); + AnnouncementDetails announcementDetails = + new AnnouncementDetails() + .withDescription("First announcement") + .withStartTime(now.plusDays(3L).toEpochSecond(ZoneOffset.UTC)) + .withEndTime(now.plusDays(5L).toEpochSecond(ZoneOffset.UTC)); + CreateThread create = + create() + .withMessage("Announcement One") + .withType(ThreadType.Announcement) + .withAnnouncementDetails(announcementDetails); + Map userAuthHeaders = authHeaders(USER.getEmail()); + createAndCheck(create, userAuthHeaders); + + CreateThread create2 = + create() + .withMessage("Announcement Two") + .withType(ThreadType.Announcement) + .withAnnouncementDetails(announcementDetails); + + // create announcement with same start and end time + assertResponse(() -> createThread(create2, userAuthHeaders), BAD_REQUEST, announcementOverlap()); + + // create announcement with start time > end time + announcementDetails + .withStartTime(now.plusDays(3L).toEpochSecond(ZoneOffset.UTC)) + .withEndTime(now.plusDays(2L).toEpochSecond(ZoneOffset.UTC)); + CreateThread create3 = create2.withAnnouncementDetails(announcementDetails); + assertResponse(() -> createThread(create3, userAuthHeaders), BAD_REQUEST, announcementInvalidStartTime()); + + // create announcement with overlaps + announcementDetails + .withStartTime(now.plusDays(2L).toEpochSecond(ZoneOffset.UTC)) + .withEndTime(now.plusDays(6L).toEpochSecond(ZoneOffset.UTC)); + CreateThread create4 = create2.withAnnouncementDetails(announcementDetails); + assertResponse(() -> createThread(create4, userAuthHeaders), BAD_REQUEST, announcementOverlap()); + + announcementDetails + .withStartTime(now.plusDays(3L).plusHours(2L).toEpochSecond(ZoneOffset.UTC)) + .withEndTime(now.plusDays(4L).toEpochSecond(ZoneOffset.UTC)); + CreateThread create5 = create2.withAnnouncementDetails(announcementDetails); + assertResponse(() -> createThread(create5, userAuthHeaders), BAD_REQUEST, announcementOverlap()); + + announcementDetails + .withStartTime(now.plusDays(2L).plusHours(12L).toEpochSecond(ZoneOffset.UTC)) + .withEndTime(now.plusDays(4L).toEpochSecond(ZoneOffset.UTC)); + CreateThread create6 = create2.withAnnouncementDetails(announcementDetails); + assertResponse(() -> createThread(create6, userAuthHeaders), BAD_REQUEST, announcementOverlap()); + + announcementDetails + .withStartTime(now.plusDays(4L).plusHours(12L).toEpochSecond(ZoneOffset.UTC)) + .withEndTime(now.plusDays(6L).toEpochSecond(ZoneOffset.UTC)); + CreateThread create7 = create2.withAnnouncementDetails(announcementDetails); + assertResponse(() -> createThread(create7, userAuthHeaders), BAD_REQUEST, announcementOverlap()); + } + @Test void put_resolveTask_description_200() throws IOException { CreateTaskDetails taskDetails = @@ -845,6 +909,133 @@ public class FeedResourceTest extends CatalogApplicationTest { assertEquals(TEST_USER_NAME, patched.getUpdatedBy()); } + @Test + void patch_announcement_200() throws IOException { + LocalDateTime now = LocalDateTime.now(); + AnnouncementDetails announcementDetails = + new AnnouncementDetails() + .withDescription("First announcement") + .withStartTime(now.plusDays(5L).toEpochSecond(ZoneOffset.UTC)) + .withEndTime(now.plusDays(6L).toEpochSecond(ZoneOffset.UTC)); + CreateThread create = + create() + .withMessage("Announcement One") + .withType(ThreadType.Announcement) + .withAnnouncementDetails(announcementDetails); + Map userAuthHeaders = authHeaders(USER.getEmail()); + Thread thread = createAndCheck(create, userAuthHeaders); + + String originalJson = JsonUtils.pojoToJson(thread); + + long startTs = now.plusDays(6L).toEpochSecond(ZoneOffset.UTC); + long endTs = now.plusDays(7L).toEpochSecond(ZoneOffset.UTC); + announcementDetails.withStartTime(startTs).withEndTime(endTs); + Thread updated = thread.withAnnouncement(announcementDetails); + + Thread patched = patchThreadAndCheck(updated, originalJson, TEST_AUTH_HEADERS); + + assertNotEquals(patched.getUpdatedAt(), thread.getUpdatedAt()); + assertEquals(TEST_USER_NAME, patched.getUpdatedBy()); + assertEquals(startTs, patched.getAnnouncement().getStartTime()); + assertEquals(endTs, patched.getAnnouncement().getEndTime()); + + Thread thread1 = getThread(thread.getId(), ADMIN_AUTH_HEADERS); + assertEquals(startTs, thread1.getAnnouncement().getStartTime()); + assertEquals(endTs, thread1.getAnnouncement().getEndTime()); + + // patch description + originalJson = JsonUtils.pojoToJson(thread1); + AnnouncementDetails announcementDetails1 = thread1.getAnnouncement().withDescription("New Description"); + updated = thread1.withAnnouncement(announcementDetails1); + patched = patchThreadAndCheck(updated, originalJson, TEST_AUTH_HEADERS); + assertEquals("New Description", patched.getAnnouncement().getDescription()); + + Thread thread2 = getThread(thread.getId(), ADMIN_AUTH_HEADERS); + assertEquals("New Description", thread2.getAnnouncement().getDescription()); + } + + @Test + void patch_invalidAnnouncement_400() throws IOException { + LocalDateTime now = LocalDateTime.now(); + AnnouncementDetails announcementDetails = + new AnnouncementDetails() + .withDescription("First announcement") + .withStartTime(now.plusDays(53L).toEpochSecond(ZoneOffset.UTC)) + .withEndTime(now.plusDays(55L).toEpochSecond(ZoneOffset.UTC)); + CreateThread create = + create() + .withMessage("Announcement One") + .withType(ThreadType.Announcement) + .withAnnouncementDetails(announcementDetails); + Map userAuthHeaders = authHeaders(USER.getEmail()); + Thread thread1 = createAndCheck(create, userAuthHeaders); + + announcementDetails + .withStartTime(now.plusDays(57L).toEpochSecond(ZoneOffset.UTC)) + .withEndTime(now.plusDays(59L).toEpochSecond(ZoneOffset.UTC)); + CreateThread create2 = + create() + .withMessage("Announcement Two") + .withType(ThreadType.Announcement) + .withAnnouncementDetails(announcementDetails); + Thread thread2 = createAndCheck(create, userAuthHeaders); + + String originalJson = JsonUtils.pojoToJson(thread2); + + // patch announcement2 with same start and end time as announcement1 + Thread updated = thread2.withAnnouncement(thread1.getAnnouncement()); + + assertResponse( + () -> patchThread(thread2.getId(), originalJson, updated, userAuthHeaders), BAD_REQUEST, announcementOverlap()); + + // create announcement with start time > end time + announcementDetails + .withStartTime(now.plusDays(58L).toEpochSecond(ZoneOffset.UTC)) + .withEndTime(now.plusDays(57L).toEpochSecond(ZoneOffset.UTC)); + Thread updated2 = thread2.withAnnouncement(announcementDetails); + assertResponse( + () -> patchThread(thread2.getId(), originalJson, updated2, userAuthHeaders), + BAD_REQUEST, + announcementInvalidStartTime()); + + // create announcement with overlaps + announcementDetails + .withStartTime(now.plusDays(52L).toEpochSecond(ZoneOffset.UTC)) + .withEndTime(now.plusDays(56L).toEpochSecond(ZoneOffset.UTC)); + Thread updated3 = thread2.withAnnouncement(announcementDetails); + assertResponse( + () -> patchThread(thread2.getId(), originalJson, updated3, userAuthHeaders), + BAD_REQUEST, + announcementOverlap()); + + announcementDetails + .withStartTime(now.plusDays(53L).plusHours(2L).toEpochSecond(ZoneOffset.UTC)) + .withEndTime(now.plusDays(54L).toEpochSecond(ZoneOffset.UTC)); + Thread updated4 = thread2.withAnnouncement(announcementDetails); + assertResponse( + () -> patchThread(thread2.getId(), originalJson, updated4, userAuthHeaders), + BAD_REQUEST, + announcementOverlap()); + + announcementDetails + .withStartTime(now.plusDays(52L).plusHours(12L).toEpochSecond(ZoneOffset.UTC)) + .withEndTime(now.plusDays(54L).toEpochSecond(ZoneOffset.UTC)); + Thread updated5 = thread2.withAnnouncement(announcementDetails); + assertResponse( + () -> patchThread(thread2.getId(), originalJson, updated5, userAuthHeaders), + BAD_REQUEST, + announcementOverlap()); + + announcementDetails + .withStartTime(now.plusDays(54L).plusHours(12L).toEpochSecond(ZoneOffset.UTC)) + .withEndTime(now.plusDays(56L).toEpochSecond(ZoneOffset.UTC)); + Thread updated6 = thread2.withAnnouncement(announcementDetails); + assertResponse( + () -> patchThread(thread2.getId(), originalJson, updated6, userAuthHeaders), + BAD_REQUEST, + announcementOverlap()); + } + @Test void patch_thread_not_allowed_fields() throws IOException { // create a thread diff --git a/openmetadata-ui/src/main/resources/ui/src/components/Modals/AnnouncementModal/AddAnnouncementModal.tsx b/openmetadata-ui/src/main/resources/ui/src/components/Modals/AnnouncementModal/AddAnnouncementModal.tsx index 20f95c236cf..cc9f08483c7 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/Modals/AnnouncementModal/AddAnnouncementModal.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/Modals/AnnouncementModal/AddAnnouncementModal.tsx @@ -97,7 +97,7 @@ const AddAnnouncementModal: FC = ({ type: 'primary', htmlType: 'submit', }} - okText="Sumbit" + okText="Submit" title="Make an announcement" visible={open} width={620}